Re: [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value

[Viktor Prutyanov]

Hi,

On Wed, 1 Sep 2021 17:25:09 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 9/1/21 4:39 PM, Peter Maydell wrote:
> > Coverity points out that we aren't checking the return value
> > from curl_easy_setopt().
> > 
> > Fixes: Coverity CID 1458895
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  contrib/elf2dmp/download.c | 28 +++++++++++++++++-----------
> >  1 file changed, 17 insertions(+), 11 deletions(-)
> > 
> > diff --git a/contrib/elf2dmp/download.c b/contrib/elf2dmp/download.c
> > index d09e607431f..01e4a7fc0dc 100644
> > --- a/contrib/elf2dmp/download.c
> > +++ b/contrib/elf2dmp/download.c
> > @@ -21,21 +21,19 @@ int download_url(const char *name, const char
> > *url) 
> >      file = fopen(name, "wb");
> >      if (!file) {
> > -        err = 1;
> > -        goto out_curl;
> > +        goto fail;
> >      }
> >  
> > -    curl_easy_setopt(curl, CURLOPT_URL, url);
> > -    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, NULL);
> > -    curl_easy_setopt(curl, CURLOPT_WRITEDATA, file);
> > -    curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1);
> > -    curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0);
> > +    if (curl_easy_setopt(curl, CURLOPT_URL, url) != CURLE_OK ||
> > +        curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, NULL) !=
> > CURLE_OK ||
> > +        curl_easy_setopt(curl, CURLOPT_WRITEDATA, file) !=
> > CURLE_OK ||
> > +        curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1) !=
> > CURLE_OK ||
> > +        curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0) != CURLE_OK)
> > {
> > +        goto fail;
> > +    }
> >  
> >      if (curl_easy_perform(curl) != CURLE_OK) {
> > -        err = 1;
> > -        fclose(file);
> > -        unlink(name);
> > -        goto out_curl;
> > +        goto fail;
> >      }
> >  
> >      err = fclose(file);
> > @@ -44,4 +42,12 @@ out_curl:
> >      curl_easy_cleanup(curl);
> >  
> >      return err;
> > +
> > +fail:
> > +    err = 1;
> > +    if (file) {
> > +        fclose(file);
> > +        unlink(name);
> > +    }
> > +    goto out_curl;
> >  }
> >   
> 
> Counter proposal without goto and less ifs:
> 
> -- >8 --  
> @@ -25,21 +25,19 @@ int download_url(const char *name, const char
> *url) goto out_curl;
>      }
> 
> -    curl_easy_setopt(curl, CURLOPT_URL, url);
> -    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, NULL);
> -    curl_easy_setopt(curl, CURLOPT_WRITEDATA, file);
> -    curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1);
> -    curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0);
> -
> -    if (curl_easy_perform(curl) != CURLE_OK) {
> -        err = 1;
> -        fclose(file);
> +    if (curl_easy_setopt(curl, CURLOPT_URL, url) != CURLE_OK
> +            || curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, NULL) !=
> CURLE_OK
> +            || curl_easy_setopt(curl, CURLOPT_WRITEDATA, file) !=
> CURLE_OK
> +            || curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1) !=
> CURLE_OK
> +            || curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0) !=
> CURLE_OK
> +            || curl_easy_perform(curl) != CURLE_OK) {
>          unlink(name);
> -        goto out_curl;
> +        fclose(file);
> +        err = 1;
> +    } else {
> +        err = fclose(file);
>      }
> 
> -    err = fclose(file);
> -
>  out_curl:
>      curl_easy_cleanup(curl);
> 
> ---
> 

Honestly, I would prefer this version over the original patch.
In any way, I have tested both of them.

-- 
Viktor Prutyanov