On 9/8/21 11:43 PM, Viktor Prutyanov wrote:
> On Wed, 1 Sep 2021 17:25:09 +0200
> Philippe Mathieu-Daudé <phi...@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.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);
>>
>> ---
>>
>
> 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.prutya...@phystech.edu>"
tag?
