Re: [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value
On 9/8/21 11:43 PM, Viktor Prutyanov wrote:
> On Wed, 1 Sep 2021 17:25:09 +0200
> Philippe Mathieu-Daudé 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
>>> ---
>>> 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 "
tag?
Re: [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value
Hi,
On Wed, 1 Sep 2021 17:25:09 +0200
Philippe Mathieu-Daudé 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
> > ---
> > 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
Re: [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value
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
> ---
> 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);
---
[PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value
Coverity points out that we aren't checking the return value
from curl_easy_setopt().
Fixes: Coverity CID 1458895
Signed-off-by: Peter Maydell
---
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;
}
--
2.20.1
