Skip to content

Update libpng to version1.6.37 and zlib to version 1.2.11 - #2410

Merged
davisking merged 7 commits into
davisking:masterfrom
arrufat:libpng-1.6.37
Aug 5, 2021
Merged

Update libpng to version1.6.37 and zlib to version 1.2.11#2410
davisking merged 7 commits into
davisking:masterfrom
arrufat:libpng-1.6.37

Conversation

@arrufat

@arrufatarrufat commented Aug 5, 2021

Copy link
Copy Markdown
Contributor

There have been several vulnerability fixes in libpng, so I decided to update it, and while at it, also zlib.

I updated the files and cherry-picked all extra commits that fixed warnings (notably on GCC and MSVC).

I pushed every change in a separate commit (libpng, zlib, and each fix) so that it's easy to keep track later on. It certainly helped me a lot to find those changes. So maybe, exceptionally, we shouldn't squash this PR before merging into master. UPDATE: maybe it's not needed, since we could always refer to this PR.

Please don't hesitate to close if you don't think this is needed.

PS: I'd like to update the Pybind11 at some point too, but that seems a bit more complicated/time-consuming...

@pfeatherstone

Copy link
Copy Markdown
Contributor

Here is a suggestion: how about embed libpng, libjpeg and libz in dlib using the same trick Davis suggested for kissFFT. I.e, rename the .c files to .cpp files and put everything in the dlib namespace then those libraries are no longer dependencies, you can update them freely and won't conflict with user code if they are linking to libpng and co themselves.

@arrufat

arrufat commented Aug 5, 2021

Copy link
Copy Markdown
ContributorAuthor

Ah, maybe I need to try that, it seems like it's not being able to build on Travis...
EDIT: it seems a bit annoying, though... :P

@pfeatherstone

Copy link
Copy Markdown
Contributor

@davisking will probs have an opinion on this.
These days, disk space is cheap, so having duplicate libraries or building a few extra C files (well in this case refactored C files) isn't an issue.

@pfeatherstone

Copy link
Copy Markdown
Contributor

Those libraries might have fortran code though....

@arrufat

Copy link
Copy Markdown
ContributorAuthor

I had to run ./configure to update the apparently half-baked pnglibconf.h.

@davisking

Copy link
Copy Markdown
Owner

Here is a suggestion: how about embed libpng, libjpeg and libz in dlib using the same trick Davis suggested for kissFFT. I.e, rename the .c files to .cpp files and put everything in the dlib namespace then those libraries are no longer dependencies, you can update them freely and won't conflict with user code if they are linking to libpng and co themselves.

Na I really don't want to do that. People should link to the proper system installs of libpng. I've only got these here in external because so many Windows users are like "what's linking/what's a system version of a library?"

The stuff in external is just a fallback for people who don't have a working libpng cmake can find.

And like, I say this specifically for the reason this stuff here needs to be updated. People who use the normal method of linking to the system libpng will always be appropriately updated.

Kissfft is special. It's ultra simple and also doesn't have a stable ABI like libpng. So "go link to the system kissfft" isn't a thing anyone would want or care about.

@davisking

davisking commented Aug 5, 2021

Copy link
Copy Markdown
Owner

Like if I had my way dlib wouldn't include copies of these libraries. They always get out of date so people shouldn't be using them. But I added them after like O(a billion) people kept complaining to me that they didn't know how to link to libpng. Or how to install libpng.

@pfeatherstone

Copy link
Copy Markdown
Contributor

Fair enough

@arrufat

Copy link
Copy Markdown
ContributorAuthor

Ah, so let's not touch this then. I guess we should not encourage using this.

@arrufatarrufat closed this Aug 5, 2021
@davisking

Copy link
Copy Markdown
Owner

Ha no this PR is good. These things should be updated periodically for the reasons you mentioned. Like people do use this :)

I'm just saying I wish they didn't. But it's here and it really should be updated every now and then.

The other reason to not change the code and dlib/c++ify it is that doing that makes updating it hard. Right now you should be able to update it by mostly just copying new files over the old ones.

@arrufatarrufat reopened this Aug 5, 2021
@arrufat

Copy link
Copy Markdown
ContributorAuthor

Understood. Yes, that's what I did, I just updated the files.

@davisking

Copy link
Copy Markdown
Owner

Understood. Yes, that's what I did, I just updated the files.

Yeah, looks good. I'll merge when the tests all pass :) There aren't any other changes you think you need to add first?

And to be clear, my objection to this stuff being in dlib is of the same flavor as saying that hospitals shouldn't exist. The world would be way better if people just didn't get injured or sick. But the world is what it is and we need hospitals. And we need this stuff in external/ too. No getting away from it.

But It's good that dlib depend on the normal public APIs of libraries like libpng/libjpeg/etc. This way people who want to (and most people should want to but some just can't for various reasons, some of which are their own fault) can use dlib with the normal versions of these libraries. Either the system installed ones or their own builds they have in their own environment, whatever that is. I know plenty of people who would be upset if dlib started to always build in its own copy of libpng. Since they would then be unable to just update libpng when there are security vulnerabilities and get their whole software system fixed.

@arrufat

arrufat commented Aug 5, 2021

Copy link
Copy Markdown
ContributorAuthor

Ha, nice analogy. I completely understand it.
Actually, I've always used dlib with the systems' libpng, so this PR is not useful to me. I got somehow curious about libpng and remembered that dlib had one version in external. I checked, and it was quite old, so I decided to update it.

I think I don't want to add anything else to this PR, I just wanted to update the version of libpng and its dependency :)

@davisking
davisking merged commit cd915b0 into davisking:masterAug 5, 2021
@davisking

Copy link
Copy Markdown
Owner

I will admit to using the external ones sometimes :) Only on windows though where it's a pain to do otherwise though.

@arrufat

Copy link
Copy Markdown
ContributorAuthor

Ah, I've never had the pleasure of working with dlib on Windows 😅

Sign up for freeto 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.

3 participants

@arrufat@pfeatherstone@davisking