Skip to content

avoid rounding boundary case in color quantization - #125

Merged
dloebl merged 2 commits into
dloebl:mainfrom
MCLoebl:fix_colorquant_round
Jul 23, 2026
Merged

avoid rounding boundary case in color quantization#125
dloebl merged 2 commits into
dloebl:mainfrom
MCLoebl:fix_colorquant_round

Conversation

@MCLoebl

@MCLoebl MCLoebl commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

(bug fix thanks to Arthur Chan)

AI summary:
crawl_decision_tree's partition loop assumes the slice's mean is a true separator: every value below it in pPalette, at most one above. That mean was computed in get_mean using single-precision float accumulators. For a slice whose weighted sum exceeds float's 24-bit exact-integer range (2^24), the accumulated sum loses low-order bits, and the final division can round the mean below every value in the slice. When that happens, the descending pointer k in the partition loop never finds a value at or below the pivot and walks past idxMin, underflowing the uint32_t index. The resulting child node claims range [idxMin, UINT32_MAX], and any subsequent walk of it (e.g. get_variance) is an unbounded out-of-bounds read, reachable at cgif_rgb_addframe with attacker-controlled pixel data.

Fix: compute the mean and variance in double throughout. sum/m in get_mean stay bounded by 255 * numPixel, and numPixel is bounded by the uint16_t width/height limit (~4.3 * 10^9), so all sums remain exactly representable in double (2^53). The resulting division is a single correctly-rounded IEEE-754 operation, with an error several orders of magnitude smaller than the smallest possible true distance from the mean to an integer boundary. This removes the rounding-direction failure mode structurally, not just empirically.

Changed:

  • treeNode.mean, argmax64, get_mean, get_variance, new_tree_node's var — float to double.
  • get_palette_from_decision_tree: roundf to round, since it now rounds a double mean (previously silently narrowed back to float before rounding, which was harmless for output but inconsistent with the type change).
  • Added poc to tests/meson.build reproducing the original crash.

pImageDataRGBfloat (the dithering error-diffusion buffer) is intentionally left as float: it holds individual pixel values (0-255, exact in float), not an accumulating sum, so it isn't part of this bug.

@dloebl dloebl left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM

@dloebl
dloebl merged commit 509a8b8 into dloebl:main Jul 23, 2026
7 checks passed
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.

2 participants