Created attachment 93553 [details] [review]
proposed patch

Subject: [PATCH] PolkitAgentSession: fix race between child and io watches

The helper flushes and fdatasyncs stdout and stderr before terminating
but this doesn't guarantee that our io watch is called before our
child watch. This means that we can end up with a successful return
from the helper which we still report as a failure.

To fix this, instead of reporting a failure from the child watch
handler, we'll just unset child_pid.

In addition, we add G_IO_HUP and G_IO_ERR to the conditions we look
for in the io watch so that if the child terminates abnormally we
still run the io watch handler which will complete the session
reporting a failure.

Comment on attachment 93553 [details] [review]
proposed patch

Review of attachment 93553 [details] [review]:
-----------------------------------------------------------------

Ok, but then...it's kind of ugly that the only reason we have a child watch is so we can call kill(pid, SIGTERM) if somehow the helper gets unreferenced.

There's really no point to this because:

1) We control the code to the helper, we know it will terminate
2) Using prctl(PR_SET_PDEATHSIG, SIGTERM) inside the helper is safer (although Linux only)
3) Even better, we should stop using a setuid binary, and fork off a helper process that
   runs as root on early start.

::: src/polkitagent/polkitagentsession.c
@@ +468,4 @@
>        goto out;
>      }
>  
> +  if (!(condition & G_IO_IN))

Don't you mean: if (condition & G_IO_HUP) ?

Otherwise I think we could exit early if the helper hasn't yet written anything.

Created attachment 93662 [details] [review]
proposed patch

(In reply to comment #2)
> Ok, but then...it's kind of ugly that the only reason we have a
child watch
> is so we can call kill(pid, SIGTERM) if somehow the helper gets
unreferenced.

Right, I removed the child watch now.

> ::: src/polkitagent/polkitagentsession.c
> @@ +468,4 @@
> >        goto out;
> >      }
> >
> > +  if (!(condition & G_IO_IN))
>
> Don't you mean: if (condition & G_IO_HUP) ?
>
> Otherwise I think we could exit early if the helper hasn't yet
written
> anything.

Right, but that's what we want because it means we were called because
of a G_IO_HUP and G_IO_ERR.

Anyway, I reworked this to be more explicit by checking for condition
& (G_IO_ERR | G_IO_HUP) before exiting the callback. Note that we
might get both G_IO_IN | G_IO_HUP in the same callback which is
perfectly fine.

(In reply to comment #3)
> Created attachment 93662 [details] [review] [review]
> proposed patch
> 
> (In reply to comment #2)
> > Ok, but then...it's kind of ugly that the only reason we have a
> child watch
> > is so we can call kill(pid, SIGTERM) if somehow the helper gets
> unreferenced.
> 
> Right, I removed the child watch now.

Soo...I was looking at this more, and I was probably wrong in suggesting this, because now we will leak zombies, since we're using DO_NOT_REAP_CHILD, and then not reaping in our code.  (We could fix this by removing the DO_NOT_REAP_CHILD, but I always thought it was ugly to have children reparented to init)

We could just go with your original patch since it's simpler, maybe with the minor fixup below?

Sorry about that...

Created attachment 93677 [details] [review]
merged patch

As penance, here's the merged patch.  Look OK?

(In reply to comment #4)
> (In reply to comment #3)
> > Created attachment 93662 [details] [review] [review] [review]
> > proposed patch
> > 
> > (In reply to comment #2)
> > > Ok, but then...it's kind of ugly that the only reason we have a
> > child watch
> > > is so we can call kill(pid, SIGTERM) if somehow the helper gets
> > unreferenced.
> > 
> > Right, I removed the child watch now.
> 
> Soo...I was looking at this more, and I was probably wrong in suggesting
> this, because now we will leak zombies, since we're using DO_NOT_REAP_CHILD,
> and then not reaping in our code.  (We could fix this by removing the
> DO_NOT_REAP_CHILD, but I always thought it was ugly to have children
> reparented to init)

Nah, I think this is OK. We do run waitpid() in kill_helper() which is always called from complete_session() . How would we leak zombies?

(In reply to comment #6)

> Nah, I think this is OK. We do run waitpid() in kill_helper() which is
> always called from complete_session() . How would we leak zombies?

Right.  Thanks for putting up with my over-analysis!

Pushed:
http://cgit.freedesktop.org/polkit/commit/?id=7650ad1e08ab13bdb461783c4995d186d9392840