Sync gd 2 4#22532
Conversation
…ns (90% rewrite). update reference images (buggy before) for tests
…ns usuable, using linear RGBA instead of previous weird squared distance
…p's config. also move it to the right place
|
I don't have time for a deep review but I asked Volker to do a proper AI audit of the added code. We should do at least that and make sure there are no major finding in the added code. Even if the issue is in the libgd , then we shouldn't merge this until this if fixed there or on this PR. The pain with bundled code is that we still have to deal with security issue and bugs in it so ideally we want to limit it. |
|
@bukka yeah |
|
@bukka btw, as it seems you have been added as reviewer because of main/php_compat.h being touch, I wonder if they could be moved to the extension folder instead. besides it is buggy now, gd is inside xml ifdef and some symbols seem not to exist (will double check those). It can be separate PR. If can ease maintenance of it, maybe generate it, as declaration macros are needed too for visibility. thoughts? |
|
@edorian hello :) Not sure what you use, upstream has a larger tests suite, including codecs corpus on demand (conformance tests), not included here (and won't be) as the assets are 200M. I have the full fuzzing too but need to clean it a bit before I enable in libgd's CI. Maybe that helps too |
|
for reference, slowly drafting the RFC here |
Hey 👋, very long time no see.
Great input. I've added that to my workflows. I should be able to get some results to you and bukka later today. |
edorian
left a comment
There was a problem hiding this comment.
Hi again,
Quoting @bukka :
I don't have time for a deep review but I asked Volker to do a proper AI audit of the added code. We should do at least that and make sure there are no major finding in the added code.
I've done what I think makes sense in terms of AI review. The expected findings first:
- The sync is faithful and backdoor-free. Bundled
ext/gd/libgdmatches upstream libgd master byte-for-byte (verified two ways: file-diff + an 89-agent classification); font tables are numerically identical; only 2 non-libgd files change (CODEOWNERS, php_compat.h); all 66 commits are on-topic (nothing touches files outside ext/gd + those 2). - No prior security patch was dropped. We checked the 30 highest-value CVE-class libgd fixes (2016–2026) — all preserved (0 dropped). This is the main risk of a wholesale re-vendor, and it did not happen.
- Under ASan/UBSan: upstream's full 246-test suite passes clean; PHP's ext/gd
.phptsuite is 310/320 on a default build with 0 sanitizer hits; corpus + fuzz found no new memory-corruption bugs beyond the two below.
I've included 5 comments on findings made along the way
Even if the issue is in the libgd , then we shouldn't merge this until this if fixed there or on this PR.
There are three more (low to medium) findings on libgd that I didn't include here that might make sense to look at in private first?
I'll send you two a mail with the details.
I've done a brief manual validation, but you're way more knowledgeable on this than I am. If a finding is invalid, just say so and close the comment.
|
@edorian I added the initial (relatively) small fuzzing, you can see the workflow in https://github.com/libgd/libgd/blob/master/.github/workflows/fuzz-codecs.yml docs to add more here https://github.com/libgd/libgd/blob/master/tests/fuzzing/README.md, and existing ones in the same tests folder |
This reverts commit add1983. wrong changes. only pkg config needed
|
Great. All public feedback processed. Let me know if there is anything I can help with still @bukka. You're also on /cc for the other discussion. So 👍 from my side so far |
TimWolla
left a comment
There was a problem hiding this comment.
please refrain yourself to slip in changes not related to the topic at hand. Adding you as code owner could be done in a separate PR.
@pierrejoye I agree with David here. Can you commit the CODEOWNER change (and any other change that is not purely related to the GD sync) separately, please?
reverted. |
|
Thanks. As for the sync itself: Is it possible to automate the sync with a script similar to https://github.com/php/php-src/blob/master/ext/lexbor/patches/update-lexbor.sh? This would make verification and future synchronization much easier. The sync should then also happen in a single commit, since individual commits for individual components will result in an inconsistent state for intermediate commits. |
| /* Keep PHP's historical PNG warning text while bundled gd_png.c stays | ||
| * identical to upstream libgd. */ | ||
| if (strcmp(format, "gd-png: fatal libpng error: %s\n") == 0) { | ||
| format = "gd-png: fatal libpng error: %s"; | ||
| } else if (strncmp(format, "gd-png error: setjmp returns error condition", | ||
| sizeof("gd-png error: setjmp returns error condition") - 1) == 0) { | ||
| format = "gd-png error: setjmp returns error condition"; | ||
| } |
There was a problem hiding this comment.
I don't think this logic is necessary, there is explicitly no BC guarantee for error messages in PHP.
Sync php's libgd to current GD's master, which will be 2.4.0
New APIs
2D vector graphic APIs using FT raster (standalone version)
Share compisitions with existing gd's functions
optionally allow to replace exising composition OPs to be replace by the new accurate ones.
ad gdImageRead* APIs, to read an image from a file/stream, codec selected using known binary signature
Codecs Added:
QOI full support (bundled/no dep)
#796 Initial QOI support, read and write libgd/libgd#1005
JXL + JXL animation
Webp animation and full formats support
Full animated WebP read/write support libgd/libgd#1001
Full TIFF support
Initial TIFF multiple pages support libgd/libgd#1010
GIF animations read and write + Full GIF support
BMP All formats but OS/2
Feature/bmpfullsupport libgd/libgd#999
AVIF animation read and write
UltraHDR support, using libultrahdr, read and write as standalone, or read SD image as gdImage
Feature/ultrahdr libgd/libgd#1003
for formats supporting it, raw metadata read/write APIs. can be then be processed and pushed back)
Updated/Fixed
. Fix RAMQ rendering
WIP:
This is the 1st phase, to ensure it builds correctly in php's CI. And hopefully the new deps will be added as well.
The next phases is to expose the new APIs in php's.