libpng 1.6.36 build failure on linux arm64 (Debian and Ubuntu)

[LocutusOfBorg]

See e.g. https://buildd.debian.org/status/fetch.php?pkg=libpng1.6&arch=arm64&ver=1.6.36-1&stamp=1545917181&raw=0

make  check-TESTS
make[3]: Entering directory '/<<PKGBUILDDIR>>'
make[4]: Entering directory '/<<PKGBUILDDIR>>'
PASS: tests/pngtest
PASS: tests/pngtest-badpngs
PASS: tests/pngvalid-gamma-16-to-8
PASS: tests/pngvalid-gamma-threshold
FAIL: tests/pngvalid-gamma-expand16-transform
PASS: tests/pngvalid-progressive-interlace-standard
PASS: tests/pngvalid-gamma-expand16-background
PASS: tests/pngvalid-gamma-background
PASS: tests/pngvalid-gamma-transform
PASS: tests/pngvalid-progressive-standard
PASS: tests/pngvalid-standard
PASS: tests/pngvalid-progressive-size
PASS: tests/pngvalid-gamma-sbit
FAIL: tests/pngvalid-transform
PASS: tests/pngvalid-gamma-alpha-mode
PASS: tests/pngvalid-gamma-expand16-alpha-mode
PASS: tests/pngstest-linear-alpha
PASS: tests/pngunknown-IDAT
PASS: tests/pngunknown-discard
PASS: tests/pngunknown-if-safe
PASS: tests/pngunknown-sAPI
PASS: tests/pngunknown-sTER
PASS: tests/pngunknown-save
PASS: tests/pngunknown-vpAg
PASS: tests/pngstest-1.8-alpha
PASS: tests/pngimage-quick
PASS: tests/pngstest-linear
PASS: tests/pngstest-1.8
PASS: tests/pngstest-none-alpha
PASS: tests/pngstest-sRGB-alpha
PASS: tests/pngstest-none
PASS: tests/pngstest-sRGB
PASS: tests/pngimage-full
=====================================
   libpng 1.6.36: ./test-suite.log
=====================================

# TOTAL: 33
# PASS:  31
# SKIP:  0
# XFAIL: 0
# FAIL:  2
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: tests/pngvalid-gamma-expand16-transform
=============================================

Gamma correction error summary

The printed value is the maximum error in the pixel values
calculated by the libpng gamma correction code.  The error
is calculated as the difference between the output pixel
value (always an integer) and the ideal value from the
libpng specification (typically not an integer).

Expect this value to be less than .5 for 8 bit formats,
less than 1 for formats with fewer than 8 bits and a small
number (typically less than 5) for the 16 bit formats.
For performance reasons the value for 16 bit formats
increases when the image file includes an sBIT chunk.
gamma 0.455->1.000: read indexed-colour[0] 8 bit: memory lost (list follows):
	1024 bytes @ 0xaaaac9b84440

Basic gamma correction:
  2 bit gray:  127.07023
  4 bit gray:  127.07023
  8 bit gray:  128.32752
  8 bit color: 128.32752
  indexed:     128.32752
 16 bit gray:  0.74080
 16 bit color: 0.74080
pngvalid: 
pngvalid: 1 errors, 0 warnings
FAIL: pngvalid --strict --gamma-transform --expand16 (floating point arithmetic)
FAIL tests/pngvalid-gamma-expand16-transform (exit status: 1)

FAIL: tests/pngvalid-transform
==============================

transform: +rgb_to_gray: read indexed-colour[0] 8 bit: memory lost (list follows):
	1024 bytes @ 0xaaaad27974c0
pngvalid: 
pngvalid: 1 errors, 0 warnings
FAIL: pngvalid --strict --transform (floating point arithmetic)
FAIL tests/pngvalid-transform (exit status: 1)

============================================================================
Testsuite summary for libpng 1.6.36
============================================================================
# TOTAL: 33
# PASS:  31
# SKIP:  0
# XFAIL: 0
# FAIL:  2
# XPASS: 0
# ERROR: 0
============================================================================
See ./test-suite.log
Please report to png-mng-implement@lists.sourceforge.net

[LocutusOfBorg]

a git bisect reveals 7734cda as the bad commit

[LocutusOfBorg]

@richard-townsend-arm do you have any idea for this regression on aarch64?

