search

pnggroup / libpng

LIBPNG: Portable Network Graphics support, official libpng repository
http://libpng.sf.net
Other
1.25k stars 611 forks source link

Correctly damage palette index test PNG #524

Closed jbowler closed 7 months ago

jbowler commented 7 months ago

The PNG IDAT did not include a '255' entry, the highest entry is '254', this corrects the test PNG to have a palette with only 254 entries so that it triggers the palette index checks.

ctruta commented 7 months ago

I integrated your change (thanks!) but I rephrased the subject line to make it less scary and more obvious to the casual reader. The "trick" was to use "quotes" with the word "correctly" 😝

On an unrelated note: could you @jbowler please keep your fixes coming, preferably (although not necessarily) into https://github.com/pnggroup/libpng? I want to release libpng-1.6.41, either today or in a day or two, and then I want to mark that other one as the official one, going forward.

jbowler commented 7 months ago

It rebased ok.

There is one very important fix missing; the revert of the commit which broke pngtrans.c png_do_check_palette_indexes. Without this the "sample-*" bad PNGs don't report the error. The test is just:

./pngtest contrib/testpngs/badpal/small-palette-1.png foo.png 2>&1 | egrep 'Read palette index exceeding num_palette'

Which should succeed but fails for small-palette-[124].png It succeeds (i.e. pngtest outputs the error) for the other five cases including small-palette-8.png because there's yet another bug - signed/unsigned compare on line 784 of pngtrans.c which makes a mess of num_palette_maxwhen it starts out -1 (but why is it -1?)

ctruta commented 7 months ago 
./pngtest contrib/testpngs/badpal/small-palette-1.png foo.png 2>&1 | egrep 'Read palette index exceeding num_palette'

I'm not seeing what you're seeing. The only message I'm seeing on my development machine is the usual pngtest complaint

Files contrib/testpngs/badpal/small-palette-1.png and foo.png are different
jbowler commented 7 months ago

That is definitely wrong and I fully synced to glennrp/libpng16 before I posted that. I'll post the full output of the current HEAD from my own dev system.

jbowler commented 7 months ago

Precise repro:

git clean -dfx # warning; try git clean -dfxn first!
mkdir build.conf
cd build.conf
../configure
make check # succeeds and builds pngtest
./pngtest ../contrib/testpngs/badpal/small-palette-1.png foo.png

This does not produce the read IDAT error, even though it should! I might have mistyped the egrep, since I did it by hand. I'll append it in a couple of minutes:

The update, using the modified regression image which has the '255' entry in the last byte:

jbowler@hippopopus ~/src/libpng/github/git/build.conf $ ./pngtest ../contrib/testpngs/badpal/regression-palette-8.png foo.png|egrep 'Read palette index exceeding num_palette'
jbowler@hippopopus ~/src/libpng/github/git/build.conf $

Whereas with the original "small" (per makepng) image where the '255' is in the last but one byte:

jbowler@hippopopus ~/src/libpng/github/git/build.conf $ ./pngtest ../contrib/testpngs/badpal/small-palette-8.png foo.png|egrep 'Read palette index exceeding num_palette'
libpng error: Wrote palette index exceeding num_palette
libpng error: Wrote palette index exceeding num_palette
libpng error: Wrote palette index exceeding num_palette
../contrib/testpngs/badpal/small-palette-8.png: libpng warning: IDAT: Read palette index exceeding num_palette
../contrib/testpngs/badpal/small-palette-8.png: libpng warning: IDAT: Read palette index exceeding num_palette
../contrib/testpngs/badpal/small-palette-8.png: libpng warning: IDAT: Read palette index exceeding num_palette
jbowler@hippopopus ~/src/libpng/github/git/build.conf $

Since that may have been slightly confusing as I used the 1bpp case (which succeeds, producing the error message), here's 1bpp in the same format:

jbowler@hippopopus ~/src/libpng/github/git/build.conf $ ./pngtest ../contrib/testpngs/badpal/small-palette-8.png foo.png|egrep 'Read palette index exceeding num_palette'
libpng error: Wrote palette index exceeding num_palette
libpng error: Wrote palette index exceeding num_palette
libpng error: Wrote palette index exceeding num_palette
../contrib/testpngs/badpal/small-palette-8.png: libpng warning: IDAT: Read palette index exceeding num_palette
../contrib/testpngs/badpal/small-palette-8.png: libpng warning: IDAT: Read palette index exceeding num_palette
../contrib/testpngs/badpal/small-palette-8.png: libpng warning: IDAT: Read palette index exceeding num_palette
jbowler@hippopopus ~/src/libpng/github/git/build.conf $

So, to clarify since it's all a double negative: to succeed the test must fail, failure means not producing the error message since all the test cases (all the regression PNGs in badpal) have the error in question. When all the bugs are fixed every one of those PNGs produces the error message.