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
