Re: [PATCH] block/curl.c: Check error return from curl_easy_setopt()

[Peter Maydell <peter.maydell@linaro.org>]

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