|
From: | Hanna Reitz |
Subject: | Re: [PATCH] block/curl.c: Check error return from curl_easy_setopt() |
Date: | Tue, 1 Feb 2022 12:25:03 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 |
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;
[Prev in Thread] | Current Thread | [Next in Thread] |