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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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?