Giter VIP home page Giter VIP logo

Comments (22)

jbowler avatar jbowler commented on September 26, 2024 1

I don't have an example, sorry.

That's fine and thank you for the response. I have a fix which I will push but it requires testing (since I don't have the required systems). It would still be good to have a crash example; I think I know how to repro it but it requires a programming error in the app. My fix is safe and minimalist, I will encourage @ctruta to request a review and testing from those who have the required build systems.

from libpng.

glennrp avatar glennrp commented on September 26, 2024

This should be sufficient

ZLIB_VERNUM >= 0x1290

The definition is in zlib.,h

If that doesn't work, it's a sign that you are building with a recent zlib.h but then linking with an older zlib,.

from libpng.

sgowdev avatar sgowdev commented on September 26, 2024

Changing zlib.h isn't an option for me, since it is defined in the macos and ios sdks.

So basically, libpng is making the assumption that it can call inflateValidate purely because ZLIB_VERNUM >= 0x1290, but Xcode is telling me that 'inflateValidate' is only available on macOS 10.13 or newer.

Yeah so the real issue is that I am compiling against mac os 10.13, but want to support 10.10 and up. If I change my deployment target to 10.13, the issue goes away.

Basically it would be nice to have another flag instead of purely relying on ZLIB_VERNUM, because I want to support mac os 10.10 and up but still compile against the latest mac os SDK (as I am forced to by Apple).

So in short, doing this fixes my issue (while allowing me to keep my deployment target set to 10.10):

if (__builtin_available(macOS 10.13, *))
{
     ret = inflateValidate(&png_ptr->zstream, 0);
}

from libpng.

sgowdev avatar sgowdev commented on September 26, 2024

I've made the above change in my fork of libpng, which is totally fine.

I just prefer to use the official repo unchanged wherever I can.

from libpng.

sgowdev avatar sgowdev commented on September 26, 2024

Actually, now that I am thinking about it more, a better change (for my purposes) is to just undefine PNG_IGNORE_ADLER32, that way all my platforms are affected equally and not just mac.

But I'm still curious if you've ran into this issue before and what you might've done to resolve it.

from libpng.

sgowdev avatar sgowdev commented on September 26, 2024

@glennrp sorry didn't see your edit, you are exactly right. So, since I am going to continue to use the zlib embedded into mac os and iOS, undefining PNG_IGNORE_ADLER32 seems to be my best solution. I'll close this issue now.

from libpng.

mrserb avatar mrserb commented on September 26, 2024

@glennrp Hello, I am faced the same issue while working on the OpenJDK 12 project.
The problem is that there is a call to "inflateValidate" function in pngrutil.c, guarded by a preprocessor check of ZLIB_VERNUM being high enough and by the "PNG_IGNORE_ADLER32". If we compile this call in and link to the newer version of zlib, we will get link errors if the code is executed on an older Mac/Solaris with an older version of zlib.

I understand that it is possible to fix it by the change of “ZLIB_VERNUM” or “PNG_IGNORE_ADLER32” but both are not the “official” options which usually defined in "pnglibconf.h"

So maybe it is possible to provide an official option like "PNG_ADLER32_SUPPORTED" in "pnglibconf.h"?

from libpng.

sgowdev avatar sgowdev commented on September 26, 2024

@mrserb this issue has been closed, not sure if @glennrp will see your comment, but now I just tagged him, so that should fix that.

from libpng.

ctruta avatar ctruta commented on September 26, 2024

I am reopening this issue.

The tip-of-tree is in near-release stage, so I will concentrate on releasing 1.6.36 first. I will come back to this issue after that.

from libpng.

vojtechkral avatar vojtechkral commented on September 26, 2024

Hi, I also just noticed this issue. Checking ZLIB_VERNUM is not enough in situation where one builds on a recent Mac OS but wants to target older ones with older SDK (and thereby older zlib).

I'm a bit confused as to what build system libpng uses, I can see both autotools and CMake in the root source dir. In case CMake is used, you can leverage the CMAKE_OSX_DEPLOYMENT_TARGET property to figure out that inflateValidate() can't be used.
Edit: You can probably just use the macro AVAILABLE_MAC_OS_X_VERSION_10_13_AND_LATER. I believe the inflateValidate() is available since 10.13, but don't take my word for it...

