Bug 1221479

I am using openSUSE 15.6 Beta in order to preview our migration to a newer platform (SLES15SP6) and this is what I discovered.

Please let me know if you require additional information.

I understood that the problem is the pointer that is incremented by 5 in case a file starting with "boot.". This makes that memory area unsuitable for being released via "free" since "free" requires the original allocated pointer, not one incremented by 5.

Our team can provide a patch in required.

Thanks for the report.

Indeed there's a bug and your analysis is correct.

However I'm not sure how the broken code should be fixed. We could make sure to pass the original allocated pointer to free() but I think skipping the "boot." prefix might be wrong in the first place.

In my understanding, this chunk of code:

        /* Strip "boot." prefix from file name for comparison (Suse specific) */
        e = startswith(filename, "boot.");
        if (e)
                filename += 5;

        /* Strip ".sh" suffix from file name for comparison */
        e = endswith(filename, ".sh");
        if (e)
                *e = '\0';

        /* Names equaling the file name of the services are redundant */
        if (streq_ptr(n, filename)) {
                *ret = NULL;
                return 0;
        }

is supposed to deal with cases where a SysV init script has a dependency on itself and in this case it should be ignored.

For example, consider the script "boot.localfs" that would contain the following dependency:

  $ cat /etc/init.d/boot.localfs
  ### BEGIN INIT INFO
  # [...]
  # Required-Start:    boot.localfs
  # [...]
  ### END INIT INFO

If the "boot." prefix is skipped then the code compares "boot.localfs" with "localfs", which are different strings, and assumes that the dependency should be added to the generated service unit.

I think it would be incorrect, wouldn't it ?

Hi,

We don't have any concern related to the logic behind the code.

The only problem is the pointer that is not properly kept to it's original allocated value.

Our approach would be to use a different variable 
.........

_cleanup_free_ char *filename_allocated = NULL;
char *filename = NULL;

r = path_extract_filename(s->path, &filename_allocated);
filename = filename_allocated;
....
the rest of the code should be unchanged.

Thanks!

(In reply to Iulian Serbanoiu from comment #3)
> We don't have any concern related to the logic behind the code.

The problem is that the current logic is borked.

Here is another example, maybe it will illustrate the problem better:

  $ cat /etc/init.d/boot.test
  #!/bin/bash
  #
  ### BEGIN INIT INFO
  # Required-Start:    boot.test
  ### END INIT INFO

  $ /usr/lib/systemd/system-generators/systemd-sysv-generator /tmp/

  $ cat /tmp/test.service
  # Automatically generated by systemd-sysv-generator

  [Unit]
  ...
  After=test.service

Basically the generator generated 'test.service' with 'After=test.service', which is obviously incorrect and the code I showed in my previous comment is supposed to address this corner case.

So it looks to me that the following patch would be more appropriate:

> @@ -308,11 +308,6 @@ static int sysv_translate_facility(SysvStub *s, unsigned line, const char *name,
>                  return 1;
>          }
> 
> -        /* Strip "boot." prefix from file name for comparison (Suse specific) */
> -        e = startswith(filename, "boot.");
> -        if (e)
> -                filename += 5;
> -
>          /* Strip ".sh" suffix from file name for comparison */
>          e = endswith(filename, ".sh");
>          if (e)

Well, I suppose you're right about that corner case.
We are more interested in having the crash avoided. We are currently patching the package and rebuilding it and we see it as very important, especially for people still holding on to their legacy services.

I submitted a fix which consists of the patch showed in comment #4.

That should address your issue hence I'm closing the bug.

Thank you for your detailed report and your analysis.

I just updated today and the crash is still present

--
localhost:/usr/src/packages # rpm -qi systemd-sysvcompat
Name        : systemd-sysvcompat
Version     : 254.9
Release     : 150600.2.14
Architecture: x86_64
Install Date: Tue Mar 26 18:10:02 2024
Group       : Unspecified
Size        : 74714
License     : LGPL-2.1-or-later
Signature   : RSA/SHA256, Thu Mar 21 15:32:27 2024, Key ID f74f09bc3fa1d6ce
Source RPM  : systemd-254.9-150600.2.14.src.rpm
Build Date  : Thu Mar 21 15:30:09 2024
Build Host  : h01-ch3d
Relocations : (not relocatable)
Packager    : https://www.suse.com/
Vendor      : SUSE LLC <https://www.suse.com/>
URL         : http://www.freedesktop.org/wiki/Software/systemd
Summary     : SySV and LSB init script support for systemd (deprecated)
Description :
This package ships the necessary files that enable minimal SysV and LSB init
scripts support in systemd. It also contains the obsolete SysV init tools
telinit(8) and runlevel(8). You should consider using systemctl(1) instead.

Unless you have a 3rd party application installed on your system that still
relies on such scripts, this package should not be needed at all.

Please note that the content of this package is considered as deprecated.
Distribution: SUSE Linux Enterprise 15
localhost:/usr/src/packages #
--

Is this is the version intended to fix the problem or there's something else?

The fix has not been released yet. It will be probably available when RC1 will be released.

Radoslav, could you confirm ?