code cleanup: chunk lengths and allocation limits

[jbowler]

Internal changes only.

Move chunk length checks to fewer places:

Change png_struct::user_chunk_malloc_max to always have a non-zero value
in order to avoid the need to check for zero in multiple places.

Add png_chunk_max(png_ptr), a function like macro defined in pngpriv.h
which expresses all the previous checks on the various USER_LIMITS and
system limitations. Replace the code which implemented such checks with
png_chunk_max.

Move the malloc limit length check in png_read_chunk_header to
png_handle_chunk and make it conditional on the chunk type.

Progressive reader: call png_read_chunk_header.

Corect the handling for the pHYs.

Signed-off-by: John Bowler jbowler@acm.org

[ctruta]

A few FYI's and a thumb down; see my full review.

[jbowler]

A few FYI's and a thumb down; see my full review.

I just pushed the changes.

Not a part of this change but getting rid of PNG_MAXSEG_64K, plus PNG_SMALL_SIZE_T and requiring (unsigned int) (which is what uInt is) be at least 32 bits would simplify the whole zlib decompression code; at present z_stream::avail_in is a uInt (as is IDAT_read_size) and ZLIB_IO_MAX is just (uInt)-1 so the decompression code has to loop just in case uInt (hence unsigned int) is too small.

IDAT_read_size hasn't been changed since 2013.

[ctruta]

Not a part of this change but getting rid of PNG_MAXSEG_64K, plus PNG_SMALL_SIZE_T and requiring (unsigned int) (which is what uInt is) be at least 32 bits would simplify the whole zlib decompression code; at present z_stream::avail_in is a uInt (as is IDAT_read_size) and ZLIB_IO_MAX is just (uInt)-1 so the decompression code has to loop just in case uInt (hence unsigned int) is too small.

If my understanding of your statement is correct (or, if it isn't, please correct me) you are implying that the 16-bit support, in which int and/or size_t are less than 32-bit, will be no longer. And if that's the case indeed, green light from me 🟢

[jbowler]

Broadly my suggestion is to require INT_MAX and SIZE_MAX (only available from C99 on) to be at least PNG_UINT_32_MAX.

The PNG_SMALL_SIZE_T mess arises because C90 does not define SIZE_MAX and PNG_SIZE_MAX is therefore no valid in a pre-processor statement. So PNG_SMALL_SIZE_T has to get set either by the magic "known compilers" stuff at starting at line 530 of pngconf.h or by definition in CFLAGS.

A minimal change, without going to C99+, is to #error out if PNG_SMALL_SIZE_T is defined and then also if UINT_MAX is less than PNG_UINT_32_MAX. Similarly the setting of PNG_MAX_MALLOC_64K at line 436 of pngpriv.h would be changed to a #error. If that goes into a release and no one complains then the next step would be to simplify the code and put in runtime checks on PNG_SIZE_MAX and ZLIB_IO_MAX just to be safe.

[ctruta]

We should start using C99's stdint.h (if not C99 or newer, entirely) in libpng18.

[jbowler]

That's another powerful argument for C23:

https://en.cppreference.com/w/c/language/typeof

libpng users can't be expected to change their own code from using png_uint_32 because, as pngconf.h says, there's no guarantee as to what type it is. Maybe it will become (short) on systems where (int) is 64 bits; I'm not sure the current libpng compiles on those systems.

So all we can do is change pngconf.h to define all the png_ types in terms of stdint.h and change all the API definitions and internal code to not use the legacy typedefs; people will gradually stop using them but they will still be used by copy'n'paste of legacy app code. They'll never go away.

Easy to do in libpng-1.8. Could be done in libpng-1.6 by requiring <stdint.h> but a little tricky because png_uint_32 can currently end up as 64-bits (but I don't think that compiles without warnings/errors.)

The same applies to zlib users. We have to use uInt to guarantee no truncation prior to C23.

It's part of the legacy pre-stdint world and API compatibility and is one good example of where zlib-ng gets it wrong:

• zconf.h - typedef unsigned int uInt; could be 16, 32 or 64 bits.
  uInt avail_in; so could be 16, 32 or 64 bits.
• zconf-ng.h - identical typedef of uInt - 16, 32 or 64 bits
  uint32_t avail_in; oops

The avail_in and avail_out parameters should be size_t but that would be an API change so they half did the work. They changed avail_in/avail_out to be guaranteed 32-bits but did not make the same change to uInt. Why not? What they did simply doesn't work; it leads to truncation on systems with 64-bit (unsigned int).

But we, as users of zlib, don't need to know any of this:

typeof(zstream.avail_in) avail_in and then the compiler will warn on potential truncation of a size_t object but:

if (sizeof typeof(zstream.avail_in) < sizeof size_t) * * *

That expression is a compile time constant, so some compilers will warn, but it's necessary with zlib_ng because the decompressed output of a PNG is currently limited only by (size_t) on most systems so can be larger than uint32_t, indeed is on my systems. Oops.

[ctruta]

Let's take it slowly. We haven't even gone beyond C89 yet. (Which, by the way, we should.)

One thing is rather clear to me: in libpng18, we can and we should take for granted the availability of snprintf and stdint.h, and the guarantee that sizeof(uint32_t)<=sizeof(size_t), and the guarantee that there's only one pointer size (i.e. the pointer size, without any "near" and "far" pointer-ing).

All of the above, and maybe even more, can be compiled with gcc -std=c89 -- notwithstanding the fact that I really don't think that we should keep on maintaining the C89 compatibility any longer.