Skip Menu |
 

This queue is for tickets about the Imager-File-WEBP CPAN distribution.

Report information
The Basics
Id: 128953
Status: open
Priority: 0/
Queue: Imager-File-WEBP

People
Owner: Nobody in particular
Requestors: ozcoder [...] gmail.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: Coverity scan
Date: Fri, 29 Mar 2019 12:20:23 +1100
To: bug-Imager-File-WEBP [...] rt.cpan.org
From: Gordon Stanton <ozcoder [...] gmail.com>
Download (untitled) / with headers
text/plain 542b
I'm not sure if I have setup Coverity properly, but it found this which I have a silly fix for. What do you think? --- a/imwebp.c +++ b/imwebp.c @@ -45,7 +45,7 @@ find_fourcc(WebPData *d, const char *fourcc, size_t *result_chsize) { p += 12; /* skip the RIFF header */ sz -= 12; while (sz > 8) { - size_t chsize = p[4] | (p[5] << 8) | (p[6] << 16) | (p[7] << 24); + size_t chsize = p[4] | (size_t)(p[5] << 8) | (size_t)(p[6] << 16) | (size_t)(p[7] << 24); if (chsize + 8 > sz) { /* corrupt? */ return NULL;
CC: ;
Subject: Re: [rt.cpan.org #128953] Coverity scan
Date: Fri, 29 Mar 2019 14:12:31 +1100
To: "ozcoder [...] gmail.com via RT" <bug-Imager-File-WEBP [...] rt.cpan.org>
From: Tony Cook <tony [...] develop-help.com>
Download (untitled) / with headers
text/plain 1.7k
On Thu, Mar 28, 2019 at 09:20:42PM -0400, ozcoder@gmail.com via RT wrote: Show quoted text
> Thu Mar 28 21:20:42 2019: Request 128953 was acted upon. > Transaction: Ticket created by ozcoder@gmail.com > Queue: Imager-File-WEBP > Subject: Coverity scan > Broken in: (no value) > Severity: (no value) > Owner: Nobody > Requestors: ozcoder@gmail.com > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=128953 > > > > I'm not sure if I have setup Coverity properly, but it found this > which I have a silly fix for. > What do you think? > > --- a/imwebp.c > +++ b/imwebp.c > @@ -45,7 +45,7 @@ find_fourcc(WebPData *d, const char *fourcc, size_t > *result_chsize) { > p += 12; /* skip the RIFF header */ > sz -= 12; > while (sz > 8) { > - size_t chsize = p[4] | (p[5] << 8) | (p[6] << 16) | (p[7] << 24); > + size_t chsize = p[4] | (size_t)(p[5] << 8) | (size_t)(p[6] << 16) > | (size_t)(p[7] << 24); > if (chsize + 8 > sz) { > /* corrupt? */ > return NULL;
Unfortunately your fix is incorrect, though Coverity is correct to complain. The problem Coverity is complaining about here is that p[7], since it's only a byte, can be trivially converted to int, and the (p[7] << 24) with a 32-bit int with shift the high-bit into the high-bit of the resulting int. If the high bit is 1, then the resulting int is negative, and chsize will become a very large number after conversion to size_t. The correct fix is to cast p[7] to size_t before it's shifted, that way the compiler won't later try to sign-extend it. There's at least one similar fix in Imager: http://git.imager.perl.org/imager.git/commitdiff/e1c0692925 I guess I'll have to put some of these newer modules through Coverity. Thanks for the report. Tony
CC: ;
Subject: Re: [rt.cpan.org #128953] Coverity scan
Date: Sat, 30 Mar 2019 13:30:49 +1100
To: "tony [...] develop-help.com via RT" <bug-Imager-File-WEBP [...] rt.cpan.org>
From: Tony Cook <tony [...] develop-help.com>
On Thu, Mar 28, 2019 at 11:12:47PM -0400, tony@develop-help.com via RT wrote: Show quoted text
> Queue: Imager-File-WEBP > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=128953 > > > On Thu, Mar 28, 2019 at 09:20:42PM -0400, ozcoder@gmail.com via RT wrote:
> > Thu Mar 28 21:20:42 2019: Request 128953 was acted upon. > > Transaction: Ticket created by ozcoder@gmail.com > > Queue: Imager-File-WEBP > > Subject: Coverity scan > > Broken in: (no value) > > Severity: (no value) > > Owner: Nobody > > Requestors: ozcoder@gmail.com > > Status: new > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=128953 > > > > > > > I'm not sure if I have setup Coverity properly, but it found this > > which I have a silly fix for. > > What do you think? > > > > --- a/imwebp.c > > +++ b/imwebp.c > > @@ -45,7 +45,7 @@ find_fourcc(WebPData *d, const char *fourcc, size_t > > *result_chsize) { > > p += 12; /* skip the RIFF header */ > > sz -= 12; > > while (sz > 8) { > > - size_t chsize = p[4] | (p[5] << 8) | (p[6] << 16) | (p[7] << 24); > > + size_t chsize = p[4] | (size_t)(p[5] << 8) | (size_t)(p[6] << 16) > > | (size_t)(p[7] << 24); > > if (chsize + 8 > sz) { > > /* corrupt? */ > > return NULL;
> > Unfortunately your fix is incorrect, though Coverity is correct to > complain. > > The problem Coverity is complaining about here is that p[7], since > it's only a byte, can be trivially converted to int, and the (p[7] << > 24) with a 32-bit int with shift the high-bit into the high-bit of the > resulting int. > > If the high bit is 1, then the resulting int is negative, and chsize > will become a very large number after conversion to size_t. > > The correct fix is to cast p[7] to size_t before it's shifted, that > way the compiler won't later try to sign-extend it. > > There's at least one similar fix in Imager: > > http://git.imager.perl.org/imager.git/commitdiff/e1c0692925 > > I guess I'll have to put some of these newer modules through Coverity. > > Thanks for the report.
Fixed in b32a74c1e0a7dd8a025bad6d0713a38598e600a1.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.