[ctruta]

Looks like a memory leak in the ARM-specific png_do_expand_palette. I don't have access to an aarch64 machine at this moment. I'll get back to you in a week or two, unless @richard-townsend-arm comes back to us with a fix.

[ctruta]

Speculative fix: #268

Caveat: not tested. (No ARM machine available at this moment; see my other comment above.)

[LocutusOfBorg]

Seems to be not fixing this issue...

LD_LIBRARY_PATH=".libs:" dh_auto_test
	make -j8 test VERBOSE=1
make[2]: Entering directory '/<<PKGBUILDDIR>>'
/usr/bin/make  pngtest pngunknown pngstest pngvalid pngimage pngcp timepng
make[3]: Entering directory '/<<PKGBUILDDIR>>'
make[3]: 'pngtest' is up to date.
make[3]: 'pngunknown' is up to date.
make[3]: 'pngstest' is up to date.
make[3]: 'pngvalid' is up to date.
make[3]: 'pngimage' is up to date.
make[3]: 'pngcp' is up to date.
make[3]: 'timepng' is up to date.
make[3]: Leaving directory '/<<PKGBUILDDIR>>'
/usr/bin/make  check-TESTS
make[3]: Entering directory '/<<PKGBUILDDIR>>'
make[4]: Entering directory '/<<PKGBUILDDIR>>'
PASS: tests/pngtest
PASS: tests/pngtest-badpngs
PASS: tests/pngvalid-gamma-threshold
PASS: tests/pngvalid-gamma-16-to-8
FAIL: tests/pngvalid-gamma-expand16-transform
PASS: tests/pngvalid-progressive-size
PASS: tests/pngvalid-progressive-interlace-standard
PASS: tests/pngvalid-progressive-standard
PASS: tests/pngvalid-gamma-transform
PASS: tests/pngvalid-gamma-expand16-background
PASS: tests/pngvalid-gamma-background
PASS: tests/pngvalid-standard
FAIL: tests/pngvalid-transform
PASS: tests/pngvalid-gamma-sbit
PASS: tests/pngvalid-gamma-alpha-mode
PASS: tests/pngvalid-gamma-expand16-alpha-mode
PASS: tests/pngstest-linear-alpha
PASS: tests/pngstest-1.8-alpha
PASS: tests/pngunknown-IDAT
PASS: tests/pngunknown-discard
PASS: tests/pngunknown-if-safe
PASS: tests/pngunknown-sAPI
PASS: tests/pngunknown-sTER
PASS: tests/pngunknown-save
PASS: tests/pngunknown-vpAg
PASS: tests/pngimage-quick
PASS: tests/pngstest-linear
PASS: tests/pngstest-1.8
PASS: tests/pngstest-none-alpha
PASS: tests/pngstest-none
PASS: tests/pngstest-sRGB-alpha
PASS: tests/pngstest-sRGB
PASS: tests/pngimage-full
qemu: Unsupported syscall: 276
=====================================
   libpng 1.6.36: ./test-suite.log
=====================================

# TOTAL: 33
# PASS:  31
# SKIP:  0
# XFAIL: 0
# FAIL:  2
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: tests/pngvalid-gamma-expand16-transform
=============================================

Gamma correction error summary

The printed value is the maximum error in the pixel values
calculated by the libpng gamma correction code.  The error
is calculated as the difference between the output pixel
value (always an integer) and the ideal value from the
libpng specification (typically not an integer).

Expect this value to be less than .5 for 8 bit formats,
less than 1 for formats with fewer than 8 bits and a small
number (typically less than 5) for the 16 bit formats.
For performance reasons the value for 16 bit formats
increases when the image file includes an sBIT chunk.
gamma 0.455->1.000: read indexed-colour[0] 8 bit: memory lost (list follows):
	1024 bytes @ 0x4000b41490

Basic gamma correction:
  2 bit gray:  127.07023
  4 bit gray:  127.07023
  8 bit gray:  128.32752
  8 bit color: 128.32752
  indexed:     128.32752
 16 bit gray:  0.74080
 16 bit color: 0.74080
pngvalid: 
pngvalid: 1 errors, 0 warnings
FAIL: pngvalid --strict --gamma-transform --expand16 (floating point arithmetic)
FAIL tests/pngvalid-gamma-expand16-transform (exit status: 1)

