[PATCH] Avoid a pipe read in aio result scheduling

[Jeremy Allison]

On Tue, Aug 23, 2016 at 08:11:41AM +0200, Volker Lendecke wrote:
> On Mon, Aug 22, 2016 at 08:25:03PM -0700, Jeremy Allison wrote:
> > static int pthreadpool_tevent_job_destructor(struct pthreadpool_tevent_job_state *state)
> > {
> >         if (state->pool == NULL) {
> >                 return 0;
> >         }
> > 
> >         /*
> >          * We should never be called with state->req != NULL,
> >          * state->pool must be cleared before the 2nd talloc_free().
> >          */
> >         if (state->req != NULL) {
> >                 abort();
> >         }
> > 
> >         /*
> >          * We need to reparent to a long term context.
> >          */
> >         (void)talloc_reparent(state->req, state->pool, state);
> >         state->req = NULL;
> >         return -1;
> > }
> > 
> > is doing :-(.
> > 
> > I get the 'normal' case where pthreadpool_tevent_job_done()
> > is called, so state->pool = NULL and all is well. Can you
> > explain the failure mode you're explicity guarding against
> > here with (state->req != NULL) ?
> 
> This is metze's piece :-)
> 
> So, the point is that we want to protect against someone doing a
> talloc_free(req) while the job is running. See the comment in
> pthreadpool_tevent_job_done().

OK, pthreadpool_tevent_job_destructor() is the destructor
attached to state, which is owned by the struct tevent_req *req
returned from pthreadpool_tevent_job_send(), so if someone
does a talloc_free(req) it'll get called - makes sense.

1). Can I rename this to pthreadpool_tevent_state_destructor(),
as it's attached to state, not job ?

2). I think that call to talloc_reparent(state->req, state->pool, state);
looks incorrect to me.

The definition of talloc_reparent() is:

_PUBLIC_ void *talloc_reparent(const void *old_parent, const void *new_parent, const void *ptr)

In the pthreadpool_tevent_job_destructor() code we
know that (state->req == NULL) - otherwise we must
have called abort().

In that case we're calling:

(void)talloc_reparent(NULL, state->pool, state);

Internally, talloc_reparent() looks for a pointer matching
old_parent or for the parent of any talloc_reference to
ptr (god how I *hate* talloc_reference() :-) before
doing the reparent. In this case as old_parent is passed
as NULL, I don't think this call will do anything.

As far as I can see this code isn't exercised in the tests
so it's not currently causing a problem.

I think it should be replaced with:

(void)talloc_steal(state->pool, state);

in order to do what the comment says.

3) If someone calls talloc_free(req) on the req
returned from pthreadpool_tevent_job_send(), won't
it always run into the abort() here ?

struct pthreadpool_tevent_job_state *state isn't exposed
externally to anywhere but the code inside pthreadpool_tevent.c,
so I can't see how pthreadpool_tevent_state_destructor can
exer get called with state->req == NULL ?

So if the code:

        /*
         * We should never be called with state->req != NULL,
         * state->pool must be cleared before the 2nd talloc_free().
         */
        if (state->req != NULL) {
                abort();
        }

were changed to be:

        /*
         * We should never be called with state->req == NULL,
         * state->pool must be cleared before the 2nd talloc_free().
         */
        if (state->req == NULL) {
                abort();
        }


it would fix problem #3, and implicitly make the code I pointed
out in problem #2 already correct.

Yeah, I think that's right. With that change, if someone
calls talloc_free(req) whilst req is in flight, then the
pthreadpool_tevent_job_destructor() reparents state to
state->pool, and clears state->req to cope with the case
if someone called talloc_free(req) again before pthreadpool_tevent_job_done()
is called. And when pthreadpool_tevent_job_done() finally
gets called, state->pool gets cleared, state->req is already
equal to NULL and and we go into the case:

        if (state->req == NULL) {
                /*
                 * There was a talloc_free() state->req
                 * while the job was pending,
                 * which mean we're reparented on a longterm
                 * talloc context.
                 *
                 * We just cleanup here...
                 */
                talloc_free(state);
                return;
        }

Sorry for the long message, but I wanted to explain my
thought processes here (it's complicated, as you can see :-).

So if you agree that the code in pthreadpool_tevent_job_destructor()
should be:

        /*
         * We should never be called with state->req == NULL,
         * state->pool must be cleared before the 2nd talloc_free().
         */
        if (state->req == NULL) {
                abort();
        }

then I'm happy to push with my 'Reviewed-by: Jeremy Allison <jra at samba.org>'.

Let me know if I'm wrong.

Cheers,

	Jeremy.