bug: png_check_sig API changed in 1.6.41

[mayeut]

Commit 27e548af2518ff8d278b45c40d11ad1bdd68eaa0 changed the semantics of png_check_sig

Before that commit, it returned a "true" value when signature was valid. After that commit, it returns a "true" value when signature is invalid.

[ctruta]

If you mean this

/* Simple signature checking function.  This is the same as calling
 * png_check_sig(sig, n) := !png_sig_cmp(sig, 0, n).
 * png_check_sig(sig, n) := (png_sig_cmp(sig, 0, n) != 0).
 */
#define png_check_sig(sig, n) !png_sig_cmp((sig), 0, (n))
#define png_check_sig(sig, n) (png_sig_cmp((sig), 0, (n)) != 0)

then see the replacement of the negation operator with the not-equal operator ;-)

[mayeut]

it should be #define png_check_sig(sig, n) (png_sig_cmp((sig), 0, (n)) == 0)

If the signature is valid: before the commit the result was !0 == true after the commit 0 != 0==false

[ctruta]

D'oh!! [Insert Homer Simpson picture here]

"Given enough eyeballs, all bugs are shallow."

This calls for an immediate release of libpng 1.6.42.

Thank you very much for reporting, Mattheu; and even more importantly, thank you for insisting!

[jbowler]

This calls for an immediate release of libpng 1.6.42.

With a validation (regression) check in pngtest.c, or elsewhere. I thought there was one but maybe not.

[jbowler]

I'll try to notify the gentoo maintainers later today, it's probably been seen already.

[ctruta]

This calls for an immediate release of libpng 1.6.42.

With a validation (regression) check in pngtest.c, or elsewhere. I thought there was one but maybe not.

One thing we always needed (and never had) were unit tests that actually satisfy the definition of unit tests (in the formal sense). Although I'm not against adding something to pngtest.c, I still think that program has absolutely surpassed its initial purpose. (In my personal opinion.)

As I already wrote in ac944e2b364cff96e8458110c2ad06a63f8543b3, png_check_sig was deprecated eons ago, like, somewhere in the mid-90's.

I'm getting my GPG signature set up (because I still don't have one yet, and I should have one...) and then I'll push a release ASAP. The test can be landed either as the last thing in 1.6.42 or as the first thing in 1.6.43.git.

[jbowler]

The gentoo bug is here:

https://bugs.gentoo.org/923298

[jbowler]

It is worth noting that a change like this is an ABI change; it causes an app or library to assume a different ABI. In general function-like macros in png.h have this problem and, so far as unit testing is concerned the approach is to ensure that each API, whether a function-like macro or a real function, doesn't change its net behavior. It's not hard but it is a lot of typing.

[ctruta]

Thank you very much again @mayeut.

Going forward, if no present-day program uses this decades-long-obsolete pseudo-function png_check_sig(), then we can say that nothing is broken, period. An interesting experiment would be to comment out its declaration and to see if doing so would break the compilation in a build like Gentoo Linux, for example. If the experiment is passing, then we can remove the declaration altogether, once and for all, from the libpng-next-generation headers.

[jbowler]

Yes, but it's only one of many unused or hardly used APIs. I'm tempted to just do an appropriate minconfig build and see what breaks. The gentoo libpng maintainer might be prepared to offer some help; package maintainers tend to have systems with the dependent packages on them. I used to do this when I was actively maintaining libpng, most of the time it isn't too much effort to rebuild all dependent packages.

Knowing what is used allows the scope of a "compatibility" library to be assessed. That would allow another library to be used as the implementation so libpng is just the compatibility wrapper. libspng has been suggested (https://github.com/randy408/libspng) and is currently maintained (APNG support is 'in progress'), the Rust library has APNG support.

[thesamesam]

I'm happy to help with such things in general, FWIW, but John is also right that we may not be able to catch everything that way either.

[ctruta]

@jbowler wrote:

Yes, but it's only one of many unused or hardly used APIs.

No argument from me here. My very specific point was about this very specific bug in png_check_sig: how to check (easily) if this libpng-1.6.41 regression has truly affected any application at all.

@thesamesam my hope is that nothing should break in the build process (...of an entire distro!) if the definition of png_check_sig is commented out in png.h. Think about it: assuming that we do remove this API, which had been deprecated before Y2K, and if doing so is still not a smooth sail today, that should give us an indication of what an uphill battle is ahead of us when it comes to doing janitorial work on the libpng API.

[jbowler]

Ok. @thesamesam I did this: SIGEXIF.PATCH In /etc/portage/patches/media-libs/libpng. The EXIF stuff is for another PR which I'm testing at the same time. Then:

emerge --oneshot media-libs/libpng
emerge --oneshot $( equery d media-libs/libpng | awk '{print "="$1}' )

That builds 34 packages on my system. I don't expect the EXIF stuff to break anything. The full list is at the end and all built successfully. This won't catch packages that depend on a library which uses libpng without depending directly on libpng and which also include png.h (directly or indirectly).

It's fairly fast apart from opencv and qtwebengine (no surprise there!) There's no Chromium there because I build -apng. There are also several copies of libpng.so in my Python -menv site packages; these are being downloaded not built locally. Python and, maybe, other interpreters pose a potential problem because they can expose the libpng API to the script. IRC the Python implementation doesn't but it's clearly possible.

PACKAGE LIST: =app-text/ghostscript-gpl-10.02.1-r1 =app-text/poppler-24.01.0 =dev-python/pygame-2.5.2 =dev-qt/qt-docs-5.15.2_p202011130614 =dev-qt/qt-docs-6.6.1_p202311210527 =dev-qt/qtbase-6.6.1-r4 =dev-qt/qtgui-5.15.12 =dev-qt/qtwebengine-5.15.12_p20240122 =dev-util/android-studio-2023.1.1.22 =gui-libs/gtk-4.12.5 =kde-apps/gwenview-23.08.4 =kde-frameworks/khtml-5.114.0 =media-gfx/optipng-0.7.8 =media-gfx/pngcrush-1.8.13 =media-gfx/qrencode-4.1.1 =media-libs/freetype-2.13.2 =media-libs/gd-2.3.3-r4 =media-libs/gst-plugins-base-1.22.3 =media-libs/jbig2dec-0.19 =media-libs/libass-0.17.1 =media-libs/libtheora-1.1.1-r2 =media-libs/libwebp-1.3.2 =media-libs/netpbm-11.5.2 =media-libs/opencv-4.8.1-r1 =media-libs/openjpeg-2.5.0-r6 =media-libs/sdl2-image-2.6.3-r1 =media-plugins/gst-plugins-libpng-1.22.3 =media-video/guvcview-2.0.8 =media-video/vlc-3.0.20-r3 =net-libs/libvncserver-0.9.14-r2 =sys-libs/slang-2.3.3-r1 =x11-libs/cairo-1.18.0 =x11-libs/gdk-pixbuf-2.42.10-r1 =x11-libs/wxGTK-3.2.2.1-r3