Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify FreeTypeFont.getmask2() #7645

Merged
merged 1 commit into from
Jan 4, 2024
Merged

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Dec 27, 2023

Changes proposed in this pull request:

  • Call Image._decompression_bomb_check directly in fill rather than duplicating its code. If an error is raised, it is propagated via the if (image == NULL) branch.
  • On error, delete the Python image object by releasing the only reference to it (causing Python to call its tp_dealloc slot, i.e. _dealloc in _imaging.c) instead of just freeing the memory it points to (added in Improved checks in font_render #7218).
  • Avoid nonlocal variables (to simplify adding type hints in a future PR by avoiding the need for a typing.cast or assert size is not None).

This partially reverts changes from #7246, which was connected to an oss-fuzz error, but I do not see any added tests or reproduction steps. IIUC, the original error was a memory leak, which I avoid in a different way in this PR:

The original code (before #7246) returned image via O format (returning a new reference) and did not Py_DECREF (causing a memory leak).
#7246 instead returns a reference via a nonlocal Python variable and added Py_DECREF.
In this PR I have removed the nonlocal variable and return the existing reference via N format.
See https://docs.python.org/3/c-api/arg.html#c.Py_BuildValue for the difference between O and N.

Copy link
Member

@radarhere radarhere left a comment

Choose a reason for hiding this comment

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

I had imagined that raising a DecompressionBombError during a Python callback from C was somehow bad, but our test suite triggers that scenario with your branch, and it is fine.

Presuming that the memory management in fine (I've checked oss-fuzz with the test case from https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=60273 and it passed), I approve - but it is probably not the best idea to adjust code that fixed an oss-fuzz problem less than a week before release, so I suggest holding off on this for now

@nulano
Copy link
Contributor Author

nulano commented Dec 28, 2023

it is probably not the best idea to adjust code that fixed an oss-fuzz problem less than a week before release, so I suggest holding off on this for now

Sure, that sounds reasonable.
There are many other things blocking a PR adding type hints to ImageFont.py anyway.

@hugovk hugovk added this to the 10.3.0 milestone Dec 31, 2023
@radarhere radarhere merged commit f71cfec into python-pillow:main Jan 4, 2024
57 of 58 checks passed
@radarhere radarhere changed the title Simplify FreeTypeFont.render Simplify FreeTypeFont.getmask2() Jan 4, 2024
@nulano nulano deleted the font-bomb branch January 4, 2024 13:33
@radarhere radarhere mentioned this pull request Oct 7, 2024
Sign up for free to 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