Skip to content

[gifdec]: add new port - #53208

Closed
Saikari (luadebug) wants to merge 14 commits into
microsoft:masterfrom
luadebug:gifdec
Closed

Saikari (luadebug) wants to merge 14 commits into
microsoft:masterfrom
luadebug:gifdec

Conversation

@luadebug

@luadebug Saikari (luadebug) commented Aug 4, 2026

Copy link
Copy Markdown
Contributor
  • Changes comply with the maintainer guide.
  • The packaged project is mature and ready for broad sharing with vcpkg users
    • Has a release at least 6 months old or 6 months of demonstrated public development
    • Is an official component of something else meeting that criteria
    • Some other reason (please explain)
  • The packaged project shows strong association with the chosen port name. Check this box if at least one of the following criteria is met:
    • The project is in Repology: https://repology.org/project//versions
    • The project is amongst the first web search results for "" or " C++". Include a screenshot of the search engine results in the PR.
    • The port name follows the 'GitHubOrg-GitHubRepo' form or equivalent Owner-Project form.
  • Optional dependencies of the build are all controlled by the port. A dependency is controlled if it is declared an unconditional dependency in vcpkg.json, or explicitly disabled through patches or build system arguments such as CMAKE_DISABLE_FIND_PACKAGE_Xxx or VCPKG_LOCK_FIND_PACKAGE
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is brief and accurate. See adding-usage for context. Don't add a usage file if the automatically generated usage is correct.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Exactly one version is added in each modified versions file.
image

@BillyONeal Billy O'Neal (BillyONeal) 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.

There seem to be a lot of unresolved CVEs submitted to https://github.com/lecram/gifdec/issues ; GPT 5.6 explicitly points to lecram/gifdec#23 . I guess we technically don't typically reject things for that kind of reason but I fear for anyone trying to actually use this.

Thanks for the new port submission nonetheless!

@@ -0,0 +1,3 @@
@PACKAGE_INIT@
include("${CMAKE_CURRENT_LIST_DIR}/unofficial-gifdec-targets.cmake")
check_required_components("gifdec")

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.

GPT 5.6:

unofficial-gifdec-config.cmake.in calls check_required_components("gifdec") although the package is unofficial-gifdec. Consequently, find_package(unofficial-gifdec REQUIRED COMPONENTS nonexistent) incorrectly succeeds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, it looks like resolved by your follow up PR. 👍🏻

Comment thread ports/gifdec/portfile.cmake Outdated
Comment on lines +27 to +28
file(WRITE "${SOURCE_PATH}/LICENSE" [[gifdec is released into the public domain, as stated in its README file.]])
vcpkg_install_copyright(FILE_LIST "${SOURCE_PATH}/LICENSE")

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.

Suggested change
file(WRITE "${SOURCE_PATH}/LICENSE" [[gifdec is released into the public domain, as stated in its README file.]])
vcpkg_install_copyright(FILE_LIST "${SOURCE_PATH}/LICENSE")
vcpkg_install_copyright(FILE_LIST "${SOURCE_PATH}/README")

We generally don't want to write license text ourselves even for public domain stuff like this because we are not the copyright holders.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, it looks like resolved by your follow up PR. 👍🏻

