[PATCH v2] fixdep: add fstat error handling
When `fstat` fails, `st` is left uninitialised. In our case, Ben Kohler
noticed our release media builds were failing in Gentoo on x86 when building
busybox with occasional SIGBUS. This turned out to be EOVERFLOW (from 32-bit
ino_t) which wasn't being reported because nothing was checking the return value
from `fstat`.
Fix that to avoid UB (use of uninit var) and to give a more friendly
error to the user.
This actually turns out to be fixed already in the kernel from back in
2010 [0] and 2016 [1].
[0]
https://github.com/torvalds/linux/commit/a3ba81131aca243bfecfa78c42edec0cd69f72d6
[1]
https://github.com/torvalds/linux/commit/46fe94ad18aa7ce6b3dad8c035fb538942020f2b
Reported-by: Ben Kohler
Signed-off-by: Sam James
---
scripts/basic/fixdep.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 66be73aad..ebc715730 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -292,7 +292,11 @@ void do_config_file(char *filename)
perror(filename);
exit(2);
}
- fstat(fd, );
+ if (fstat(fd, ) < 0) {
+ fprintf(stderr, "fixdep: fstat");
+ perror(filename);
+ exit(2);
+ }
if (st.st_size == 0) {
close(fd);
return;
@@ -368,7 +372,11 @@ void print_deps(void)
perror(depfile);
exit(2);
}
- fstat(fd, );
+ if (fstat(fd, ) < 0) {
+ fprintf(stderr, "fixdep: fstat");
+ perror(depfile);
+ exit(2);
+ }
if (st.st_size == 0) {
fprintf(stderr,"fixdep: %s is empty\n",depfile);
close(fd);
--
2.44.0
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] switch_root: remove /init check
Linus Heckemann 於 2024年4月23日 星期二寫道： > > I don't really see the existence of /init being so critical for > this check given that we check below that it's a ramfs or tmpfs, which > seems to me to be enough that people won't be destroying filesystems > they cared a great deal about. > The ramfs/tmpfs check was good as it won't destroy any _permanent_ file the user could have, but it wouldn't hurt either when there is another sanity check. >> Perhaps a better approach is to check the existence of what's specified in >> the "rdinit" parameter instead. > > That would introduce an additional dependency on /proc being mounted and > require additional parsing. I don't think the check is that necessary, > again because we have the /-is-ramfs-or-tmpfs check. But if you do think > we need it I can rewrite the patch to check for rdinit= on cmdline as well. > /proc should be mounted by most init systems anyway. But we can skip the check when /proc doesn't exist, just in case. The logic would be roughly like this: If "/proc/cmdline" exists Read the "rdinit" parameter from "/proc/cmdline"; if it's unspecified, default to "/init" If the file in "rdinit" doesn't exist, stop. Else Skip the "rdinit" existence check and continue the switch_root process ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] switch_root: remove /init check
Kang-Che Sung writes:
> Linus Heckemann 於 2024年4月19日 星期五寫道：
>> We were having some difficulty switching out of our custom initramfs
>> into the final filesystem, with the error "message '/init' is not a
>> regular file". We were confused as to why it was looking for `/init`
>> -- we didn't have `/init`, neither in our initrd nor in the
>> destination filesystem -- we were using the rdinit= command line
>> parameter to provide an alternative path for the init in the initrd,
>> and the target init was determined by userspace.
>>
>> Thankfully, the error message was fairly clear and easy to find in the
>> source!
>>
>> We thus propose removing this check, since not all initrds have their
>> init at /init -- setting rdinit= on the kernel command line allows
>> using an alternative path. Thus, this check can prevent switching root
>> in a scenario where it would be perfectly appropriate.
>> ---
>> util-linux/switch_root.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/util-linux/switch_root.c b/util-linux/switch_root.c
>> index 14139736e..ef6669f5c 100644
>> --- a/util-linux/switch_root.c
>> +++ b/util-linux/switch_root.c
>> @@ -238,9 +238,6 @@ int switch_root_main(int argc UNUSED_PARAM, char
> **argv)
>> // Additional sanity checks: we're about to rm -rf /, so be
> REALLY SURE
>> // we mean it. I could make this a CONFIG option, but I would get
> email
>> // from all the people who WILL destroy their filesystems.
>> - if (stat("/init", ) != 0 || !S_ISREG(st.st_mode)) {
>> - bb_error_msg_and_die("'%s' is not a regular file",
> "/init");
>> - }
>> statfs("/", ); // this never fails
>> if ((unsigned)stfs.f_type != RAMFS_MAGIC
>> && (unsigned)stfs.f_type != TMPFS_MAGIC
>> --
>> 2.42.0
>
> Did you read the code comments on the lines above what you deleted? It's a
> sanity check before deleting everything on the "/" filesystem.
I did! I don't really see the existence of /init being so critical for
this check given that we check below that it's a ramfs or tmpfs, which
seems to me to be enough that people won't be destroying filesystems
they cared a great deal about.
> Perhaps a better approach is to check the existence of what's specified in
> the "rdinit" parameter instead.
That would introduce an additional dependency on /proc being mounted and
require additional parsing. I don't think the check is that necessary,
again because we have the /-is-ramfs-or-tmpfs check. But if you do think
we need it I can rewrite the patch to check for rdinit= on cmdline as well.
Cheers
Linus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