FAIL: tests/pngvalid-transform
==============================

transform: +rgb_to_gray: read indexed-colour[0] 8 bit: memory lost (list follows):
	1024 bytes @ 0x4000b2c510
pngvalid: 
pngvalid: 1 errors, 0 warnings
FAIL: pngvalid --strict --transform (floating point arithmetic)
FAIL tests/pngvalid-transform (exit status: 1)

============================================================================
Testsuite summary for libpng 1.6.36
============================================================================
# TOTAL: 33
# PASS:  31
# SKIP:  0
# XFAIL: 0
# FAIL:  2
# XPASS: 0
# ERROR: 0
============================================================================
See ./test-suite.log
Please report to png-mng-implement@lists.sourceforge.net
============================================================================
make[4]: *** [Makefile:1457: test-suite.log] Error 1
make[4]: Leaving directory '/<<PKGBUILDDIR>>'
make[3]: *** [Makefile:1565: check-TESTS] Error 2
make[3]: Leaving directory '/<<PKGBUILDDIR>>'
make[2]: *** [Makefile:1998: check-am] Error 2
make[2]: Leaving directory '/<<PKGBUILDDIR>>'
dh_auto_test: make -j8 test VERBOSE=1 returned exit code 2
make[1]: *** [debian/rules:21: override_dh_auto_test] Error 2
make[1]: Leaving directory '/<<PKGBUILDDIR>>'
make: *** [debian/rules:14: build] Error 2
dpkg-buildpackage: error: debian/rules build subprocess returned exit status 2
--------------------------------------------------------------------------------
Build finished at 2018-12-31T08:34:59Z

btw I use pbuilder-dist to setup an arm64 machine on my amd64 pc
(pbuilder-dist sid arm64 create), with qemu it works well for debugging this kind of things, even if slow :)

[richard-townsend-arm]

Hi, thanks for reporting @LocutusOfBorg. The riffled palette is currently allocated lazily, so I think it's just figuring out how to make sure that the test suite cleans it up in an appropriate place, I'll be able to investigate it a bit more once I'm back in the office.

[LocutusOfBorg]

thanks a lot!

[LocutusOfBorg]

right now, I'm disabling your arm optimizations on arm64, and this solution is really sub-optimal for Debian and Ubuntu :)

[hmaarrfk]

@LocutusOfBorg do you have a patch for disabling the optimization you want to share?

[LocutusOfBorg]

yes, reverting the commits on arm64 :)
apply the following patches:
https://sources.debian.org/src/libpng1.6/1.6.36-2/debian/patches/revert-1ceaa83a844cd3ecef25279d60720f910b96f297.patch/
https://sources.debian.org/src/libpng1.6/1.6.36-2/debian/patches/revert-b66ed711315c46ef6c556c83c0074ecdcbd9937f.patch/
https://sources.debian.org/src/libpng1.6/1.6.36-2/debian/patches/revert-7734cda20cf1236aef60f3bbd2267c97bbb40869.patch/
conditionally on arm64

[hmaarrfk]

Strange, I keep failing with:

make[1]: *** No rule to make target 'arm/palette_neon_intrinsics.c', needed by 'arm/palette_neon_intrinsics.lo'.  Stop.

Do I need to run any autotools?

[LocutusOfBorg]

sure, because one patch deletes the file :)

[LocutusOfBorg]

this is what I do:

override_dh_autoreconf:
ifeq (arm64,$(DEB_HOST_ARCH))
	patch -p1 < debian/patches/revert-1ceaa83a844cd3ecef25279d60720f910b96f297.patch
	patch -p1 < debian/patches/revert-b66ed711315c46ef6c556c83c0074ecdcbd9937f.patch
	patch -p1 < debian/patches/revert-7734cda20cf1236aef60f3bbd2267c97bbb40869.patch
endif
	dh_autoreconf

[LocutusOfBorg]

so, I have to do it before autoreconfing

[hmaarrfk]

magic, thanks @LocutusOfBorg!

[richard-townsend-arm]

I managed to replicate the issue on an arm64 debian system, @LocutusOfBorg, could you try #272 and see whether it fixes your issue?

[LocutusOfBorg]

yeah it works! I uploaded in debian, lets see what happens in a few minutes
https://buildd.debian.org/status/package.php?p=libpng1.6&suite=unstable