int main()
{
gd_GIF *gif;
gif = gd_open_gif("test.gif");

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.

Should we give this thing a real gif? This probably just returns nullptr right now.

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 guess we aren't actually running it anyways

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think if it was actually running it would become CVE nightmare 🐞 . So vcpkg-ci-%PORT% actually just checks build success but does not execute?

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.

So vcpkg-ci-%PORT% actually just checks build success but does not execute?

I mean, it does whatever portfile.cmake says. We have examples of both. For instance #53010 added a running example

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@luadebug

Saikari (luadebug) commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

There seem to be a lot of unresolved CVEs submitted to https://github.com/lecram/gifdec/issues ; GPT 5.6 explicitly points to lecram/gifdec#23 . I guess we technically don't typically reject things for that kind of reason but I fear for anyone trying to actually use this.

Thanks for the new port submission nonetheless!

We could apply that PR's diff as patch without storing it here. I dont know maybe its called "CVE-2022-43359".

@luadebug
Saikari (luadebug) marked this pull request as ready for review August 5, 2026 07:05
@BillyONeal

Copy link
Copy Markdown
Member

We could apply that PR's diff as patch without storing it here.

I try to not do that for PRs because any triggered re-merge GitHub does would invalidate the SHA512.

@BillyONeal

Copy link
Copy Markdown
Member

Also, applying a change the upstream maintainers have not is the exact kind of "putting words in upstream's mouth" that we try to avoid most of the time. https://learn.microsoft.com/vcpkg/contributing/maintainer-guide#patching

If upstream is gone enough that they have unfixed CVEs for that long that's a reason to not be in our registry in the first place. Is this something you're adding because you have a particular need for it or did you just go "adding the *gif* packages would be fun"? :)

@BillyONeal
Billy O'Neal (BillyONeal) marked this pull request as draft August 6, 2026 00:33
@BillyONeal

Copy link
Copy Markdown
Member

Drafted due to merge conflicts

@luadebug

Copy link
Copy Markdown
Contributor Author

Also, applying a change the upstream maintainers have not is the exact kind of "putting words in upstream's mouth" that we try to avoid most of the time. https://learn.microsoft.com/vcpkg/contributing/maintainer-guide#patching

If upstream is gone enough that they have unfixed CVEs for that long that's a reason to not be in our registry in the first place. Is this something you're adding because you have a particular need for it or did you just go "adding the *gif* packages would be fun"? :)

That is because I try to debundle that for aui PR, certainly adding MIT/Public Domain ones is maybe useful.

@luadebug

Copy link
Copy Markdown
Contributor Author

We could apply that PR's diff as patch without storing it here.

I try to not do that for PRs because any triggered re-merge GitHub does would invalidate the SHA512.

So that's better option to store patch in vcpkg repo in that case?

@luadebug
Saikari (luadebug) marked this pull request as ready for review August 6, 2026 05:25
@BillyONeal

Copy link
Copy Markdown
Member

So that's better option to store patch in vcpkg repo in that case?

Yes please

@luadebug

Copy link
Copy Markdown
Contributor Author

So that's better option to store patch in vcpkg repo in that case?

Yes please

That is already in PR content by d1a4134 ?

@BillyONeal

Copy link
Copy Markdown
Member

That is already in PR content by d1a4134 ?

Sorry I wrote that at close to midnight half asleep; I was just replying to the question not re-reviewing.

Unfortunately our robot overlords do not like the actual patch there:

The CVE patch imports all six commits from open upstream PR #23. Its discard_sub_blocks() condition, if (!first_try && size == seek_pos) break;, mistakes consecutive valid sub-blocks with equal byte counts for failure to advance. With the patch, a valid 1×1 GIF containing two consecutive one-byte sub-blocks decodes its first frame but returns a parse error from the second gd_get_frame(); upstream correctly reports GIF end.

That is because I try to debundle that for aui PR,

Oh, I see, this is my fault for #53142 (review)

Maybe we should just accept that the one thing vendoring this is the lesser of the available evils :(

@BillyONeal Billy O'Neal (BillyONeal) added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 6, 2026
@BillyONeal Billy O'Neal (BillyONeal) mentioned this pull request Aug 6, 2026
17 tasks
@BillyONeal

Billy O'Neal (BillyONeal) commented Aug 6, 2026

Copy link
Copy Markdown
Member

vcpkg-team-review with #53142 : keep gifdec vendored there and don't add this because it is an exploitable mess?

@luadebug

Copy link
Copy Markdown
Contributor Author

Hi. I think "gifdec.h" and "gif_load.h" are not used anywhere once we git checkout to "v7.1.2" for "aui". The used headers are "nsgif.h" and "stb_image_write.h" and "stb_image.h". It is mine mistake to think to debundle them.

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

Labels

requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants