diff mbox series

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

Message ID 20210901143910.17112-2-peter.maydell@linaro.org
State New
Headers show
Series elf2dmp: Fix minor Coverity nits | expand

Commit Message

Peter Maydell Sept. 1, 2021, 2:39 p.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé Sept. 1, 2021, 3:25 p.m. UTC | #1 
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);

---
Viktor Prutyanov Sept. 8, 2021, 9:43 p.m. UTC | #2 
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.
Philippe Mathieu-Daudé Sept. 8, 2021, 11:25 p.m. UTC | #3 
On 9/8/21 11:43 PM, Viktor Prutyanov wrote:
> 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.

OK I will post this properly and let Peter pick whichever he
prefers. Do you mind to reply to the cover letter with a
"Tested-by: Viktor Prutyanov <viktor.prutyanov@phystech.edu>"
tag?
diff mbox series

Patch

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;
 }