Message ID | 20220128165535.2550899-1-peter.maydell@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series | block/curl.c: Check error return from curl_easy_setopt() | expand |
On Fri, Jan 28, 2022 at 04:55:35PM +0000, Peter Maydell wrote: > Coverity points out that we aren't checking the return value > from curl_easy_setopt() for any of the calls to it we make > in block/curl.c. > > Fixes: Coverity CID 1459336, 1459482, 1460331 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Big fat disclaimer: tested only with 'make check', which I suspect > may not be exercising this block backend. Hints on how to test > more thoroughly are welcome. yeah afaik, qemu-iotests don't have support for this. As a very basic smoke test do $ qemu-img info https://cloud.debian.org/images/cloud/buster/daily/latest/debian-10-nocloud-amd64-daily.qcow2 image: https://cloud.debian.org/images/cloud/buster/daily/latest/debian-10-nocloud-amd64-daily.qcow2 file format: qcow2 virtual size: 2 GiB (2147483648 bytes) disk size: unavailable cluster_size: 65536 Format specific information: compat: 1.1 compression type: zlib lazy refcounts: false refcount bits: 16 corrupt: false extended l2: false and/or $ qemu-img info --image-opts driver=qcow2,file.driver=https,file.url=https://cloud.debian.org/images/cloud/buster/daily/latest/debian-10-nocloud-amd64-daily.qcow2 image: https://cloud.debian.org/images/cloud/buster/daily/latest/debian-10-nocloud-amd64-daily.qcow2 file format: qcow2 virtual size: 2 GiB (2147483648 bytes) disk size: unavailable cluster_size: 65536 Format specific information: compat: 1.1 compression type: zlib lazy refcounts: false refcount bits: 16 corrupt: false extended l2: false > > block/curl.c | 90 +++++++++++++++++++++++++++++++++------------------- > 1 file changed, 58 insertions(+), 32 deletions(-) > > diff --git a/block/curl.c b/block/curl.c > index 6a6cd729758..aaee1b17bef 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -458,38 +458,51 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state) > if (!state->curl) { > return -EIO; > } > - curl_easy_setopt(state->curl, CURLOPT_URL, s->url); > - curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER, > - (long) s->sslverify); > - curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYHOST, > - s->sslverify ? 2L : 0L); > - if (s->cookie) { > - curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie); > + if (curl_easy_setopt(state->curl, CURLOPT_URL, s->url) || > + curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER, > + (long) s->sslverify) || > + curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYHOST, > + s->sslverify ? 2L : 0L)) { > + goto err; > + } > + if (s->cookie) { > + if (curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie)) { > + goto err; > + } > + } > + if (curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout) || > + curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, > + (void *)curl_read_cb) || > + curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state) || > + curl_easy_setopt(state->curl, CURLOPT_PRIVATE, (void *)state) || > + curl_easy_setopt(state->curl, CURLOPT_AUTOREFERER, 1) || > + curl_easy_setopt(state->curl, CURLOPT_FOLLOWLOCATION, 1) || > + curl_easy_setopt(state->curl, CURLOPT_NOSIGNAL, 1) || > + curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg) || > + curl_easy_setopt(state->curl, CURLOPT_FAILONERROR, 1)) { > + goto err; > } > - curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout); > - curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, > - (void *)curl_read_cb); > - curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state); > - curl_easy_setopt(state->curl, CURLOPT_PRIVATE, (void *)state); > - curl_easy_setopt(state->curl, CURLOPT_AUTOREFERER, 1); > - curl_easy_setopt(state->curl, CURLOPT_FOLLOWLOCATION, 1); > - curl_easy_setopt(state->curl, CURLOPT_NOSIGNAL, 1); > - curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg); > - curl_easy_setopt(state->curl, CURLOPT_FAILONERROR, 1); > - > if (s->username) { > - curl_easy_setopt(state->curl, CURLOPT_USERNAME, s->username); > + if (curl_easy_setopt(state->curl, CURLOPT_USERNAME, s->username)) { > + goto err; > + } > } > if (s->password) { > - curl_easy_setopt(state->curl, CURLOPT_PASSWORD, s->password); > + if (curl_easy_setopt(state->curl, CURLOPT_PASSWORD, s->password)) { > + goto err; > + } > } > if (s->proxyusername) { > - curl_easy_setopt(state->curl, > - CURLOPT_PROXYUSERNAME, s->proxyusername); > + if (curl_easy_setopt(state->curl, > + CURLOPT_PROXYUSERNAME, s->proxyusername)) { > + goto err; > + } > } > if (s->proxypassword) { > - curl_easy_setopt(state->curl, > - CURLOPT_PROXYPASSWORD, s->proxypassword); > + if (curl_easy_setopt(state->curl, > + CURLOPT_PROXYPASSWORD, s->proxypassword)) { > + goto err; > + } > } > > /* Restrict supported protocols to avoid security issues in the more > @@ -499,18 +512,27 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state) > * Restricting protocols is only supported from 7.19.4 upwards. > */ > #if LIBCURL_VERSION_NUM >= 0x071304 > - curl_easy_setopt(state->curl, CURLOPT_PROTOCOLS, PROTOCOLS); > - curl_easy_setopt(state->curl, CURLOPT_REDIR_PROTOCOLS, PROTOCOLS); > + if (curl_easy_setopt(state->curl, CURLOPT_PROTOCOLS, PROTOCOLS) || > + curl_easy_setopt(state->curl, CURLOPT_REDIR_PROTOCOLS, PROTOCOLS)) { > + goto err; > + } > #endif > > #ifdef DEBUG_VERBOSE > - curl_easy_setopt(state->curl, CURLOPT_VERBOSE, 1); > + if (curl_easy_setopt(state->curl, CURLOPT_VERBOSE, 1)) { > + goto err; > + } > #endif > } > > state->s = s; > > return 0; > + > +err: > + curl_easy_cleanup(state->curl); > + state->curl = NULL; > + return -EIO; > } > > /* Called with s->mutex held. */ > @@ -763,10 +785,11 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, > } > > s->accept_range = false; > - curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1); > - curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION, > - curl_header_cb); > - curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s); > + if (curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1) || > + curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION, curl_header_cb) || > + curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s)) { > + goto out; > + } > if (curl_easy_perform(state->curl)) > goto out; > if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)) { > @@ -879,7 +902,10 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb) > > snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end); > trace_curl_setup_preadv(acb->bytes, start, state->range); > - curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range); > + if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range)) { > + curl_clean_state(state); > + goto out; > + } > > if (curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) { > state->acb[0] = NULL; > -- > 2.25.1 > > Regards, Daniel
On Fri, 28 Jan 2022 at 17:50, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Fri, Jan 28, 2022 at 04:55:35PM +0000, Peter Maydell wrote: > > Coverity points out that we aren't checking the return value > > from curl_easy_setopt() for any of the calls to it we make > > in block/curl.c. > > > > Fixes: Coverity CID 1459336, 1459482, 1460331 > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > Big fat disclaimer: tested only with 'make check', which I suspect > > may not be exercising this block backend. Hints on how to test > > more thoroughly are welcome. > > yeah afaik, qemu-iotests don't have support for this. As a very > basic smoke test do > [command lines snipped] Thanks; yes, those both work. -- PMM
On 28.01.22 17:55, Peter Maydell wrote: > Coverity points out that we aren't checking the return value > from curl_easy_setopt() for any of the calls to it we make > in block/curl.c. > > Fixes: Coverity CID 1459336, 1459482, 1460331 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Big fat disclaimer: tested only with 'make check', which I suspect > may not be exercising this block backend. Hints on how to test > more thoroughly are welcome. > > block/curl.c | 90 +++++++++++++++++++++++++++++++++------------------- > 1 file changed, 58 insertions(+), 32 deletions(-) One problem I see in general is that most of the setopt functions are (indirectly) called from `curl_open()`, which is supposed to return an error message. Its `out` label seems to expect some error description in `state->errmsg`. The error handling here doesn’t set such a description. Then again, there are enough existing error paths that don’t set this description either, so it isn’t quite this patch’s duty to fix that situation. I guess it would be nice if we had a wrapper for `curl_easy_setopt()` with an `Error **` parameter, so we could easily generate error messages that describe key and value (and then `curl_init_state()` should have an `Error **` parameter, too). But this patch doesn’t make anything worse than it already is, so that’d rather be an idea for future clean-up. > diff --git a/block/curl.c b/block/curl.c > index 6a6cd729758..aaee1b17bef 100644 > --- a/block/curl.c > +++ b/block/curl.c [...] > @@ -879,7 +902,10 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb) > > snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end); > trace_curl_setup_preadv(acb->bytes, start, state->range); > - curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range); > + if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range)) { > + curl_clean_state(state); > + goto out; I think we need to mark the request as failed by setting `acb->ret` to a negative value (and probably also clear `state->acb[0]` like the error path below does). Hanna > + } > > if (curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) { > state->acb[0] = NULL;
On Tue, 1 Feb 2022 at 11:25, Hanna Reitz <hreitz@redhat.com> wrote: > > On 28.01.22 17:55, Peter Maydell wrote: > > Coverity points out that we aren't checking the return value > > from curl_easy_setopt() for any of the calls to it we make > > in block/curl.c. > > > > Fixes: Coverity CID 1459336, 1459482, 1460331 > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > Big fat disclaimer: tested only with 'make check', which I suspect > > may not be exercising this block backend. Hints on how to test > > more thoroughly are welcome. > > > > block/curl.c | 90 +++++++++++++++++++++++++++++++++------------------- > > 1 file changed, 58 insertions(+), 32 deletions(-) > > One problem I see in general is that most of the setopt functions are > (indirectly) called from `curl_open()`, which is supposed to return an > error message. Its `out` label seems to expect some error description > in `state->errmsg`. The error handling here doesn’t set such a description. Ah, yes, I hadn't noticed that -- it's a pre-existing bug, where we do this: if (curl_init_state(s, state) < 0) { goto out; } and curl_init_state() already has an error path (for when curl_easy_init() fails) that can take that goto without setting state->errmsg... > Then again, there are enough existing error paths that don’t set this > description either, so it isn’t quite this patch’s duty to fix that > situation. ...as you've already noticed :-) > I guess it would be nice if we had a wrapper for > `curl_easy_setopt()` with an `Error **` parameter, so we could easily > generate error messages that describe key and value (and then > `curl_init_state()` should have an `Error **` parameter, too). > > But this patch doesn’t make anything worse than it already is, so that’d > rather be an idea for future clean-up. > > > diff --git a/block/curl.c b/block/curl.c > > index 6a6cd729758..aaee1b17bef 100644 > > --- a/block/curl.c > > +++ b/block/curl.c > > [...] > > > @@ -879,7 +902,10 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb) > > > > snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end); > > trace_curl_setup_preadv(acb->bytes, start, state->range); > > - curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range); > > + if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range)) { > > + curl_clean_state(state); > > + goto out; > > I think we need to mark the request as failed by setting `acb->ret` to a > negative value (and probably also clear `state->acb[0]` like the error > path below does). OK; or I could roll the two curl calls into the same if: if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range) || curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) { /* existing error handling and goto-out code */ } thanks -- PMM
On 21.02.22 20:45, Peter Maydell wrote: > On Tue, 1 Feb 2022 at 11:25, Hanna Reitz <hreitz@redhat.com> wrote: >> On 28.01.22 17:55, Peter Maydell wrote: >>> Coverity points out that we aren't checking the return value >>> from curl_easy_setopt() for any of the calls to it we make >>> in block/curl.c. >>> >>> Fixes: Coverity CID 1459336, 1459482, 1460331 >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> Big fat disclaimer: tested only with 'make check', which I suspect >>> may not be exercising this block backend. Hints on how to test >>> more thoroughly are welcome. >>> >>> block/curl.c | 90 +++++++++++++++++++++++++++++++++------------------- >>> 1 file changed, 58 insertions(+), 32 deletions(-) >> One problem I see in general is that most of the setopt functions are >> (indirectly) called from `curl_open()`, which is supposed to return an >> error message. Its `out` label seems to expect some error description >> in `state->errmsg`. The error handling here doesn’t set such a description. > Ah, yes, I hadn't noticed that -- it's a pre-existing bug, where > we do this: > > if (curl_init_state(s, state) < 0) { > goto out; > } > > and curl_init_state() already has an error path (for when curl_easy_init() > fails) that can take that goto without setting state->errmsg... > >> Then again, there are enough existing error paths that don’t set this >> description either, so it isn’t quite this patch’s duty to fix that >> situation. > ...as you've already noticed :-) > >> I guess it would be nice if we had a wrapper for >> `curl_easy_setopt()` with an `Error **` parameter, so we could easily >> generate error messages that describe key and value (and then >> `curl_init_state()` should have an `Error **` parameter, too). >> >> But this patch doesn’t make anything worse than it already is, so that’d >> rather be an idea for future clean-up. >> >>> diff --git a/block/curl.c b/block/curl.c >>> index 6a6cd729758..aaee1b17bef 100644 >>> --- a/block/curl.c >>> +++ b/block/curl.c >> [...] >> >>> @@ -879,7 +902,10 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb) >>> >>> snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end); >>> trace_curl_setup_preadv(acb->bytes, start, state->range); >>> - curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range); >>> + if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range)) { >>> + curl_clean_state(state); >>> + goto out; >> I think we need to mark the request as failed by setting `acb->ret` to a >> negative value (and probably also clear `state->acb[0]` like the error >> path below does). > OK; or I could roll the two curl calls into the same if: > > if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range) || > curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) { > /* existing error handling and goto-out code */ > } Sure, that sounds perfectly good to me. Hanna
diff --git a/block/curl.c b/block/curl.c index 6a6cd729758..aaee1b17bef 100644 --- a/block/curl.c +++ b/block/curl.c @@ -458,38 +458,51 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state) if (!state->curl) { return -EIO; } - curl_easy_setopt(state->curl, CURLOPT_URL, s->url); - curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER, - (long) s->sslverify); - curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYHOST, - s->sslverify ? 2L : 0L); - if (s->cookie) { - curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie); + if (curl_easy_setopt(state->curl, CURLOPT_URL, s->url) || + curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER, + (long) s->sslverify) || + curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYHOST, + s->sslverify ? 2L : 0L)) { + goto err; + } + if (s->cookie) { + if (curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie)) { + goto err; + } + } + if (curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout) || + curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, + (void *)curl_read_cb) || + curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state) || + curl_easy_setopt(state->curl, CURLOPT_PRIVATE, (void *)state) || + curl_easy_setopt(state->curl, CURLOPT_AUTOREFERER, 1) || + curl_easy_setopt(state->curl, CURLOPT_FOLLOWLOCATION, 1) || + curl_easy_setopt(state->curl, CURLOPT_NOSIGNAL, 1) || + curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg) || + curl_easy_setopt(state->curl, CURLOPT_FAILONERROR, 1)) { + goto err; } - curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, (long)s->timeout); - curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, - (void *)curl_read_cb); - curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state); - curl_easy_setopt(state->curl, CURLOPT_PRIVATE, (void *)state); - curl_easy_setopt(state->curl, CURLOPT_AUTOREFERER, 1); - curl_easy_setopt(state->curl, CURLOPT_FOLLOWLOCATION, 1); - curl_easy_setopt(state->curl, CURLOPT_NOSIGNAL, 1); - curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg); - curl_easy_setopt(state->curl, CURLOPT_FAILONERROR, 1); - if (s->username) { - curl_easy_setopt(state->curl, CURLOPT_USERNAME, s->username); + if (curl_easy_setopt(state->curl, CURLOPT_USERNAME, s->username)) { + goto err; + } } if (s->password) { - curl_easy_setopt(state->curl, CURLOPT_PASSWORD, s->password); + if (curl_easy_setopt(state->curl, CURLOPT_PASSWORD, s->password)) { + goto err; + } } if (s->proxyusername) { - curl_easy_setopt(state->curl, - CURLOPT_PROXYUSERNAME, s->proxyusername); + if (curl_easy_setopt(state->curl, + CURLOPT_PROXYUSERNAME, s->proxyusername)) { + goto err; + } } if (s->proxypassword) { - curl_easy_setopt(state->curl, - CURLOPT_PROXYPASSWORD, s->proxypassword); + if (curl_easy_setopt(state->curl, + CURLOPT_PROXYPASSWORD, s->proxypassword)) { + goto err; + } } /* Restrict supported protocols to avoid security issues in the more @@ -499,18 +512,27 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state) * Restricting protocols is only supported from 7.19.4 upwards. */ #if LIBCURL_VERSION_NUM >= 0x071304 - curl_easy_setopt(state->curl, CURLOPT_PROTOCOLS, PROTOCOLS); - curl_easy_setopt(state->curl, CURLOPT_REDIR_PROTOCOLS, PROTOCOLS); + if (curl_easy_setopt(state->curl, CURLOPT_PROTOCOLS, PROTOCOLS) || + curl_easy_setopt(state->curl, CURLOPT_REDIR_PROTOCOLS, PROTOCOLS)) { + goto err; + } #endif #ifdef DEBUG_VERBOSE - curl_easy_setopt(state->curl, CURLOPT_VERBOSE, 1); + if (curl_easy_setopt(state->curl, CURLOPT_VERBOSE, 1)) { + goto err; + } #endif } state->s = s; return 0; + +err: + curl_easy_cleanup(state->curl); + state->curl = NULL; + return -EIO; } /* Called with s->mutex held. */ @@ -763,10 +785,11 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, } s->accept_range = false; - curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1); - curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION, - curl_header_cb); - curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s); + if (curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1) || + curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION, curl_header_cb) || + curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s)) { + goto out; + } if (curl_easy_perform(state->curl)) goto out; if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)) { @@ -879,7 +902,10 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb) snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end); trace_curl_setup_preadv(acb->bytes, start, state->range); - curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range); + if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range)) { + curl_clean_state(state); + goto out; + } if (curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) { state->acb[0] = NULL;
Coverity points out that we aren't checking the return value from curl_easy_setopt() for any of the calls to it we make in block/curl.c. Fixes: Coverity CID 1459336, 1459482, 1460331 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Big fat disclaimer: tested only with 'make check', which I suspect may not be exercising this block backend. Hints on how to test more thoroughly are welcome. block/curl.c | 90 +++++++++++++++++++++++++++++++++------------------- 1 file changed, 58 insertions(+), 32 deletions(-)