[ctruta]

Thank you for the patches, @richard-townsend-arm

In your two patches, there is the first patch which fixes the core problem as in my original hotfix, and the second one, which fixes the testing issue.

I updated my hotfix also, see the new branch hotfix/memleak-riffled-palette-r2. I hope you won't mind, by I prefer the original solution from my hotfix, because it removes the unnecessary code from pngwrite.c, and it compiles the ARM-specific optimization conditionally so that the rest of the platforms aren't affected. This optimization can be added later to x86, etc., and then the conditionals can be expanded.

As for your 2nd patch, the fix to pngvalid.c, please don't mind me but I'm still looking for a better solution. At this moment I believe that the error is in the validation code, not in libpng itself, and a proper fix is needed. You are making a supplemental call to png_destroy_read_struct() with a contrived set of parameters, which appeases the tests, but the real question is: why is there a need for a supplemental call? Why isn't the original call insufficient?

To illustrate the underlying problem (which is still not fixed, unfortunately), I pushed my draft work to the topic branch topic/fix-pngstruct-memleak. You can see that, outside of ARM-specific code, I'm adding a dummy malloc, which I am freeing, and yet the pngvalid.c test program is showing leaks. With and without your png_destroy_read_struct() call, there are still leaks shown. (3 leaks with your patch, or 4 leaks without your patch.) There should be no leaks.

Still investigating.

[Sentimentron]

This has come back on my radar as a few more people try to roll libpng on arm64 platforms. When you say original call, is store_read_reset in pngvalid.c supposed to take care of cleaning everything up?

[ctruta]

Yes. See my debug experiment:

https://github.com/glennrp/libpng/commits/topic/debug-pngstruct-memleak
7cfabf1

I wanted to investigate it closer last weekend, but something came up and I couldn't. Hope to be able to do it next weekend.

For the time being, just by picking up this hotfix:
https://github.com/glennrp/libpng/commits/hotfix/memleak-riffled-palette-r2
abaf471
the users should consider libpng fixed, even though pngvalid.c fails.

Although I understand that disabling the checks based on pngvalid.c in regression scripts feels less than ideal, it is the best course of action available at this moment. If pngvalid.c fails and yet memory checking tools like Valgrind or Address Sanitizer are not complaining, then the problem is clearly with pngvalid.c.

For the longer term, I contemplate replacing pngvalid.c with simpler tools that do a better job. Address Sanitizer FTW!

[ctruta]

Fixed in master. Also refactored. "Works For Me (tm)."
See 70d122a

A hotfix for 1.6.36 is also available, see the branch hotfix/memleak-riffled-palette.

[LocutusOfBorg]

@ctruta looks like there is still something wrong.
EDIT: I reuploaded to my ppa after forgetting one chunk of the patch
I applied
70d122a
and also
8439534

on top of 1.6.36 release, and it failed on arm64 again
https://launchpad.net/~costamagnagianfranco/+archive/ubuntu/locutusofborg-ppa/+sourcepub/9893938/+listing-archive-extra

   debian/rules override_dh_auto_test
make[1]: Entering directory '/<<PKGBUILDDIR>>'
LD_LIBRARY_PATH=".libs:" dh_auto_test
	make -j4 test VERBOSE=1
