Skip reassignment of the initialization value

[irwir]

This was incorrect: Reversed condition changed to less instead of less or equal because the value still should be PNG_UINT_31_MAX.
Reassignment of the same value is a common kind of copy-paste errors.
Hence static analysis emits warning in this case.
A better way to avoid reassignment is in the updated commit.

[jbowler]

I don't see any reason for such a change. The compiler can work it out too and making the change requires a code review which explains why the change from:

if (a > b)
  spurious assignment;
else if (a <= b)
  required assignment;

To:

if (a < b)
   required assignment;

Is correct. I couldn't work it out; why is the case "a == b" irrelevant?

However, back to the words of the pull request rather than the code; why is this better documentation of what the code does than the original form, which you present as equivalent (ignoring the "a == b" issue)? It seems to me that "if/else" is always more clear ab initio than "if".

[irwir]

Thanks for reviewing.
I changed the code and the description in the first message.

[jbowler]

libpng-ng comment: libpng(ng) shouldn't even contain this code for IDAT There is no point detecting an over-long IDAT unless it exceeds the PNG spec length (indicating a corrupted stream). IDAT buffering needs to be controlled internally to optimize the specific zlib implementation (if known) and the architecture performance; small buffers or large buffers depend on the relative speed of the zlib window copy vs the libpng row handling for large images and for small images the code can avoid the window copy entirely if enough is known about the zlib implementation.

[irwir]

As could be read, at least one more version of 1.6 was planned. So this code might stay for some time, while 1.7 branch did not pan out.

PS. About 5 years ago in one discussion it was suggested to rename libspng to libpng-ng. It did not happen.

[jbowler]

@ctruta: won't fix

[ctruta]

@ctruta: won't fix

"If It Ain't Broken Don't Fix It" and yet the code patched by @irwir is neither "broken" nor "not broken".

We work in the C89 mode and the C89 mindset in which variable declarations and variable definitions need not be co-located. (But they should be from C99 onward, and they must be in C++ and Rust and any other RAII language.)

Moreover, I actually like the way in which @irwir is using the ternary operator. It is more obvious (if only slightly more obvious) that the code pattern looks like an initialization of idat_limit with a hand-coded min function, except... it isn't.

It is for this reason I am saying that the code in question is neither "broken" nor "not broken". It looks like an accidental typo. It makes no difference semantically if we replace PNG_UINT_32_MAX with PNG_UINT_31_MAX, or if we don't, but... if we don't, then we should at least add in a comment in which we state that we intentionally don't and here is why. My personal preference would be not to add any comment, but rather, to replace PNG_UINT_32_MAX with PNG_UINT_31_MAX instead.

@irwir if you do modify your code further, as I'm suggesting here, I will appreciate it. But then, if you could please add your name -- your real name -- to the AUTHORS file in your updated commit, that'd be great!

[irwir]

RAII is not a must in C++, but a good practice in many cases.
The orignal code also follows RAII guidelines, and the only purpose of this PR was to eliminate the warning "assigned value was never used" without any deeper code investigation.
If there are other issues with the code, an appropriate fix should be applied.

As for 32 vs 31, if neither png_ptr->width nor png_ptr->channels are zeros (not allowed in PNG specification?), the factor would be at least 2. Therefore, it was most probably intentional and needs no changes.
What further changes could be considered:

• maximum allocation size could be above 32-bit value in 64-bit mode;
• user's limits might be taken into accout - such as PNG_USER_CHUNK_MALLOC_MAX, PNG_USER_HEIGHT_MAX or PNG_USER_WIDTH_MAX.

[ctruta]

@irwir, if I accept your contributions (this one and/or others in the future), I would very much appreciate it if you could please reveal your real name. Libpng is considered "free software". Its license can pass a suite of "freedom tests", including the "dissident test" (https://wiki.debian.org/DissidentTest).

However, the libpng license also requires all modifications to be clearly marked. If we integrate a contributor's modification into the core libpng sources (especially if the contribution is non-trivial), we acknowledge the contributor's name, for copyright purposes (https://github.com/pnggroup/libpng/blob/libpng16/AUTHORS).

Under these considerations, could you please tell us your real-world name, or modify your commits to show this name in the Author field?

[jbowler]

The calling code is "png_read_chunk_header" which is implemented inline (effectively) in pngpread.c resulting in code duplication and potential inconsistency between the progressive and sequential readers. My previous argument stands; both readers read IDAT incrementally so no allocation is required so the check should not be performed for 'IDAT'. Also the progressive reader should be calling png_read_chunk_header since it has already ensured that the push buffer is filled.

I'll look at it again after #645 is in; it should be inlined into png_read_chunk_header and, anyway, limit > PNG_UINT_31_MAX is not in the least bit "benign" - both the progressive read and the sequential reader are apparently going to stall or get an io error immediately afterward. Normally this error can only arise with a corrupted stream and libpng can't do any recovery.

This is also related to the changes I made to png_check_chunk_name; I wanted to call 'check_chunk_name' in png_read_chunk_header because on some systems inlining the check may allow for massive speed improvement but the fact that png_check_chunk_name is an extern discourages the inline and makes it less valuable.