* [PATCH v2 0/2] block/curl: check error return from curl_easy_setopt()
@ 2022-02-22 15:23 Peter Maydell
2022-02-22 15:23 ` [PATCH v2 1/2] block/curl.c: Set error message string if curl_init_state() fails Peter Maydell
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Peter Maydell @ 2022-02-22 15:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Hanna Reitz, qemu-block
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.
Tested with 'make check' and with some basic smoke test command lines
suggested by Dan:
qemu-img info https://cloud.debian.org/images/cloud/buster/daily/latest/debian-10-nocloud-amd64-daily.qcow2
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
Changes v1->v2:
* new patch 1 which fixes a missing "set the error string" for
when curl_init_state() returns failure, since we're about to
add more cases when that function can fail
* set the error string in the failure path for the direct setopt
calls in curl_open()
* fix the failure path in curl_setup_preadv() by putting
the curl_easy_setopt() call in the same if() condition
as the existing curl_multi_add_handle()
thanks
-- PMM
Peter Maydell (2):
block/curl.c: Set error message string if curl_init_state() fails
block/curl.c: Check error return from curl_easy_setopt()
block/curl.c | 94 +++++++++++++++++++++++++++++++++-------------------
1 file changed, 60 insertions(+), 34 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] block/curl.c: Set error message string if curl_init_state() fails
2022-02-22 15:23 [PATCH v2 0/2] block/curl: check error return from curl_easy_setopt() Peter Maydell
@ 2022-02-22 15:23 ` Peter Maydell
2022-02-22 21:30 ` Philippe Mathieu-Daudé
2022-02-24 14:08 ` Hanna Reitz
2022-02-22 15:23 ` [PATCH v2 2/2] block/curl.c: Check error return from curl_easy_setopt() Peter Maydell
2022-02-24 14:13 ` [PATCH v2 0/2] block/curl: check " Hanna Reitz
2 siblings, 2 replies; 8+ messages in thread
From: Peter Maydell @ 2022-02-22 15:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Hanna Reitz, qemu-block
In curl_open(), the 'out' label assumes that the state->errmsg string
has been set (either by curl_easy_perform() or by manually copying a
string into it); however if curl_init_state() fails we will jump to
that label without setting the string. Add the missing error string
setup.
(We can't be specific about the cause of failure: the documentation
of curl_easy_init() just says "If this function returns NULL,
something went wrong".)
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
block/curl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/curl.c b/block/curl.c
index 6a6cd729758..95168529715 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -759,6 +759,8 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
// Get file size
if (curl_init_state(s, state) < 0) {
+ pstrcpy(state->errmsg, CURL_ERROR_SIZE,
+ "curl library initialization failed.");
goto out;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] block/curl.c: Check error return from curl_easy_setopt()
2022-02-22 15:23 [PATCH v2 0/2] block/curl: check error return from curl_easy_setopt() Peter Maydell
2022-02-22 15:23 ` [PATCH v2 1/2] block/curl.c: Set error message string if curl_init_state() fails Peter Maydell
@ 2022-02-22 15:23 ` Peter Maydell
2022-02-24 14:11 ` Hanna Reitz
2022-02-24 14:13 ` [PATCH v2 0/2] block/curl: check " Hanna Reitz
2 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2022-02-22 15:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Hanna Reitz, qemu-block
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.
Some of these options are documented as always succeeding (e.g.
CURLOPT_VERBOSE) but others have documented failure cases (e.g.
CURLOPT_URL). For consistency we check every call, even the ones
that theoretically cannot fail.
Fixes: Coverity CID 1459336, 1459482, 1460331
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Changes v1->v2:
* set the error string in the failure path for the
direct setopt calls in curl_open()
* fix the failure path in curl_setup_preadv() by putting
the curl_easy_setopt() call in the same if() condition
as the existing curl_multi_add_handle()
---
block/curl.c | 92 +++++++++++++++++++++++++++++++++-------------------
1 file changed, 58 insertions(+), 34 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index 95168529715..1e0f6095797 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. */
@@ -765,10 +787,13 @@ 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)) {
+ pstrcpy(state->errmsg, CURL_ERROR_SIZE,
+ "curl library initialization failed.");
+ goto out;
+ }
if (curl_easy_perform(state->curl))
goto out;
if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)) {
@@ -881,9 +906,8 @@ 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_multi_add_handle(s->multi, state->curl) != CURLM_OK) {
+ if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range) ||
+ curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) {
state->acb[0] = NULL;
acb->ret = -EIO;
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] block/curl.c: Set error message string if curl_init_state() fails
2022-02-22 15:23 ` [PATCH v2 1/2] block/curl.c: Set error message string if curl_init_state() fails Peter Maydell
@ 2022-02-22 21:30 ` Philippe Mathieu-Daudé
2022-02-24 14:08 ` Hanna Reitz
1 sibling, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-02-22 21:30 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: Kevin Wolf, Hanna Reitz, qemu-block
On 22/2/22 16:23, Peter Maydell wrote:
> In curl_open(), the 'out' label assumes that the state->errmsg string
> has been set (either by curl_easy_perform() or by manually copying a
> string into it); however if curl_init_state() fails we will jump to
> that label without setting the string. Add the missing error string
> setup.
>
> (We can't be specific about the cause of failure: the documentation
> of curl_easy_init() just says "If this function returns NULL,
> something went wrong".)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> block/curl.c | 2 ++
> 1 file changed, 2 insertions(+)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] block/curl.c: Set error message string if curl_init_state() fails
2022-02-22 15:23 ` [PATCH v2 1/2] block/curl.c: Set error message string if curl_init_state() fails Peter Maydell
2022-02-22 21:30 ` Philippe Mathieu-Daudé
@ 2022-02-24 14:08 ` Hanna Reitz
1 sibling, 0 replies; 8+ messages in thread
From: Hanna Reitz @ 2022-02-24 14:08 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: Kevin Wolf, qemu-block
On 22.02.22 16:23, Peter Maydell wrote:
> In curl_open(), the 'out' label assumes that the state->errmsg string
> has been set (either by curl_easy_perform() or by manually copying a
> string into it); however if curl_init_state() fails we will jump to
> that label without setting the string. Add the missing error string
> setup.
>
> (We can't be specific about the cause of failure: the documentation
> of curl_easy_init() just says "If this function returns NULL,
> something went wrong".)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> block/curl.c | 2 ++
> 1 file changed, 2 insertions(+)
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] block/curl.c: Check error return from curl_easy_setopt()
2022-02-22 15:23 ` [PATCH v2 2/2] block/curl.c: Check error return from curl_easy_setopt() Peter Maydell
@ 2022-02-24 14:11 ` Hanna Reitz
2022-02-24 14:26 ` Peter Maydell
0 siblings, 1 reply; 8+ messages in thread
From: Hanna Reitz @ 2022-02-24 14:11 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: Kevin Wolf, qemu-block
On 22.02.22 16:23, 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.
>
> Some of these options are documented as always succeeding (e.g.
> CURLOPT_VERBOSE) but others have documented failure cases (e.g.
> CURLOPT_URL). For consistency we check every call, even the ones
> that theoretically cannot fail.
>
> Fixes: Coverity CID 1459336, 1459482, 1460331
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Changes v1->v2:
> * set the error string in the failure path for the
> direct setopt calls in curl_open()
> * fix the failure path in curl_setup_preadv() by putting
> the curl_easy_setopt() call in the same if() condition
> as the existing curl_multi_add_handle()
> ---
> block/curl.c | 92 +++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 58 insertions(+), 34 deletions(-)
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/2] block/curl: check error return from curl_easy_setopt()
2022-02-22 15:23 [PATCH v2 0/2] block/curl: check error return from curl_easy_setopt() Peter Maydell
2022-02-22 15:23 ` [PATCH v2 1/2] block/curl.c: Set error message string if curl_init_state() fails Peter Maydell
2022-02-22 15:23 ` [PATCH v2 2/2] block/curl.c: Check error return from curl_easy_setopt() Peter Maydell
@ 2022-02-24 14:13 ` Hanna Reitz
2 siblings, 0 replies; 8+ messages in thread
From: Hanna Reitz @ 2022-02-24 14:13 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: Kevin Wolf, qemu-block
On 22.02.22 16:23, 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.
>
> Tested with 'make check' and with some basic smoke test command lines
> suggested by Dan:
>
> qemu-img info https://cloud.debian.org/images/cloud/buster/daily/latest/debian-10-nocloud-amd64-daily.qcow2
> 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
>
> Changes v1->v2:
> * new patch 1 which fixes a missing "set the error string" for
> when curl_init_state() returns failure, since we're about to
> add more cases when that function can fail
> * set the error string in the failure path for the direct setopt
> calls in curl_open()
> * fix the failure path in curl_setup_preadv() by putting
> the curl_easy_setopt() call in the same if() condition
> as the existing curl_multi_add_handle()
Thanks, applied to my block branch:
https://gitlab.com/hreitz/qemu/-/commits/block
Hanna
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] block/curl.c: Check error return from curl_easy_setopt()
2022-02-24 14:11 ` Hanna Reitz
@ 2022-02-24 14:26 ` Peter Maydell
0 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2022-02-24 14:26 UTC (permalink / raw)
To: Hanna Reitz; +Cc: Kevin Wolf, qemu-devel, qemu-block
On Thu, 24 Feb 2022 at 14:11, Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 22.02.22 16:23, 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.
> >
> > Some of these options are documented as always succeeding (e.g.
> > CURLOPT_VERBOSE) but others have documented failure cases (e.g.
> > CURLOPT_URL). For consistency we check every call, even the ones
> > that theoretically cannot fail.
> >
> > Fixes: Coverity CID 1459336, 1459482, 1460331
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Changes v1->v2:
> > * set the error string in the failure path for the
> > direct setopt calls in curl_open()
> > * fix the failure path in curl_setup_preadv() by putting
> > the curl_easy_setopt() call in the same if() condition
> > as the existing curl_multi_add_handle()
> > ---
> > block/curl.c | 92 +++++++++++++++++++++++++++++++++-------------------
> > 1 file changed, 58 insertions(+), 34 deletions(-)
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
For the record, I had a late thought that maybe we were setting
some optional-for-us options that were only added in later versions
of libcurl and accidentally relying on not checking the error code.
But it turns out this isn't the case: most of the options we set
are always supported, and the exceptions are:
NOSIGNAL -- 7.10 onward
PRIVATE -- 7.10.3 onward
USERNAME, PASSWORD, PROXYUSERNAME, PROXYPASSWORD -- 7.19.1 onward,
but we only set these if the user asked for them, so failing
would be the right thing anyway
PROTOCOLS, REDIR_PROTOCOLS -- 7.19.4 onward, guarded by a
compile-time LIBCURL_VERSION_NUM check
And in any case our meson.build insists on at least 7.29.
(That means we could drop the LIBCURL_VERSION_NUM guards, I guess.)
-- PMM
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-02-24 14:28 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-22 15:23 [PATCH v2 0/2] block/curl: check error return from curl_easy_setopt() Peter Maydell
2022-02-22 15:23 ` [PATCH v2 1/2] block/curl.c: Set error message string if curl_init_state() fails Peter Maydell
2022-02-22 21:30 ` Philippe Mathieu-Daudé
2022-02-24 14:08 ` Hanna Reitz
2022-02-22 15:23 ` [PATCH v2 2/2] block/curl.c: Check error return from curl_easy_setopt() Peter Maydell
2022-02-24 14:11 ` Hanna Reitz
2022-02-24 14:26 ` Peter Maydell
2022-02-24 14:13 ` [PATCH v2 0/2] block/curl: check " Hanna Reitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.