* [Qemu-devel] [PATCH v2] curl: Don't deref NULL pointer in call to aio_poll. @ 2014-08-28 8:04 Richard W.M. Jones 2014-08-28 8:04 ` Richard W.M. Jones 2014-08-29 15:19 ` Stefan Hajnoczi 0 siblings, 2 replies; 7+ messages in thread From: Richard W.M. Jones @ 2014-08-28 8:04 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, famz, stefanha No change in the patch itself since v1. However I have added a longer explanation to the commit message. Rich. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v2] curl: Don't deref NULL pointer in call to aio_poll. 2014-08-28 8:04 [Qemu-devel] [PATCH v2] curl: Don't deref NULL pointer in call to aio_poll Richard W.M. Jones @ 2014-08-28 8:04 ` Richard W.M. Jones 2014-08-28 14:54 ` Benoît Canet 2014-08-29 9:03 ` Stefan Hajnoczi 2014-08-29 15:19 ` Stefan Hajnoczi 1 sibling, 2 replies; 7+ messages in thread From: Richard W.M. Jones @ 2014-08-28 8:04 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, famz, stefanha In commit 63f0f45f2e89b60ff8245fec81328ddfde42a303 the following mechanical change was made: if (!state) { - qemu_aio_wait(); + aio_poll(state->s->aio_context, true); } The new code now checks if state is NULL and then dereferences it ('state->s') which is obviously incorrect. This commit replaces state->s->aio_context with bdrv_get_aio_context(bs), fixing this problem. The two other hunks are concerned with getting the BlockDriverState pointer bs to where it is needed. The original bug causes a segfault when using libguestfs to access a VMware vCenter Server and doing any kind of complex read-heavy operations. With this commit the segfault goes away. Signed-off-by: Richard W.M. Jones <rjones@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> --- block/curl.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/block/curl.c b/block/curl.c index d4b85d2..f59615d 100644 --- a/block/curl.c +++ b/block/curl.c @@ -352,7 +352,7 @@ static void curl_multi_timeout_do(void *arg) #endif } -static CURLState *curl_init_state(BDRVCURLState *s) +static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s) { CURLState *state = NULL; int i, j; @@ -370,7 +370,7 @@ static CURLState *curl_init_state(BDRVCURLState *s) break; } if (!state) { - aio_poll(state->s->aio_context, true); + aio_poll(bdrv_get_aio_context(bs), true); } } while(!state); @@ -541,7 +541,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, DPRINTF("CURL: Opening %s\n", file); s->aio_context = bdrv_get_aio_context(bs); s->url = g_strdup(file); - state = curl_init_state(s); + state = curl_init_state(bs, s); if (!state) goto out_noclean; @@ -625,7 +625,7 @@ static void curl_readv_bh_cb(void *p) } // No cache found, so let's start a new request - state = curl_init_state(s); + state = curl_init_state(acb->common.bs, s); if (!state) { acb->common.cb(acb->common.opaque, -EIO); qemu_aio_release(acb); -- 2.0.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] curl: Don't deref NULL pointer in call to aio_poll. 2014-08-28 8:04 ` Richard W.M. Jones @ 2014-08-28 14:54 ` Benoît Canet 2014-08-29 9:03 ` Stefan Hajnoczi 1 sibling, 0 replies; 7+ messages in thread From: Benoît Canet @ 2014-08-28 14:54 UTC (permalink / raw) To: Richard W.M. Jones; +Cc: pbonzini, famz, qemu-devel, stefanha The Thursday 28 Aug 2014 à 09:04:21 (+0100), Richard W.M. Jones wrote : > In commit 63f0f45f2e89b60ff8245fec81328ddfde42a303 the following > mechanical change was made: > > if (!state) { > - qemu_aio_wait(); > + aio_poll(state->s->aio_context, true); > } > > The new code now checks if state is NULL and then dereferences it > ('state->s') which is obviously incorrect. > > This commit replaces state->s->aio_context with > bdrv_get_aio_context(bs), fixing this problem. The two other hunks > are concerned with getting the BlockDriverState pointer bs to where it > is needed. > > The original bug causes a segfault when using libguestfs to access a > VMware vCenter Server and doing any kind of complex read-heavy > operations. With this commit the segfault goes away. > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/curl.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/block/curl.c b/block/curl.c > index d4b85d2..f59615d 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -352,7 +352,7 @@ static void curl_multi_timeout_do(void *arg) > #endif > } > > -static CURLState *curl_init_state(BDRVCURLState *s) > +static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s) > { > CURLState *state = NULL; > int i, j; > @@ -370,7 +370,7 @@ static CURLState *curl_init_state(BDRVCURLState *s) > break; > } > if (!state) { > - aio_poll(state->s->aio_context, true); > + aio_poll(bdrv_get_aio_context(bs), true); > } > } while(!state); > > @@ -541,7 +541,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, > DPRINTF("CURL: Opening %s\n", file); > s->aio_context = bdrv_get_aio_context(bs); > s->url = g_strdup(file); > - state = curl_init_state(s); > + state = curl_init_state(bs, s); > if (!state) > goto out_noclean; > > @@ -625,7 +625,7 @@ static void curl_readv_bh_cb(void *p) > } > > // No cache found, so let's start a new request > - state = curl_init_state(s); > + state = curl_init_state(acb->common.bs, s); > if (!state) { > acb->common.cb(acb->common.opaque, -EIO); > qemu_aio_release(acb); > -- > 2.0.4 > > Reviewed-by: Benoît Canet <benoit.canet@nodalink.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] curl: Don't deref NULL pointer in call to aio_poll. 2014-08-28 8:04 ` Richard W.M. Jones 2014-08-28 14:54 ` Benoît Canet @ 2014-08-29 9:03 ` Stefan Hajnoczi 2014-08-29 9:19 ` Richard W.M. Jones 1 sibling, 1 reply; 7+ messages in thread From: Stefan Hajnoczi @ 2014-08-29 9:03 UTC (permalink / raw) To: Richard W.M. Jones; +Cc: pbonzini, famz, qemu-devel [-- Attachment #1: Type: text/plain, Size: 837 bytes --] On Thu, Aug 28, 2014 at 09:04:21AM +0100, Richard W.M. Jones wrote: > diff --git a/block/curl.c b/block/curl.c > index d4b85d2..f59615d 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -352,7 +352,7 @@ static void curl_multi_timeout_do(void *arg) > #endif > } > > -static CURLState *curl_init_state(BDRVCURLState *s) > +static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s) > { > CURLState *state = NULL; > int i, j; Why add the BDRVCURLState *s argument... > @@ -370,7 +370,7 @@ static CURLState *curl_init_state(BDRVCURLState *s) > break; > } > if (!state) { > - aio_poll(state->s->aio_context, true); > + aio_poll(bdrv_get_aio_context(bs), true); > } > } while(!state); > ...if it is not used? [-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] curl: Don't deref NULL pointer in call to aio_poll. 2014-08-29 9:03 ` Stefan Hajnoczi @ 2014-08-29 9:19 ` Richard W.M. Jones 2014-08-29 15:13 ` Stefan Hajnoczi 0 siblings, 1 reply; 7+ messages in thread From: Richard W.M. Jones @ 2014-08-29 9:19 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: pbonzini, famz, qemu-devel On Fri, Aug 29, 2014 at 10:03:59AM +0100, Stefan Hajnoczi wrote: > On Thu, Aug 28, 2014 at 09:04:21AM +0100, Richard W.M. Jones wrote: > > diff --git a/block/curl.c b/block/curl.c > > index d4b85d2..f59615d 100644 > > --- a/block/curl.c > > +++ b/block/curl.c > > @@ -352,7 +352,7 @@ static void curl_multi_timeout_do(void *arg) > > #endif > > } > > > > -static CURLState *curl_init_state(BDRVCURLState *s) > > +static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s) > > { > > CURLState *state = NULL; > > int i, j; > > Why add the BDRVCURLState *s argument... > > > @@ -370,7 +370,7 @@ static CURLState *curl_init_state(BDRVCURLState *s) > > break; > > } > > if (!state) { > > - aio_poll(state->s->aio_context, true); > > + aio_poll(bdrv_get_aio_context(bs), true); > > } > > } while(!state); > > > > ...if it is not used? I may be misunderstanding this, but BDRVCURLState *s is used elsewhere in the function. However there is a potential to derive `BDRVCURLState *s = bs->opaque;' if that's what you meant? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] curl: Don't deref NULL pointer in call to aio_poll. 2014-08-29 9:19 ` Richard W.M. Jones @ 2014-08-29 15:13 ` Stefan Hajnoczi 0 siblings, 0 replies; 7+ messages in thread From: Stefan Hajnoczi @ 2014-08-29 15:13 UTC (permalink / raw) To: Richard W.M. Jones; +Cc: pbonzini, famz, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1423 bytes --] On Fri, Aug 29, 2014 at 10:19:04AM +0100, Richard W.M. Jones wrote: > On Fri, Aug 29, 2014 at 10:03:59AM +0100, Stefan Hajnoczi wrote: > > On Thu, Aug 28, 2014 at 09:04:21AM +0100, Richard W.M. Jones wrote: > > > diff --git a/block/curl.c b/block/curl.c > > > index d4b85d2..f59615d 100644 > > > --- a/block/curl.c > > > +++ b/block/curl.c > > > @@ -352,7 +352,7 @@ static void curl_multi_timeout_do(void *arg) > > > #endif > > > } > > > > > > -static CURLState *curl_init_state(BDRVCURLState *s) > > > +static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s) > > > { > > > CURLState *state = NULL; > > > int i, j; > > > > Why add the BDRVCURLState *s argument... > > > > > @@ -370,7 +370,7 @@ static CURLState *curl_init_state(BDRVCURLState *s) > > > break; > > > } > > > if (!state) { > > > - aio_poll(state->s->aio_context, true); > > > + aio_poll(bdrv_get_aio_context(bs), true); > > > } > > > } while(!state); > > > > > > > ...if it is not used? > > I may be misunderstanding this, but BDRVCURLState *s is used elsewhere > in the function. > > However there is a potential to derive `BDRVCURLState *s = bs->opaque;' > if that's what you meant? Sorry, I misread the code. I thought you added s but you really added bs. That's fine and it makes sense now :). Stefan [-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] curl: Don't deref NULL pointer in call to aio_poll. 2014-08-28 8:04 [Qemu-devel] [PATCH v2] curl: Don't deref NULL pointer in call to aio_poll Richard W.M. Jones 2014-08-28 8:04 ` Richard W.M. Jones @ 2014-08-29 15:19 ` Stefan Hajnoczi 1 sibling, 0 replies; 7+ messages in thread From: Stefan Hajnoczi @ 2014-08-29 15:19 UTC (permalink / raw) To: Richard W.M. Jones; +Cc: pbonzini, famz, qemu-devel [-- Attachment #1: Type: text/plain, Size: 293 bytes --] On Thu, Aug 28, 2014 at 09:04:20AM +0100, Richard W.M. Jones wrote: > No change in the patch itself since v1. However I have added a longer > explanation to the commit message. > > Rich. > Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan [-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-08-29 15:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-28 8:04 [Qemu-devel] [PATCH v2] curl: Don't deref NULL pointer in call to aio_poll Richard W.M. Jones 2014-08-28 8:04 ` Richard W.M. Jones 2014-08-28 14:54 ` Benoît Canet 2014-08-29 9:03 ` Stefan Hajnoczi 2014-08-29 9:19 ` Richard W.M. Jones 2014-08-29 15:13 ` Stefan Hajnoczi 2014-08-29 15:19 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).