Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not build unused Adler32 code #514

Closed
wants to merge 1 commit into from
Closed

Do not build unused Adler32 code #514

wants to merge 1 commit into from

Conversation

jbowler
Copy link
Contributor

@jbowler jbowler commented Dec 30, 2023

This removes the default build of an undocumented feature to disable Adler32 checksums on those systems where it was the default.

The PR is motived by github #187 however it fixes a much more general problem (#187 is limited to an issue where libpng "crashes" on some manufacturer systems). The fix is based on a suggestion by @sgowdev who is the originator of the issue.

When libpng disables the checking of Adler32 checksums it does so by an undocumented and therefore possibly unsupported call to a zlib function which does not exist in some versions of zlib.

Fortunately libpng only does this if the caller of libpng explicitly asks for it to happen. Unfortunately the call to the undocumented function is still in the compiled and built libpng and this means that on some systems (as identified in #187) libpng can fail to load or maybe even crash.

The libpng authors are currently unaware of any program or system that uses this feature and none has been identified by the contributors to #187.

In this fix an option is added to enable the code so that by default the code is disabled - this is a simple generalization of the suggestion by @sgowdev.

BENEFITS: the problem is eliminated, users of the functionality, if any, are idenfified, the functionality can be implemented correctly in the future or it can be removed. Hardly anyone complains.

COSTS: someone will complain that they have to enable an option in a libpng build to use a feature that never worked consistently in the first place.

This patch has been tested both with the option enabled and with it disabled via pngusr.dfa. Tests, checks pass with cmake and configure, make distcheck passes on configure.

@jbowler
Copy link
Contributor Author

jbowler commented Dec 30, 2023

Tested with and without the code enabled i.e. with and without:

option DISABLE_ADLER32_CHECK on

in pngusr.dfa

cmake all; cmake test
configure; make all; make check; make clean; make distcheck

This is the only one of all the solutions that I am happy with. I'd go for the "delete it all" approach too if necessary, but the rest of them are a bug farm.

This removes the default build of an undocumented feature to disable
Adler32 checksums on those systems where it was the default.

The PR is motived by github #187 however it fixes a much more general
problem (#187 is limited to an issue where libpng "crashes" on some
manufacturer systems).  The fix is based on a suggestion by @sgowdev who is the
originator of the issue.

When libpng disables the checking of Adler32 checksums it does so by an
undocumented and therefore possibly unsupported call to a zlib function
which does not exist in some versions of zlib.

Fortunately libpng only does this if the caller of libpng explicitly
asks for it to happen.  Unfortunately the call to the undocumented
function is still in the compiled and built libpng and this means that
on some systems (as identified in #187) libpng can fail to load or maybe
even crash.

The libpng authors are currently unaware of any program or system that
uses this feature and none has been identified by the contributors to #187.

In this fix an option is added to *enable* the code so that by default
the code is *disabled* - this is a simple generalization of the
suggestion by @sgowdev.

BENEFITS: the problem is eliminated, users of the functionality, if any,
are idenfified, the functionality can be implemented correctly in the
future or it can be removed.  Hardly anyone complains.

COSTS: someone will complain that they have to enable an option in a
libpng build to use a feature that never worked consistently in the
first place.

This patch has been tested both with the option enabled and with it
disabled via pngusr.dfa.  Tests, checks pass with cmake and configure,
make distcheck passes on configure.

Signed-off-by: John Bowler <jbowler@acm.org>
@jbowler jbowler closed this by deleting the head repository Jan 18, 2024
@0-wiz-0
Copy link

0-wiz-0 commented Feb 1, 2024

I found one: pngcrush:

pngcrush.c: In function 'main':
pngcrush.c:5523:46: error: 'PNG_IGNORE_ADLER32' undeclared (first use in this function); did you mean 'PNG_NORETURN'?
                     png_set_option(read_ptr, PNG_IGNORE_ADLER32,
                                              ^~~~~~~~~~~~~~~~~~
                                              PNG_NORETURN
pngcrush.c:5523:46: note: each undeclared identifier is reported only once for each function it appears in
*** Error code 1

@jbowler
Copy link
Contributor Author

jbowler commented Feb 1, 2024

Yeah, well it was written by Glenn :-) If he was the only person to ever use it we are on pretty safe ground.

glennrp/pmt#2

The same fix has already been made on gentoo, it's probably better to take that one if the maintainer pushed it upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants