* [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.