On Fri, Dec 15, 2023 at 6:57 AM Dominique Martinet <dominique.marti...@atmark-techno.com> wrote: > > > > For this particular problem die_func could just be overriden in > > > > time.c's run_command after vfork, but this problem actually came up > > > > before in shell/hush.c as per the fflush_and__exit comment so it seems > > > > more appropriate to do this globally in the xvfork macro and get rid of > > > > this whole class of bugs forever. > > > > > > Maybe we can always use _exit(), > > > or fflush_all(); _exit() in xfunc_die()? > > > > hmm, exit does a bunch of stuff: > > - we apparently use atexit() in a few applets, so these would need the > > real exit() to get invoked > > - any lib destructor would also be called, I'm not sure we rely on any > > so that's probably fine. Same with on_exit(), not used directly. > > - The man page for exit(3) on my system also says files created by > > tmpfile(3) are removed; we don't seem to use it either. > > - flushing and closinig all FILEs; I agree close itself isn't needed, > > fflush_all() as you've suggested is probably enough... And hush.c has > > been calling it after vfork so it's probably fine? because while the > > address space is shared, that takes and releases locks when it's done, > > so while it's not explicitely safe it's probably ok. And fflush is > > definitely needed in the normal case. > > > > I'm not sure there's anything else that needs to be done on exit? > > > > > > So that leaves atexit hooks which we need in some applets (mount, sed -- > > the one in libpwdgrp is just a free "to make valgrind happy" so can be > > skipped); if we update these two to override die_func instead (don't > > even need to use exit there, they can just register their hook as > > die_func instead of atexit), then I guess it would be ok to do > > `fflush_all(); _exit()` in xfunc_die(), but I'm not 100% comfortable > > with that so that'll require a bunch of testing. > > > > I can update the patch if you really prefer this, but I won't be able to > > test much more than running the test suite on x86_64/debian and alpines > > (I guess it's a good excuse to add a new test item for 'time > > notavalidcommand' at the same time), it'll probably require a bit more > > validiation. > > What would you like me to do with this, should I send a v2 that tries to > default to `flush_all(); exit()` in xfunc_die()?
Yes, I think we should try it. Conditional on musl. Something like this in libbb/xfunc_die.c: +//musl has no __MUSL__ or similar define to check for, +//but its <sys/types.h> has these lines: +// #define __NEED_fsblkcnt_t +// #define __NEED_fsfilcnt_t +#if defined(__linux__) && defined(__NEED_fsblkcnt_t) && defined(__NEED_fsfilcnt_t) +# define LIBC_IS_MUSL 1 +# include <sys/syscall.h> +#else +# define LIBC_IS_MUSL 0 +#endif void FAST_FUNC xfunc_die(void) { if (die_func) die_func(); +#if LIBC_IS_MUSL +// We may be in the child after vfork(). +// On musl, sometime between 1.1.20 and 1.1.22, exit() after vfork() +// stopped working properly: it takes init_fini_lock but does not release it +// before dying, which makes subsequent exit in parent process hang +// trying to take the same lock again. Thus, use _exit instead: fflush_all(); _exit(xfunc_error_retval); +#else exit(xfunc_error_retval); +#endif } _______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox