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

[Philippe Mathieu-Daudé]

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.mayd...@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);

---