853190 - pngset.c:701:8: warning: comparison is always false due to limited range of data type [-Wtype-limits]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

Build warning:
{
media/libpng/pngset.c: In function 'MOZ_PNG_set_text_2':
media/libpng/pngset.c:701:8: warning: comparison is always false due to limited range of data type [-Wtype-limits]
         (unsigned int)/*SAFE*/(num_text +/*SAFE*/
         ^
}

This is for this chunk of code:
{
699    if (num_text < 0 ||
700        num_text > INT_MAX - info_ptr->num_text - 8 ||
701        (unsigned int)/*SAFE*/(num_text +/*SAFE*/
702        info_ptr->num_text + 8) >=
703        PNG_SIZE_MAX/png_sizeof(png_text))
}
https://mxr.mozilla.org/mozilla-central/source/media/libpng/pngset.c#699

...added here, in January:
https://hg.mozilla.org/mozilla-central/diff/51d02de3f48e/media/libpng/pngset.c#l1.29

...for bug 832487.

(not sure offhand which part of that expression is always-false, but it looks like the warning is pointing to lines 701 through 703.)

This is technically an upstream bug, but I've seen other libpng-related bugs filed in bugzilla.mozilla.org before, so I'm starting out by filing it here. Glenn, let me know if there's somewhere else I should file this.

Comment 1

[Daniel Holbert [:dholbert]]

(looks like bug 841734 is already tracking another libpng upgrade. I wonder if that upgrade modifies this code & addresses this build warning.)

Comment 2

[u153838]

Compiler version?

Comment 3

[u153838]

Answering my own question:

test -n "$LIB_OLD" ## i.e. libpng 1.5.x ## && case "${cpu}-${compiler}-${version}" in

* * * 

*-g??-4.*)
     warnings_off+=" -Wno-type-limits"
     :;;&

* * *

which, UTSL, speaks for itself - either upgrade to 1.6 or turn off -Wtype-limits.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to [e-mail] from comment #2)
> Compiler version?

GCC 4.8 (from "gcc-snapshot" package on Ubuntu 13.04 prerelease) though I'd expect the same results from earlier GCC versions as well.  (Sounds like it, from comment 3)
(In reply to [e-mail] from comment #3)

> which, UTSL, speaks for itself - either upgrade to 1.6 or turn off
> -Wtype-limits.

Turning off -Wtype-limits isn't an option :) We'd like to know when we perform useless comparisons.  There's also no dire need to silence the warning -- I just filed this bug since this build warning usually indicates that something's wrong (or at least unnecessary) in the code.

However -- upgrading to 1.6 seems to be happening in the not-too-distant future (bug 841734).  [Marking this as depending on that bug.] It sounds like this is actually fixed there (or the code has changed such that this is no longer an issue) -- is that right?

Comment 5

[Daniel Holbert [:dholbert]]

(Thanks for the quick response, BTW!)

Comment 6

[u153838]

I don't know about gcc 4.8; 4.7.2 is the latest I have, but that version emits the warning for libpng 1.5 but not for libpng 1.6.1rc01, and not, I think, for 1.6.0.  This is with -Wall -Wextra -Werror plus a number of other warnings not in Wall+Wextra.

gcc doesn't warn about most of the comparisons in libpng; there are a lot of the form:

if (new_count * sizeof (foo) > MAX_SIZE_T - count * sizeof (foo))

but it does seem to object to:

if (new_count > (MAX_SIZE_T/sizeof (foo) - count)

and there are three cases of using division in pngset.c - if I rewrite those to the first form GCC no longer objects.  The issue is that the *second* form preserves (or enforces) the data types, whereas the multiply in the first form makes the LHS of type (size_t) but with a limited range.  I believe current versions of GCC don't warn about expressions that are true because of a limited range of *values* but do warn about expressions that are true because of a limit on the *types*.

The three cases in question were rewritten in 1.6 to avoid having the same tricky code in multiple places and the expression that is evaluated for the division is no longer constant (well, it is, but this is hidden from the compiler).

Comment 7

[Glenn Randers-Pehrson]

Fixed by checkin of libpng-1.6.6