Bugzilla – Bug 1221479
systemd-sysv-generator crashes in "free" when /etc/init.d/ contains scripts starting with "boot."
Last modified: 2024-03-27 08:11:50 UTC
Created attachment 873545 [details] core file obtained by running "/usr/lib/systemd/system-generators/systemd-sysv-generator /run/systemd/generator.late/" with boot.ksm file in /etc/init.d I have a crash when my /etc/init.d/ directory contains scripts starting with "boot". I looked at the sources, I see there's a special case but I don't know exactly why it crashes but I see that boot. is specially handled. File: src/sysv-generator/sysv-generator.c (see *********** ) static int sysv_translate_facility(SysvStub *s, unsigned line, const char *name, char **ret) { /* We silently ignore the $ prefix here. According to the LSB * spec it simply indicates whether something is a * standardized name or a distribution-specific one. Since we * just follow what already exists and do not introduce new * uses or names we don't care who introduced a new name. */ static const char * const table[] = { /* LSB defined facilities */ "local_fs", NULL, "network", SPECIAL_NETWORK_ONLINE_TARGET, "named", SPECIAL_NSS_LOOKUP_TARGET, "portmap", SPECIAL_RPCBIND_TARGET, "remote_fs", SPECIAL_REMOTE_FS_TARGET, "syslog", NULL, "time", SPECIAL_TIME_SYNC_TARGET, "all", SPECIAL_DEFAULT_TARGET, }; _cleanup_free_ char *filename = NULL; const char *n; char *e, *m; int r; assert(name); assert(s); assert(ret); r = path_extract_filename(s->path, &filename); ........................ ********************************** ********************************** ********************************** /* Strip "boot." prefix from file name for comparison (Suse specific) */ e = startswith(filename, "boot."); if (e) filename += 5; ********************************** ********************************** ********************************** ............. So having boot in file name only adds 5 to filename. Only this seems to be the difference that makes the code crash. I've attached the core file obtained by running "/usr/lib/systemd/system-generators/systemd-sysv-generator /run/systemd/generator.late/".
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 ?