make[2]: Entering directory '/<<PKGBUILDDIR>>'
make  pngtest pngunknown pngstest pngvalid pngimage pngcp timepng
make[3]: Entering directory '/<<PKGBUILDDIR>>'
make[3]: 'pngtest' is up to date.
make[3]: 'pngunknown' is up to date.
make[3]: 'pngstest' is up to date.
make[3]: 'pngvalid' is up to date.
make[3]: 'pngimage' is up to date.
make[3]: 'pngcp' is up to date.
make[3]: 'timepng' is up to date.
make[3]: Leaving directory '/<<PKGBUILDDIR>>'
make  check-TESTS
make[3]: Entering directory '/<<PKGBUILDDIR>>'
make[4]: Entering directory '/<<PKGBUILDDIR>>'
PASS: tests/pngtest
PASS: tests/pngtest-badpngs
PASS: tests/pngvalid-gamma-16-to-8
PASS: tests/pngvalid-gamma-background
PASS: tests/pngvalid-gamma-expand16-background
FAIL: tests/pngvalid-gamma-expand16-transform
PASS: tests/pngvalid-gamma-threshold
PASS: tests/pngvalid-gamma-alpha-mode
PASS: tests/pngvalid-gamma-transform
PASS: tests/pngvalid-gamma-expand16-alpha-mode
PASS: tests/pngvalid-progressive-interlace-standard
PASS: tests/pngvalid-progressive-standard
PASS: tests/pngvalid-gamma-sbit
PASS: tests/pngvalid-standard
PASS: tests/pngvalid-progressive-size
PASS: tests/pngvalid-transform
PASS: tests/pngstest-1.8-alpha
FAIL: tests/pngstest-linear
FAIL: tests/pngstest-1.8
PASS: tests/pngstest-linear-alpha
PASS: tests/pngstest-none-alpha
PASS: tests/pngunknown-IDAT
PASS: tests/pngunknown-discard
PASS: tests/pngunknown-if-safe
PASS: tests/pngunknown-sAPI
PASS: tests/pngunknown-sTER
PASS: tests/pngunknown-save
PASS: tests/pngunknown-vpAg
PASS: tests/pngimage-quick
FAIL: tests/pngstest-none
FAIL: tests/pngstest-sRGB
PASS: tests/pngstest-sRGB-alpha
PASS: tests/pngimage-full
=====================================
   libpng 1.6.36: ./test-suite.log
=====================================

# TOTAL: 33
# PASS:  28
# SKIP:  0
# XFAIL: 0
# FAIL:  5
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: tests/pngvalid-gamma-expand16-transform
=============================================

Gamma correction error summary

The printed value is the maximum error in the pixel values
calculated by the libpng gamma correction code.  The error
is calculated as the difference between the output pixel
value (always an integer) and the ideal value from the
libpng specification (typically not an integer).

Expect this value to be less than .5 for 8 bit formats,
less than 1 for formats with fewer than 8 bits and a small
number (typically less than 5) for the 16 bit formats.
For performance reasons the value for 16 bit formats
increases when the image file includes an sBIT chunk.

Basic gamma correction:
  2 bit gray:  127.07023
  4 bit gray:  127.07023
  8 bit gray:  128.32752
  8 bit color: 128.32752
  indexed:     18530.13919
 16 bit gray:  0.74080
 16 bit color: 0.74080
pngvalid: read: indexed-colour[0] 8 bit: gamma 0.455->1.000: red(131/255) ^2.20(gamma correction) = 15163.0 < 15138.5 (libpng: 33667)/65535 < 15163.0
pngvalid: 0 errors, 739 warnings
FAIL: pngvalid --strict --gamma-transform --expand16 (floating point arithmetic)
FAIL tests/pngvalid-gamma-expand16-transform (exit status: 1)

FAIL: tests/pngstest-1.8
========================

PASS: ./contrib/testpngs/gray-1-1.8-tRNS.png
PASS: ./contrib/testpngs/gray-1-1.8.png
PASS: ./contrib/testpngs/gray-16-1.8-tRNS.png
PASS: ./contrib/testpngs/gray-16-1.8.png
PASS: ./contrib/testpngs/gray-2-1.8-tRNS.png
PASS: ./contrib/testpngs/gray-2-1.8.png
PASS: ./contrib/testpngs/gray-4-1.8-tRNS.png
PASS: ./contrib/testpngs/gray-4-1.8.png
PASS: ./contrib/testpngs/gray-8-1.8-tRNS.png
PASS: ./contrib/testpngs/gray-8-1.8.png
PASS: ./contrib/testpngs/palette-1-1.8-tRNS.png
PASS: ./contrib/testpngs/palette-1-1.8.png
PASS: ./contrib/testpngs/palette-2-1.8-tRNS.png
PASS: ./contrib/testpngs/palette-2-1.8.png
PASS: ./contrib/testpngs/palette-4-1.8-tRNS.png
PASS: ./contrib/testpngs/palette-4-1.8.png
./contrib/testpngs/palette-8-1.8-tRNS.png(10,0) alpha component error:
 color-mapped-sRGB-rgb+alpha(78,78,255,220) on background sRGB-rgb(251,188,65) ->
       sRGB-rgb(47,47,255)
  not: sRGB-rgb(123,103,240).
 The error happened when reading the original file with this format.