from libpng.

vadz avatar vadz commented on September 26, 2024

FWIW I think the fix from wxWidgets#5 linked above is the best solution to this problem and should be integrated in libpng itself.

from libpng.

jbowler avatar jbowler commented on September 26, 2024

This seems to me to be an Apple bug. Apple systems use multi-lib builds which compile single executables for use with multiple ABIs. This is handled in libpng by not configuring features at the build level but rather the compile level. The Apple build system builds the same code multiple times and by some magic compiles the different results and does a switch at run time.

Apparently in this case the Apple build system is compiling libpng with #defines from a single version of zlib. Either it needs to be changed to use a highest-common-factor version of zlib or it needs to not do that.

It's an Apple bug.

from libpng.

vadz avatar vadz commented on September 26, 2024

This seems to me to be an Apple bug. Apple systems use multi-lib builds which compile single executables for use with multiple ABIs. This is handled in libpng by not configuring features at the build level but rather the compile level. The Apple build system builds the same code multiple times and by some magic compiles the different results and does a switch at run time.

Sorry, but there is no magic here and it has absolutely nothing to do with universal builds (a.k.a. "building the same code multiple times"). There is a run-time check for the OS version, but this is hardly magic. Or, rather, there should be a run-time check for it, but currently there is none.

The problem is that there are 2 different macOS API versions: the one available at compile-time, determined by the SDK being used, and the one available at run-time. libpng assumes they are the same which is often (and I would even say almost always) not the case.

Apparently in this case the Apple build system is compiling libpng with #defines from a single version of zlib. Either it needs to be changed to use a highest-common-factor version of zlib or it needs to not do that.

It's an Apple bug.

This is absolutely not the case, unless you just consider the existence of macOS and its development environment as a bug (which might be true, in some sense of the word, but not very helpful).

Anyhow, as I already wrote almost exactly 3 years ago, there is a relatively simple fix available for this problem and I don't see what prevents libpng from integrating it. If anybody has any questions about it, I'd be glad to answer them but otherwise I don't really know what can we do to help.

from libpng.

jbowler avatar jbowler commented on September 26, 2024

I think this happened somewhere else too. Maybe it was this. Since it isn't possible to tell which version of zlib is available without an OS specific check the suggested change is to use both an os-specific version check (which seems to be checking for a lot more than zlib) along with, implicitly, a system that does delayed load (so the symbol resolve only happens on the first call).

The result is Apple specific code which requires a complex dev system, a specific companies computers and, IRC, an Apple Developer subscription (free but testing may require either a paid subscription or an Apple dev licence).

