Re: [PATCH] s6-uevent-spawner: Fix possibly delaying uevents

From: Olivier Brunel <jjk_at_jjacky.com>
Date: Sun, 14 Jun 2015 21:57:45 +0200

On 06/14/15 21:11, Laurent Bercot wrote:
> On 14/06/2015 14:37, Olivier Brunel wrote:
>> Because of the buffered IO, the possible scenario could occur:
>> - netlink uevents (plural) occur, i.e. data ready on stdin
>> - iopause triggered, handle_stdin() called. The first uevent is
>> processed, child
>> launched, we're waiting for a signal
>> - SIGCHLD occurs, we're back to iopausing on stdin again, only it's
>> not ready
>> yet; Because we've read it all already and still have unprocessed data
>> (uevents) on our own internal buffer (buffer_0)
>
> Right, thanks for the catch. I usually avoid that trap, but meh.
> I committed a simpler change than your patch, please tell me if it fixes
> things for you.

Haven't tested it, but I'm sure it'll "work". In fact I had done
something similar at first, but changed it because I don't think this is
quite correct.

That is, in your test now you're using x[1] even though it might not
have been used in the iopause call before, so while I guess this isn't
random memory, it doesn't really feel right either.

Moreover, imagine this very likely scenario:
- uevent comes in, handle_stdin is called, children forked, and we're
iopausing in x[0] (selfpipe) only.
- SIGCHLD occurs, handle_signals is called, then you test on x[1] which
should still be as it was last time it was actually used, so stating
stdin is readable even though it isn't anymore... and we're gonna block
in handle_stdin.

Not that it's a big deal since at this point we were gonna iopause to
wait for that fd to become readable anyways, since we're not expecting
any signal (as we don't have children anymore). So in effect this works
basically the same, but you're using memory (x[1]) that you probably
shouldn't, and effectively trying to read (and blocking) in handle_stdin
despite having an iopause in place.
You might as well change your test to "if (cont) handle_stdin(...) ;"
then, it'll surely work just as well. But it doesn't feel right, to me
at least, since we have a loop w/ iopause, hence why I went with the bit
longer (and maybe less elegant) version.

-j
Received on Sun Jun 14 2015 - 19:57:45 UTC