FAIL: ./contrib/testpngs/palette-8-1.8-tRNS.png
PASS: ./contrib/testpngs/palette-8-1.8.png
PASS: ./contrib/testpngs/rgb-16-1.8-tRNS.png
PASS: ./contrib/testpngs/rgb-16-1.8.png
PASS: ./contrib/testpngs/rgb-8-1.8-tRNS.png
PASS: ./contrib/testpngs/rgb-8-1.8.png
FAIL tests/pngstest-1.8 (exit status: 1)

FAIL: tests/pngstest-linear
===========================

PASS: ./contrib/testpngs/gray-1-linear-tRNS.png
PASS: ./contrib/testpngs/gray-1-linear.png
PASS: ./contrib/testpngs/gray-16-linear-tRNS.png
PASS: ./contrib/testpngs/gray-16-linear.png
PASS: ./contrib/testpngs/gray-2-linear-tRNS.png
PASS: ./contrib/testpngs/gray-2-linear.png
PASS: ./contrib/testpngs/gray-4-linear-tRNS.png
PASS: ./contrib/testpngs/gray-4-linear.png
PASS: ./contrib/testpngs/gray-8-linear-tRNS.png
PASS: ./contrib/testpngs/gray-8-linear.png
PASS: ./contrib/testpngs/palette-1-linear-tRNS.png
PASS: ./contrib/testpngs/palette-1-linear.png
PASS: ./contrib/testpngs/palette-2-linear-tRNS.png
PASS: ./contrib/testpngs/palette-2-linear.png
PASS: ./contrib/testpngs/palette-4-linear-tRNS.png
PASS: ./contrib/testpngs/palette-4-linear.png
./contrib/testpngs/palette-8-linear-tRNS.png(5,0) alpha component error:
 color-mapped-sRGB-rgb+alpha(160,160,255,236) on background sRGB-rgb(251,188,65) ->
       sRGB-rgb(90,90,255)
  not: sRGB-rgb(169,162,247).
 The error happened when reading the original file with this format.
FAIL: ./contrib/testpngs/palette-8-linear-tRNS.png
PASS: ./contrib/testpngs/palette-8-linear.png
PASS: ./contrib/testpngs/rgb-16-linear-tRNS.png
PASS: ./contrib/testpngs/rgb-16-linear.png
PASS: ./contrib/testpngs/rgb-8-linear-tRNS.png
PASS: ./contrib/testpngs/rgb-8-linear.png
FAIL tests/pngstest-linear (exit status: 1)

FAIL: tests/pngstest-none
=========================

PASS: ./contrib/testpngs/gray-1-tRNS.png
PASS: ./contrib/testpngs/gray-1.png
PASS: ./contrib/testpngs/gray-16-tRNS.png
PASS: ./contrib/testpngs/gray-16.png
PASS: ./contrib/testpngs/gray-2-tRNS.png
PASS: ./contrib/testpngs/gray-2.png
PASS: ./contrib/testpngs/gray-4-tRNS.png
PASS: ./contrib/testpngs/gray-4.png
PASS: ./contrib/testpngs/gray-8-tRNS.png
PASS: ./contrib/testpngs/gray-8.png
PASS: ./contrib/testpngs/palette-1-tRNS.png
PASS: ./contrib/testpngs/palette-1.png
PASS: ./contrib/testpngs/palette-2-tRNS.png
PASS: ./contrib/testpngs/palette-2.png
PASS: ./contrib/testpngs/palette-4-tRNS.png
PASS: ./contrib/testpngs/palette-4.png
./contrib/testpngs/palette-8-tRNS.png(13,0) alpha component error:
 color-mapped-sRGB-rgb+alpha(32,32,255,210) on background sRGB-rgb(251,188,65) ->
       sRGB-rgb(32,32,255)
  not: sRGB-rgb(118,89,235).
 The error happened when reading the original file with this format.
FAIL: ./contrib/testpngs/palette-8-tRNS.png
PASS: ./contrib/testpngs/palette-8.png
PASS: ./contrib/testpngs/rgb-16-tRNS.png
PASS: ./contrib/testpngs/rgb-16.png
PASS: ./contrib/testpngs/rgb-8-tRNS.png
PASS: ./contrib/testpngs/rgb-8.png
FAIL tests/pngstest-none (exit status: 1)