The other version of the fix seems more complete (it has #defines so that other builds aren't broken) but it's still unmaintainable except by someone with the appropriate Apple support (computer, dev subscription). The #defines should also be in pngpriv.h.

This is one of those cases that is best fixed by an Apple specific downstream of zlib, one maintained by people with Apple knowledge and that is precisely what https://github.com/wxWidgets/libpng is (it has 19 other commits in addition to this one.) So my opinion is that this is the best approach.

from libpng.

vadz avatar vadz commented on September 26, 2024

Sorry, I'm completely lost here. From what I understand, you seem to be arguing that libpng shouldn't support macOS at all, which seems like a very strange position to me (and I'm saying this as a certified Apple hater). If this is indeed the case, I really can't add much to this discussion and you should just close all Mac-specific issues to save time.

If, however, I'm mistaken -- as I sincerely hope to be -- then I can reiterate that IMHO it would make a lot more sense to carry this patch in the upstream libpng instead of having it in wxWidgets fork only and I'd be glad to help with integrating it if I can, including addressing any comments about it (e.g. moving this stuff to pngpriv.h).

from libpng.

jbowler avatar jbowler commented on September 26, 2024

libpng shouldn't support macOS at all

libpng doesn't support MacOS: this bug has been sitting here since Nov 07, 2017, @ctruta's last comment was on Nov 14, 2018. Meanwhile the solution I suggest has been implemented and people are using it; issue solved. As I also pointed out the fork has a lot more fixes that this one.

I guess libpng could just had a "PNG_ZLIB_MAX_VERSION" parameter (setting). That strikes me as safe; it doesn't require extensive testing and, anyway, I assume most people are using the fork(s).

I also suggest that for maintainability the libpng source code should contain ONLY ANSI-C89 standard functionality and calls to zlib. This issue has already been raised, and fixed, in a different context where the MIPS code included <stdint.h>.

Given that we may be at the point of starting a new major release it's tempting to abstract out all the zlib calls; they are inconvenient because some systems may chose to use 'reduced' versions of zlib which don't offer the same API (e.g. derived from puff.c). This could be done in a major release by putting everything into, say "pngzlib.c" and that file could then be replaced on a per-system basis (like hardware optimizatons). Of course that's dev work so requires resourcing but testing is relatively easy (no special equipment requirements).

from libpng.

vadz avatar vadz commented on September 26, 2024

libpng doesn't support MacOS

If this is serious, then http://www.libpng.org/pub/png/libpng.html should be updated not to mention it as one of supported platforms.

this bug has been sitting here since Nov 07, 2017,

But it's hard to shake a feeling that it isn't serious because there is, of course, a huge differences between having a platform-specific bug (which could be easily fixed) and not supporting a platform at all.

@ctruta's last comment was on Nov 14, 2018. Meanwhile the solution I suggest has been implemented and people are using it; issue solved. As I also pointed out the fork has a lot more fixes that this one.

No, it doesn't (just in case it's not obvious, I'm the maintainer of this "fork"). 5 of the commits there are related to this problem, the rest are (not macOS-specific) warning fixes (at least some of which I had already submitted here and I should probably do the same for the rest).

I guess libpng could just had a "PNG_ZLIB_MAX_VERSION" parameter (setting). That strikes me as safe; it doesn't require extensive testing and, anyway, I assume most people are using the fork(s).

I don't have any numbers to prove it, but I'm pretty sure that practically nobody uses our "fork" which is not even a fork in the real sense of the word because the main reason we have it in the first place is to be able to use relative paths in the main repository submodules and it's only used by wxWidgets internally. And I'm not aware of any other, more popular, forks.

I also suggest that for maintainability the libpng source code should contain ONLY ANSI-C89 standard functionality and calls to zlib. This issue has already been raised, and fixed, in a different context where the MIPS code included <stdint.h>.

It's not realistic to restrict yourself to 100% portable ANSI C if you want to support more than a few very common platforms. But it's your choice, of course.

Anyhow, if there is nothing that can be done to help with this issue, it should be closed and, ideally, it should be mentioned somewhere that libpng requires macOS 10.13 (which is not a big deal by now, of course, unlike in 2017).

from libpng.

jbowler avatar jbowler commented on September 26, 2024

@sgowdev, @vadz, @ctruta

This bug is just about a warning message generated during the build, isn't it? There's absolutely no problem in the code generated with the currently libpng 1.6 source. Glenn changed the check to fix what seems to be a bug in Solaris (distribution of a version of zlib which, I think, was called 1.2.8.f and which appeared to be >=1.2.8.1 but wasn't - Glenn's comment is confusing.)

The call has to be explicitly turned on by the app and apps should not be turning it on normally; that's only done to allow dangerously invalid PNG files. The code that is executed is not changed by the wxWidgets patch unless the app explicitly turns off Adler32 checking with png_set_option and, even then, what happens?

from libpng.

vadz avatar vadz commented on September 26, 2024

This bug is just about a warning message generated during the build, isn't it?

No.

This is a warning message indicating that the program won't run under macOS < 10.13 as it uses a function only available since this version.

The call has to be explicitly turned on by the app and apps should not be turning it on normally; that's only done to allow dangerously invalid PNG files. The code that is executed is not changed by the wxWidgets patch unless the app explicitly turns off Adler32 checking with png_set_option and, even then, what happens?

If this code is executed under macOS < 10.13 it will crash.

from libpng.

jbowler avatar jbowler commented on September 26, 2024

If this code is executed under macOS < 10.13 it will crash.

Please give me an example of a specific app; this is required for testing.

from libpng.

vadz avatar vadz commented on September 26, 2024

I don't have an example, sorry. I think someone did run into this in practice, but this was originally reported N years ago and I don't remember details any longer.

Of course, if this code can't be executed at all, it should be removed...

from libpng.

jbowler avatar jbowler commented on September 26, 2024

Here it is:

#512

Please test.

from libpng.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.