Avoid integer overflow when calculating reduced image size

[lazerg]

ImagingReduce() sizes the output image with (box[2] + xscale - 1) / xscale. When the box width plus the scale exceeds INT_MAX that addition overflows and the result is a zero-width, zero-height image. ImagingReduceCorners() then writes to imOut->image8[0][0], which has no rows allocated, so Image.new("L", (4, 4)).reduce(2**31 - 1) segfaults.

Since the box is never empty, the same round-up can be written as (box[2] - 1) / xscale + 1, which cannot overflow and gives a 1x1 image as expected.

Fixes #9903

[akx]

I'd maybe add a comment here about the operation order, and that it guarantees the image to have at least size 1x1.

[lazerg]

Added in bf60f3c.

While checking this I noticed that xscale * yscale in ImagingReduceNxN and (box[2] % xscale) * yscale in ImagingReduceCorners still overflow with a factor this large. Those values only feed loops that run zero iterations, so the output pixels are correct and no bad memory access happens, but an instrumented build will still report the overflow on the same input. Clamping both scales to the box size after the size calculation would remove it.

I am happy to add that here, or to keep this PR to the crash and raise it separately.

[akx]

What do you mean with "Those values only feed loops that run zero iterations"? Those functions probably shouldn't end up being called...

In fact, it could be a good idea to add separate paths when the reduction ends up being one-dimensional in one dimension or the other, or both, since isn't the end result then just an average over rows, columns or both (separable)?

[lazerg]

They are still called, not skipped. What's zero is the inner loop count: box[2] / xscale and box[3] / yscale. Box can never exceed INT_MAX, since it's just an image dimension. Here xscale is 2**31-1. So both divisions come out to 0, and the pixel-reading loops never run.

The overflow happens earlier, before the loop starts. It's in yscale * xscale (ImagingReduceNxN) and (box[2] % xscale) * yscale (ImagingReduceCorners). Those feed multiplier and amend, which only get used inside the loop that never executes. So the overflowed value never reaches memory or an output pixel. It's still UB at that line though. An UBSan build will still report it on this input, even though the real output is correct.

The separable idea for the one-dimensional case is worth doing. But it's a bigger change than this crash fix. I'd rather open it as its own issue instead of growing this PR.

[radarhere]

I don't see why different modes would affect this at all?

[lazerg]

Fair point, the sizes don't change by mode. I added L/I/F so the test also exercises the 32bpc path (ImagingReduceNxN_32bpc / ImagingReduceCorners_32bpc), a separate C function from the 8-bit one that hit the same crash. Without I/F only the 8-bit path gets covered.

[radarhere]

Those other paths come after the fix. So you're testing to make sure that there aren't other overflow bugs lurking in reduce()?

[lazerg]

Not lurking bugs in general, more specific: the crash itself lives in two separate C functions depending on bit depth (8bpc writes image8, 32bpc writes image32), and the size-calc fix is shared by both. So L only proves the 8bpc writer is safe after the fix, I/F prove the 32bpc one is too.