FAIL: tests/pngstest-sRGB
=========================

PASS: ./contrib/testpngs/gray-1-sRGB-tRNS.png
PASS: ./contrib/testpngs/gray-1-sRGB.png
PASS: ./contrib/testpngs/gray-16-sRGB-tRNS.png
PASS: ./contrib/testpngs/gray-16-sRGB.png
PASS: ./contrib/testpngs/gray-2-sRGB-tRNS.png
PASS: ./contrib/testpngs/gray-2-sRGB.png
PASS: ./contrib/testpngs/gray-4-sRGB-tRNS.png
PASS: ./contrib/testpngs/gray-4-sRGB.png
PASS: ./contrib/testpngs/gray-8-sRGB-tRNS.png
PASS: ./contrib/testpngs/gray-8-sRGB.png
PASS: ./contrib/testpngs/palette-1-sRGB-tRNS.png
PASS: ./contrib/testpngs/palette-1-sRGB.png
PASS: ./contrib/testpngs/palette-2-sRGB-tRNS.png
PASS: ./contrib/testpngs/palette-2-sRGB.png
PASS: ./contrib/testpngs/palette-4-sRGB-tRNS.png
PASS: ./contrib/testpngs/palette-4-sRGB.png
./contrib/testpngs/palette-8-sRGB-tRNS.png(13,0) alpha component error:
 color-mapped-sRGB-rgb+alpha(32,32,255,210) on background sRGB-rgb(251,188,65) ->
       sRGB-rgb(32,32,255)
  not: sRGB-rgb(118,89,235).
 The error happened when reading the original file with this format.
FAIL: ./contrib/testpngs/palette-8-sRGB-tRNS.png
PASS: ./contrib/testpngs/palette-8-sRGB.png
PASS: ./contrib/testpngs/rgb-16-sRGB-tRNS.png
PASS: ./contrib/testpngs/rgb-16-sRGB.png
PASS: ./contrib/testpngs/rgb-8-sRGB-tRNS.png
PASS: ./contrib/testpngs/rgb-8-sRGB.png
FAIL tests/pngstest-sRGB (exit status: 1)

============================================================================
Testsuite summary for libpng 1.6.36
============================================================================
# TOTAL: 33
# PASS:  28
# SKIP:  0
# XFAIL: 0
# FAIL:  5
# XPASS: 0
# ERROR: 0
============================================================================
See ./test-suite.log
Please report to png-mng-implement@lists.sourceforge.net
============================================================================

[LocutusOfBorg]

right now, I uploaded the following three patches on top of clean 1.6.36:

cat debian/patches/272.patch (PR #272)

From d0c0f5f96bad2391f62511bf1c92be6ffeaaae41 Mon Sep 17 00:00:00 2001
From: Richard Townsend <Richard.Townsend@arm.com>
Date: Fri, 11 Jan 2019 16:55:51 +0000
Subject: [PATCH 2/2] Free png_structp members after use in gamma_test

Fixes #266
---
 contrib/libtests/pngvalid.c | 3 +++
 1 file changed, 3 insertions(+)

Index: libpng1.6/pngread.c
===================================================================
--- libpng1.6.orig/pngread.c
+++ libpng1.6/pngread.c
@@ -994,6 +994,12 @@
    png_ptr->chunk_list = NULL;
 #endif
 
+#if defined(PNG_READ_EXPAND_SUPPORTED) && \
+    defined(PNG_ARM_NEON_IMPLEMENTATION)
+   png_free(png_ptr, png_ptr->riffled_palette);
+   png_ptr->riffled_palette = NULL;
+#endif
+
    /* NOTE: the 'setjmp' buffer may still be allocated and the memory and error
     * callbacks are still set at this point.  They are required to complete the
     * destruction of the png_struct itself.
Index: libpng1.6/contrib/libtests/pngvalid.c
===================================================================
--- libpng1.6.orig/contrib/libtests/pngvalid.c
+++ libpng1.6/contrib/libtests/pngvalid.c
@@ -10472,6 +10472,9 @@
             d.this.ps->validated = 1;
       }
 
+      /* Clean up png_structp, without destroying the structure itself. */
+      png_destroy_read_struct(&(d.pm->this.pread), &pi, &pi);
+
       modifier_reset(d.pm);
 
       if (d.pm->log && !d.threshold_test && !d.this.speed)

commit: 8439534

and
cat debian/patches/CVE-2019-7317.patch

Origin: https://github.com/glennrp/libpng/issues/275
Author: Cosmin Truta
Description: Fix for CVE-2019-7317
diff --git a/png.c b/png.c
index 9d9926f638..efd1aecfbd 100644
--- a/png.c
+++ b/png.c
@@ -4588,8 +4588,7 @@ png_image_free(png_imagep image)
    if (image != NULL && image->opaque != NULL &&
       image->opaque->error_buf == NULL)
    {
-      /* Ignore errors here: */
-      (void)png_safe_execute(image, png_image_free_function, image);
+      png_image_free_function(image);
       image->opaque = NULL;
    }
 }

