Skip to content

Check size arithmetic where it is used in the quantizers - #10202

Open
Thanatos9404 wants to merge 2 commits into
python-pillow:mainfrom
Thanatos9404:checked-size-arithmetic
Open

Thanatos9404 wants to merge 2 commits into
python-pillow:mainfrom
Thanatos9404:checked-size-arithmetic

Conversation

@Thanatos9404

Copy link
Copy Markdown

Changes proposed in this pull request:

  • Add src/libImaging/CheckedArithmetic.h with two small inline helpers, ImagingCheckedMul() and ImagingCheckedAdd(). They compute a size_t and return 0 instead of overflowing. They use __builtin_mul_overflow() and __builtin_add_overflow() where the compiler has them, and a portable check otherwise (MSVC).
  • Use them in the quantization code, so that each allocation size is checked in the function that computes it, rather than relying on checks made by the callers.

In #9474 it was pointed out that the multiplications in quantize_pngquant() cannot overflow, because ImagingQuantize() limits the number of pixels to INT_MAX and _quantize() returns early for empty images. That is still true, and this PR does not change the output for any image. The aim is that these functions no longer depend on checks in other files to be safe.

  • ImagingQuantize(): xsize > INT_MAX / ysize divides by ysize, so it relies on _quantize() never passing an image with a height of 0. The pixel count is now computed with ImagingCheckedMul() and compared with INT_MAX, without a division.
  • quantize_pngquant(): malloc(width * height) and malloc(sizeof(uint32_t) * width * height) relied on the limit in ImagingQuantize(). With a 32-bit size_t, the second product can wrap even when width * height fits in an int (32768 x 32768, for example). Today that is only avoided because ImagingQuantize() has already allocated a buffer of the same size. The pixel count is now checked once at the start, the arrays are allocated with calloc(), and the row offsets are computed in size_t.
  • quantize(), quantize2() and k_means() each had their own copy of the check and the two allocations for the n * n distance tables. These are now a single alloc_distance_tables(), which also notes why n * n has to fit in a uint32_t: the tables are indexed with uint32_t arithmetic. The check nPaletteEntries > UINT32_MAX / sizeof(uint32_t) at the start of k_means() is removed, since calloc() already checks those allocations and the n * n check is stricter.
  • QuantOctree.c: create_sorted_color_palette() and combined_palette() compared sizes against LONG_MAX, which is not the limit of size_t and is only 32 bits on Windows. They now use the helpers, and create_sorted_color_palette() uses the same checked size for malloc() and memcpy(), instead of zeroing the buffer with calloc() and then overwriting it. quantize_octree() allocates its uint32_t output with sizeof(uint32_t) instead of sizeof(Pixel). Both are 4 bytes.
  • QuantHash.c: the bucket arrays were allocated with malloc(sizeof(HashNode *) * length) followed by memset(). They now use calloc().

Tests:

  • test_quantize_sizes runs each method on 1x1, 1x300, 300x1 and 64x64 images with 1, 2 and 256 colors, and checks that every pixel refers to an entry in the palette. test_libimagequant_quantize_sizes does the same for the narrow images with libimagequant. The k_means() path is already covered by test_quantize_kmeans.
  • checks/check_large_memory.py has a new test that quantizing an image with 2**31 pixels raises MemoryError. Like the rest of that file, it is not run automatically, as it needs 2 GB.

These tests cover the changed code, and they also pass on main, since nothing here is meant to change the results. Quantizing 1,256 combinations of image, mode, method, number of colors and kmeans gives identical pixel data and palettes on main and on this branch. libimagequant was not available locally, so quantize_pngquant() was compared with the version on main using a small stand-in for the libimagequant functions it calls: 400 random sizes gave identical results with GCC and MSVC (x64 and x86), and with a 32-bit size_t the new version returns an error for 65536 x 65536 before calling libimagequant, and for 32768 x 32768 instead of making a short allocation. The helpers were checked at the SIZE_MAX boundaries with GCC (builtins and fallback, with ASan and UBSan) and MSVC (x64 and x86), and the changed files compile without new warnings under GCC -Wall -Wextra and MSVC /W4.

If this looks like a reasonable approach, the same helpers could replace the other hand-written checks in libImaging in later PRs.

Add ImagingCheckedMul() and ImagingCheckedAdd() in CheckedArithmetic.h
for computing sizes without overflowing, and use them for the
allocation sizes in Quant.c, QuantOctree.c and QuantPngQuant.c, so that
each size is checked in the function that computes it. Some of these
sizes were only safe because of checks made by the callers.

Allocate the two distance tables in one function instead of three
copies, allocate the hash table arrays with calloc() instead of
malloc() and memset(), and use sizeof(uint32_t) for the uint32_t output
of quantize_octree().

The output is unchanged for valid images.
@radarhere

Copy link
Copy Markdown
Member

Hi.

  1. To restate what you've said above - this PR's goal isn't to alter output, improve performance or even reduce code. The goal is to make functions more self-contained. This is in order to... try and prevent future contributions from breaking something?
  2. Is this AI assisted?

@Thanatos9404

Copy link
Copy Markdown
Author
  1. Yes. Each allocation size in the quantizers is now checked in the function that computes it, so a later change to a caller, or a 32-bit size_t, cannot turn one of these products into a short allocation without the callee noticing. It also replaces the three copies of the distance table check with one function. The output does not change.

  2. Yes. I directed the work, and the code and tests were written with AI tools. The checks listed in the description (the output comparison over 1,256 cases, the 32-bit MSVC builds and the test suite runs) were run as part of that work. If this is not the kind of change you want, I understand, and I can close it.

@radarhere radarhere added the 🤖-assisted AI-assisted label Oct 14, 2026

This branch has not been deployed

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

Labels

🤖-assisted AI-assisted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants