Re: [PATCH] bug in pollset_wakeup() + nocopy
On Mon, Mar 29, 2010 at 5:53 PM, Neil Conway n...@cs.berkeley.edu wrote: On Mon, Mar 29, 2010 at 2:14 PM, Nick Kew n...@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
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 n...@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 n...@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 n...@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 n...@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
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
Re: [PATCH] bug in pollset_wakeup() + nocopy
On Mon, Mar 29, 2010 at 2:14 PM, Nick Kew n...@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 apr_pollset_wakeup_nocopy_14backport.patch Description: Binary data apr_pollset_wakeup_nocopy_trunk.patch Description: Binary data
Re: [PATCH] bug in pollset_wakeup() + nocopy
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 n...@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 n...@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 n...@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
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 n...@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 n...@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 apr_pollset_wakeup_nocopy-3.patch Description: Binary data
Re: [PATCH] bug in pollset_wakeup() + nocopy
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 n...@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 apr_pollset_wakeup_nocopy-2.patch Description: Binary data
