Re: [PATCH] bug in pollset_wakeup() + nocopy

[Neil Conway <nr...@cs.berkeley.edu>]

Ping? This patch addresses a crash that exists in both 1.4.x and
trunk, and is quite straightforward.

Neil

On Thu, Feb 4, 2010 at 8:14 PM, Neil Conway <nr...@cs.berkeley.edu> wrote:
> Any feedback on this patch? The bug it addresses exists in both 1.4.x and trunk.
>
> Neil
>
> On Sat, Jan 16, 2010 at 5:30 PM, Neil Conway <nr...@cs.berkeley.edu> wrote:
>> Attached is a refreshed version of this patch that applies against
>> current APR trunk (after the recent pollcb_wakeup() changes). The
>> patch is now pretty trivial.
>>
>> Note that if you want to backport this bug fix to the 1.4 branch, the
>> previous version of the patch should be used. But perhaps the easiest
>> route is to first backport the pollcb_wakeup() change, and then apply
>> this version of the patch.
>>
>> Neil
>>
>> On Wed, Jan 6, 2010 at 9:06 PM, Neil Conway <nr...@cs.berkeley.edu> wrote:
>>> Attached is a slightly revised version of this patch. Changes:
>>>
>>> * Initialize the apr_pool_t field of the apr_pollfd_t we use for the
>>> wakeup pipe. Not clear what this field is actually used for (candidate
>>> for removal in APR2?), but we may as well be tidy.
>>>
>>> * Fix a minor bug in one of the versions of close_wakeup_pipe():
>>> initialize both "rv0" and "rv1", to avoid potentially reading an
>>> uninitialized value.
>>>
>>> Neil
>>>
>>> On Wed, Jan 6, 2010 at 5:47 PM, Neil Conway <nr...@cs.berkeley.edu> wrote:
>>>> apr_pollset_wakeup() is buggy when used with APR_POLLSET_NOCOPY,
>>>> because create_wakeup_pipe() passes a stack-allocated apr_pollfd_t to
>>>> apr_pollset_add(). This is unsafe if the user specified
>>>> APR_POLLSET_NOCOPY when creating the pollset.
>>>>
>>>> The attached patch fixes this by adding an apr_pollfd_t for the wakeup
>>>> pipe to apr_pollset_t, so that it has a sufficiently-long-lived
>>>> lifetime.
>>>>
>>>> Neil
>>>>
>>>
>>
>

Re: [PATCH] bug in pollset_wakeup() + nocopy

[Neil Conway <nr...@cs.berkeley.edu>]

Hey Jeff,

Thanks for applying the patch!

On Fri, Apr 9, 2010 at 2:01 PM, Jeff Trawick <tr...@gmail.com> wrote:
> Neil, you mentioned earlier in the thread:
>
>>Note that if you want to backport this bug fix to the 1.4 branch, the
>>previous version of the patch should be used. But perhaps the easiest
>>route is to first backport the pollcb_wakeup() change, and then apply
>>this version of the patch.
>
> As I understand it, the pollcb changes you refer to are for a new API,
> which can't be added to 1.4.x at this point; is that  "new API"
> understanding correct?  Anyway, there's no reason it can't go to
> 1.5.x; care to post a patch to get 1.5.x caught up?

Right; my original statement was prior to 1.4.x branching.  I'll look
into backporting the pollcb_wakeup() API to 1.5.x -- although I'm
about to travel for a week, so I might not get to it for a little
while.

Thanks,

Neil

Re: [PATCH] bug in pollset_wakeup() + nocopy

[Jeff Trawick <tr...@gmail.com>]

On Mon, Mar 29, 2010 at 5:53 PM, Neil Conway <nr...@cs.berkeley.edu> wrote:
> On Mon, Mar 29, 2010 at 2:14 PM, Nick Kew <ni...@apache.org> wrote:
>> I don't see the patch in this post.  Is it small/simple/clear enough to
>> review in a brief-ish session?
>
> Yep, should be very straightforward. Attached are two versions of the
> patch (one for the 1.4.x branch, one for trunk). The reasoning for the
> fix is simple (APR_POLLSET_NOCOPY == don't pass a stack-allocated
> pollfd_t to pollset_add()), and discussed in earlier emails:
>
> http://markmail.org/message/izj3zpc65sckzuao

finally committed to 1.4.x-trunk

Neil, you mentioned earlier in the thread:

>Note that if you want to backport this bug fix to the 1.4 branch, the
>previous version of the patch should be used. But perhaps the easiest
>route is to first backport the pollcb_wakeup() change, and then apply
>this version of the patch.

As I understand it, the pollcb changes you refer to are for a new API,
which can't be added to 1.4.x at this point; is that  "new API"
understanding correct?  Anyway, there's no reason it can't go to
1.5.x; care to post a patch to get 1.5.x caught up?

Re: [PATCH] bug in pollset_wakeup() + nocopy

[Neil Conway <nr...@cs.berkeley.edu>]

On Mon, Mar 29, 2010 at 2:14 PM, Nick Kew <ni...@apache.org> wrote:
> I don't see the patch in this post.  Is it small/simple/clear enough to
> review in a brief-ish session?

Yep, should be very straightforward. Attached are two versions of the
patch (one for the 1.4.x branch, one for trunk). The reasoning for the
fix is simple (APR_POLLSET_NOCOPY == don't pass a stack-allocated
pollfd_t to pollset_add()), and discussed in earlier emails:

http://markmail.org/message/izj3zpc65sckzuao

Thanks,

Neil

Re: [PATCH] bug in pollset_wakeup() + nocopy

[Nick Kew <ni...@apache.org>]

On 29 Mar 2010, at 18:19, Neil Conway wrote:

> Ping? This patch addresses a crash that exists in both 1.4.x and
> trunk, and is quite straightforward.

I don't see the patch in this post.  Is it small/simple/clear enough to
review in a brief-ish session?  A bugzilla entry for it would be something
you could usefully reference in bringing it to our attention.

-- 
Nick Kew