Skip to content

Sync gd 2 4#22532

Open
pierrejoye wants to merge 70 commits into
php:masterfrom
pierrejoye:SYNC_GD_2_4
Open

Sync gd 2 4#22532
pierrejoye wants to merge 70 commits into
php:masterfrom
pierrejoye:SYNC_GD_2_4

Conversation

@pierrejoye

@pierrejoye pierrejoye commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Sync php's libgd to current GD's master, which will be 2.4.0

New APIs

Updated/Fixed

  • Color Match improvement using Lab* + Delta-E and actual % as input
  • Fix and improve freetype rendering and merge both php's libgd + libgd "new encoding"
    . Fix RAMQ rendering
  • All codecs with conformance tests (upstream, C)
  • Perceptual difference (upstream C, will be exposed so php's tests can rely on it

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.

@bukka

bukka commented Jul 5, 2026

Copy link
Copy Markdown
Member

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.

@pierrejoye

Copy link
Copy Markdown
Contributor Author

@bukka yeah
while the asan and co are green here snd libgd. I did fix some false positive before the PRs too. let see :)

@pierrejoye

pierrejoye commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@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?

@pierrejoye

Copy link
Copy Markdown
Contributor Author

@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

@pierrejoye

Copy link
Copy Markdown
Contributor Author

for reference, slowly drafting the RFC here

@edorian

edorian commented Jul 6, 2026

Copy link
Copy Markdown
Member

@edorian hello :)

Hey 👋, very long time no see.

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

Great input. I've added that to my workflows. I should be able to get some results to you and bukka later today.

@edorian edorian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/libgd matches 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 .phpt suite 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.

Comment thread ext/gd/gd.c
Comment thread ext/gd/config.m4
Comment thread ext/gd/gd.c
Comment thread main/php_compat.h
Comment thread ext/gd/config.w32
@pierrejoye

Copy link
Copy Markdown
Contributor Author

@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

@edorian

edorian commented Jul 7, 2026

Copy link
Copy Markdown
Member

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 TimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@pierrejoye

Copy link
Copy Markdown
Contributor Author

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.

@TimWolla TimWolla dismissed their stale review July 9, 2026 10:34

Resolved

@TimWolla

TimWolla commented Jul 9, 2026

Copy link
Copy Markdown
Member

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.

Comment thread ext/gd/gd.c
Comment on lines +302 to +309
/* 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";
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this logic is necessary, there is explicitly no BC guarantee for error messages in PHP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants