All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] elf2dmp: Fix minor Coverity nits
@ 2021-09-01 14:39 Peter Maydell
  2021-09-01 14:39 ` [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value Peter Maydell
  2021-09-01 14:39 ` [PATCH 2/2] elf2dmp: Fail cleanly if PDB file specifies zero block_size Peter Maydell
  0 siblings, 2 replies; 7+ messages in thread
From: Peter Maydell @ 2021-09-01 14:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: Viktor Prutyanov

Coverity complains about a couple of minor issues in elf2dmp:
 * we weren't checking the return value from curl_easy_setopt()
 * we might divide by zero if presented with a corrupt PDB file

This patchseries fixes those.

NB: I have only compile tested this as I don't have any
files to test it with.

thanks
-- PMM

Peter Maydell (2):
  elf2dmp: Check curl_easy_setopt() return value
  elf2dmp: Fail cleanly if PDB file specifies zero block_size

 contrib/elf2dmp/download.c | 28 +++++++++++++++++-----------
 contrib/elf2dmp/pdb.c      |  4 ++++
 2 files changed, 21 insertions(+), 11 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value
  2021-09-01 14:39 [PATCH 0/2] elf2dmp: Fix minor Coverity nits Peter Maydell
@ 2021-09-01 14:39 ` Peter Maydell
  2021-09-01 15:25   ` Philippe Mathieu-Daudé
  2021-09-01 14:39 ` [PATCH 2/2] elf2dmp: Fail cleanly if PDB file specifies zero block_size Peter Maydell
  1 sibling, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2021-09-01 14:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: Viktor Prutyanov

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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] elf2dmp: Fail cleanly if PDB file specifies zero block_size
  2021-09-01 14:39 [PATCH 0/2] elf2dmp: Fix minor Coverity nits Peter Maydell
  2021-09-01 14:39 ` [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value Peter Maydell
@ 2021-09-01 14:39 ` Peter Maydell
  2021-09-08 21:28   ` Viktor Prutyanov
  1 sibling, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2021-09-01 14:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: Viktor Prutyanov

Coverity points out that if the PDB file we're trying to read
has a header specifying a block_size of zero then we will
end up trying to divide by zero in pdb_ds_read_file().
Check for this and fail cleanly instead.

Fixes: Coverity CID 1458869
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 contrib/elf2dmp/pdb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
index b3a65470680..adcfa7e154c 100644
--- a/contrib/elf2dmp/pdb.c
+++ b/contrib/elf2dmp/pdb.c
@@ -215,6 +215,10 @@ out_symbols:
 
 static int pdb_reader_ds_init(struct pdb_reader *r, PDB_DS_HEADER *hdr)
 {
+    if (hdr->block_size == 0) {
+        return 1;
+    }
+
     memset(r->file_used, 0, sizeof(r->file_used));
     r->ds.header = hdr;
     r->ds.toc = pdb_ds_read(hdr, (uint32_t *)((uint8_t *)hdr +
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value
  2021-09-01 14:39 ` [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value Peter Maydell
@ 2021-09-01 15:25   ` Philippe Mathieu-Daudé
  2021-09-08 21:43     ` Viktor Prutyanov
  0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-01 15:25 UTC (permalink / raw)
  To: Peter Maydell, qemu-devel; +Cc: Viktor Prutyanov

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

---



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] elf2dmp: Fail cleanly if PDB file specifies zero block_size
  2021-09-01 14:39 ` [PATCH 2/2] elf2dmp: Fail cleanly if PDB file specifies zero block_size Peter Maydell
@ 2021-09-08 21:28   ` Viktor Prutyanov
  0 siblings, 0 replies; 7+ messages in thread
From: Viktor Prutyanov @ 2021-09-08 21:28 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel

Hi,

On Wed,  1 Sep 2021 15:39:10 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> Coverity points out that if the PDB file we're trying to read
> has a header specifying a block_size of zero then we will
> end up trying to divide by zero in pdb_ds_read_file().
> Check for this and fail cleanly instead.
> 
> Fixes: Coverity CID 1458869
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  contrib/elf2dmp/pdb.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
> index b3a65470680..adcfa7e154c 100644
> --- a/contrib/elf2dmp/pdb.c
> +++ b/contrib/elf2dmp/pdb.c
> @@ -215,6 +215,10 @@ out_symbols:
>  
>  static int pdb_reader_ds_init(struct pdb_reader *r, PDB_DS_HEADER
> *hdr) {
> +    if (hdr->block_size == 0) {
> +        return 1;
> +    }
> +
>      memset(r->file_used, 0, sizeof(r->file_used));
>      r->ds.header = hdr;
>      r->ds.toc = pdb_ds_read(hdr, (uint32_t *)((uint8_t *)hdr +

Looks good.

Reviewed-by: Viktor Prutyanov <viktor.prutyanov@phystech.edu>

-- 
Viktor Prutyanov


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value
  2021-09-01 15:25   ` Philippe Mathieu-Daudé
@ 2021-09-08 21:43     ` Viktor Prutyanov
  2021-09-08 23:25       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 7+ messages in thread
From: Viktor Prutyanov @ 2021-09-08 21:43 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: Peter Maydell, qemu-devel

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.

-- 
Viktor Prutyanov


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value
  2021-09-08 21:43     ` Viktor Prutyanov
@ 2021-09-08 23:25       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-08 23:25 UTC (permalink / raw)
  To: Viktor Prutyanov; +Cc: Peter Maydell, qemu-devel

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?



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-08 23:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-01 14:39 [PATCH 0/2] elf2dmp: Fix minor Coverity nits Peter Maydell
2021-09-01 14:39 ` [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value Peter Maydell
2021-09-01 15:25   ` Philippe Mathieu-Daudé
2021-09-08 21:43     ` Viktor Prutyanov
2021-09-08 23:25       ` Philippe Mathieu-Daudé
2021-09-01 14:39 ` [PATCH 2/2] elf2dmp: Fail cleanly if PDB file specifies zero block_size Peter Maydell
2021-09-08 21:28   ` Viktor Prutyanov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.