* [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
* 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 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
* [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 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
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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).