[LocutusOfBorg]

this way looks like everything builds correctly

[ctruta]

I've been finally able to debug what's going on, and I discovered that pngvalid does some funky things that were fine with the old libpng code, but not really fine with the new one.

As of commit 70d122a, the memory leak is fixed. The riffled palette buffer is allocated and initialized once, where is should be (i.e. inside png_init_palette_transformations), and deallocated once, where is should be (i.e. inside png_read_destroy). Everything should be fine.

When executed with certain parameters, pngvalid is modifying certain png_ptr attributes (e.g. png_ptr->num_palette and png_ptr->num_trans) without going again fully through the initialization phase. I am not 100% sure that this is correct API-wise, though.

An easy fix would have been to leave the riffled palette buffer allocation as it's done in 70d122a, inside png_init_palette_transformations, but move its initialization back to its original spot, inside png_do_read_transformations. I've tested it on an ARM64 machine, and confirmed that it works. The work is not done yet, however, because, aside from the not-so-nice decoupling of allocation and initialization, a performance issue may now be introduced.

Is it a significant issue? I don't know yet. I'm still looking for the better fix.

Here is the interim fix, applicable to commit 70d122a:

diff --git a/pngrtran.c b/pngrtran.c
index 3294340912..35a867e840 100644
--- a/pngrtran.c
+++ b/pngrtran.c
@@ -1163,14 +1163,11 @@ png_init_palette_transformations(png_structrp png_ptr)

 #ifdef PNG_READ_EXPAND_SUPPORTED
 #ifdef PNG_ARM_NEON_INTRINSICS_AVAILABLE
-   /* Initialize the accelerated palette expansion, if applicable. */
    if ((png_ptr->transformations & PNG_EXPAND) != 0)
    {
+      /* Allocate the accelerated palette expansion buffer, if applicable. */
       if ((png_ptr->num_trans > 0) && (png_ptr->bit_depth == 8))
-      {
          png_ptr->riffled_palette = (png_bytep)png_malloc(png_ptr, 256 * 4);
-         png_riffle_palette_rgba8(png_ptr);
-      }
    }
 #endif /* PNG_ARM_NEON_INTRINSICS_AVAILABLE */

@@ -4785,6 +4782,11 @@ png_do_read_transformations(png_structrp png_ptr, png_row_infop row_info)
    {
       if (row_info->color_type == PNG_COLOR_TYPE_PALETTE)
       {
+#ifdef PNG_ARM_NEON_INTRINSICS_AVAILABLE
+         /* Initialize the accelerated palette expansion buffer. */
+         if (png_ptr->riffled_palette)
+            png_riffle_palette_rgba8(png_ptr);
+#endif
          png_do_expand_palette(png_ptr, row_info, png_ptr->row_buf + 1,
              png_ptr->palette, png_ptr->trans_alpha, png_ptr->num_trans);
       }

[ctruta]

Quick update:

Is it a significant issue? I don't know yet. I'm still looking for the better fix.

Of course it is significant. png_do_read_transformations is executed for every row. The original 1.6.36 code checks for "is it allocated yet?", also at every row, and does nothing unless it's the first allocation. The original check is not a free check, but it's cheap. The new one isn't.

The above patch is good enough for a proof-of-concept correctness fix, but otherwise it's unacceptable, because it will ruin the performance. A proper fix is clearly needed.

[ctruta]

I pushed the fix. "It Works For Me."

Could you please give it a respin, @LocutusOfBorg?

[LocutusOfBorg]

it looks like working, thanks!