Repository navigation
Check size arithmetic where it is used in the quantizers - #10202
Open
Thanatos9404 wants to merge 2 commits into
Open
Thanatos9404 wants to merge 2 commits into
Thanatos9404 wants to merge 2 commits into
Conversation
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.
Member
|
Hi.
|
Author
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes proposed in this pull request:
src/libImaging/CheckedArithmetic.hwith two small inline helpers,ImagingCheckedMul()andImagingCheckedAdd(). They compute asize_tand 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).In #9474 it was pointed out that the multiplications in
quantize_pngquant()cannot overflow, becauseImagingQuantize()limits the number of pixels toINT_MAXand_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 / ysizedivides byysize, so it relies on_quantize()never passing an image with a height of 0. The pixel count is now computed withImagingCheckedMul()and compared withINT_MAX, without a division.quantize_pngquant():malloc(width * height)andmalloc(sizeof(uint32_t) * width * height)relied on the limit inImagingQuantize(). With a 32-bitsize_t, the second product can wrap even whenwidth * heightfits in anint(32768 x 32768, for example). Today that is only avoided becauseImagingQuantize()has already allocated a buffer of the same size. The pixel count is now checked once at the start, the arrays are allocated withcalloc(), and the row offsets are computed insize_t.quantize(),quantize2()andk_means()each had their own copy of the check and the two allocations for then * ndistance tables. These are now a singlealloc_distance_tables(), which also notes whyn * nhas to fit in auint32_t: the tables are indexed withuint32_tarithmetic. The checknPaletteEntries > UINT32_MAX / sizeof(uint32_t)at the start ofk_means()is removed, sincecalloc()already checks those allocations and then * ncheck is stricter.QuantOctree.c:create_sorted_color_palette()andcombined_palette()compared sizes againstLONG_MAX, which is not the limit ofsize_tand is only 32 bits on Windows. They now use the helpers, andcreate_sorted_color_palette()uses the same checked size formalloc()andmemcpy(), instead of zeroing the buffer withcalloc()and then overwriting it.quantize_octree()allocates itsuint32_toutput withsizeof(uint32_t)instead ofsizeof(Pixel). Both are 4 bytes.QuantHash.c: the bucket arrays were allocated withmalloc(sizeof(HashNode *) * length)followed bymemset(). They now usecalloc().Tests:
test_quantize_sizesruns 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_sizesdoes the same for the narrow images with libimagequant. Thek_means()path is already covered bytest_quantize_kmeans.checks/check_large_memory.pyhas a new test that quantizing an image with 2**31 pixels raisesMemoryError. 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 andkmeansgives identical pixel data and palettes onmainand on this branch. libimagequant was not available locally, soquantize_pngquant()was compared with the version onmainusing 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-bitsize_tthe 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 theSIZE_MAXboundaries 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 -Wextraand MSVC/W4.If this looks like a reasonable approach, the same helpers could replace the other hand-written checks in
libImagingin later PRs.