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

Report information
The Basics
Id:
128953
Status:
resolved
Priority:
Low/Low

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

BugTracker
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>
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>
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 was fixed in 0.005


This service runs on Request Tracker, is sponsored by The Perl Foundation, and maintained by Best Practical Solutions.

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