[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH] xvfork: override die_func to _exit on child processes
From:       Denys Vlasenko <vda.linux () googlemail ! com>
Date:       2024-01-01 23:56:58
Message-ID: CAK1hOcPEBbdRXjoMnnKSQy5dKUrC_mzX7+tK8rM63obJStHikQ () mail ! gmail ! com
[Download RAW message or body]

On Fri, Dec 15, 2023 at 6:57 AM Dominique Martinet
<dominique.martinet@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

[prev in list] [next in list] [prev in thread] [next in thread]