Repository navigation
[font] Saturate synthetic bold and glyph origin arithmetic - #6585
Conversation
harfbuzz#6154 made hb_position_t arithmetic saturate and harfbuzz#6296 did the same for synthetic_glyph_extents(), but the rest of hb_font_t still used plain signed arithmetic: - changed() computed the embolden strength with abs (x_scale), which overflows for INT32_MIN, and converted the float product to int without a range check. A strength of INT32_MIN could also not be negated later on, for negative scales. - get_font_[hv]_extents(), the advance, origin and contour point getters added the strength with += and could overflow. - add_offset(), subtract_offset() and apply_offset() overflow when they apply a large origin, and so do the origin fallbacks in apply_glyph_[hv]_origins_with_fallback(). - em_multf() converted its float result to hb_position_t without a range check, which is undefined for a value such as an ascender above the em at a scale close to INT32_MAX. Use hb_saturate_add(), hb_saturate_sub() and hb_clamp_to() in these places, and keep the strength away from INT32_MIN so that negating it stays defined. Results that fit do not change. The shift variables that were int are now hb_position_t, since the saturating helpers deduce one type and int32_t is not int everywhere. Tested with test-font under UBSan: the new tests fail on the base commit at hb-font.hh:365, :848, :1154 and :1167, and pass here. Assisted-by: Claude
ideal-song
left a comment
ideal-song
left a comment
There was a problem hiding this comment.
Reviewed the library change and the tests.
The saturation fixes are correct, and the extreme-strength test and the real-font metrics test look good too.
I left just two non-blocking questions as line comments.
| hb_position_t em_mult (int32_t v, int64_t mult) | ||
| { return (hb_position_t) ((v * mult + 32768) >> 16); } | ||
| hb_position_t em_multf (float v, float mult) | ||
| { return (hb_position_t) roundf (em_fmultf (v, mult)); } |
There was a problem hiding this comment.
It's good that the strength can never be INT32_MIN. Negating it later is safe.
|
|
||
| /* The strength does not fit in 32 bits, in either direction. */ | ||
| hb_font_set_scale (font, 2048, 2048); | ||
| hb_font_set_synthetic_bold (font, 1e30f, 1e30f, FALSE); |
There was a problem hiding this comment.
Checking the extreme strength in both directions is good.
Checking the INT32_MIN scale too is good.
| { return (hb_position_t) ((v * mult + 32768) >> 16); } | ||
| hb_position_t em_multf (float v, float mult) | ||
| { return (hb_position_t) roundf (em_fmultf (v, mult)); } | ||
| { return hb_clamp_to<hb_position_t> (roundf (em_fmultf (v, mult))); } |
There was a problem hiding this comment.
Clamping the float conversion for metrics is good too.
It's also good that a real font is used to check it.
| g_assert_cmpint (a, ==, INT32_MAX); | ||
|
|
||
| hb_position_t x = 0, y = 0; | ||
| g_assert_true (hb_font_get_glyph_h_origin (font, 1, &x, &y)); |
There was a problem hiding this comment.
Non-blocking question:
Checking the plural origins too seems good to me — what do you think?
The test currently checks only the singular origins. Removing the saturation in the plural versions would still pass. Calling the plural functions once each and checking for INT32_MAX would cover it.
There was a problem hiding this comment.
Good point, added in 723ea77. test_synthetic_bold_saturates now also calls hb_font_get_glyph_h_origins and hb_font_get_glyph_v_origins once each and checks both coordinates for INT32_MAX.
I also verified the new assertions actually cover the plural code by mutation: replacing the plural hb_saturate_add calls with plain addition makes the test fail (the value wraps to a negative). Full meson test -C build after the change: 270 ok, 0 fail, 7 skipped, same as before.
| /* Slant */ | ||
| if (slant_xy) | ||
| *x += roundf (*y * slant_xy); | ||
| *x = hb_saturate_add (*x, hb_clamp_to<hb_position_t> (roundf (*y * slant_xy))); |
There was a problem hiding this comment.
Non-blocking question
Adding a slant case to the test seems good to me — what do you think?
The test never sets a slant, so removing the clamp on this line would still pass. Setting a large slant would check whether x saturates.
There was a problem hiding this comment.
Agreed, added in 723ea77: a new test_synthetic_slant_saturates sets a slant of 1 on a font whose contour point is INT32_MAX - 1 in both axes, and checks that x saturates to INT32_MAX.
Mutation-checked too: dropping the hb_clamp_to on this line makes the new test fail (x comes out as -2 instead of INT32_MAX).
Review feedback: the plural origin getters and the slant clamp in get_glyph_contour_point were not covered by the tests, so removing their saturation would still pass. test_synthetic_bold_saturates now also calls hb_font_get_glyph_h_origins and hb_font_get_glyph_v_origins once each and checks both coordinates for INT32_MAX. A new test_synthetic_slant_saturates sets a slant of 1 on a contour point one short of INT32_MAX in both axes and checks that x saturates to INT32_MAX. Verified by mutation: replacing the plural hb_saturate_add calls with plain addition makes the first test fail (the value wraps), and dropping the hb_clamp_to in the slant path makes the second fail (x comes out as -2). Full meson test -C build: 270 ok, 0 fail, 7 skipped (subset+repack, missing repacker dependency; same as the baseline run for this PR). Assisted-by: Claude
|
Thanks, both questions are now covered: the plural origins check both coordinates, and the new slant test pins x to INT32_MAX. The mutation check makes this convincing. No further questions from me. |
behdad
left a comment
behdad
left a comment
There was a problem hiding this comment.
One correctness regression in the contour-point slant calculation; reproduced through the public API against the base and PR head.
| /* Slant */ | ||
| if (slant_xy) | ||
| *x += roundf (*y * slant_xy); | ||
| *x = hb_saturate_add (*x, hb_clamp_to<hb_position_t> (roundf (*y * slant_xy))); |
There was a problem hiding this comment.
[P2] Clamp the final slanted coordinate instead of the displacement
Clamping the displacement before adding *x changes valid results when the two values cancel. With equal x/y scales, a contour-point callback returning x = -2000000000, y = 2000000000, and hb_font_set_synthetic_slant(font, 2.f), hb_font_get_glyph_contour_point() returns x = 2000000000 on the base, but 147483647 here. The original floating-point sum fits in hb_position_t; the new code first truncates the displacement of 4000000000 to INT32_MAX.
Please clamp after the floating-point addition, for example:
*x = hb_clamp_to<hb_position_t> (*x + roundf (*y * slant_xy));This preserves the previous calculation for representable results while making the final conversion safe. Please add the cancellation case alongside the existing slant saturation test.
Reproduced on macOS ARM64 with base fcebeeba6 and head 723ea77bc.
There was a problem hiding this comment.
Fixed in 872872f: the slant shift is now added to *x first and the sum clamped with hb_clamp_to, as suggested.
Added test_synthetic_slant_cancelling next to test_synthetic_slant_saturates with the case from your report (x = -2000000000, y = 2000000000, slant 2, equal scales), asserting x == 2000000000. Mutation-checked: restoring the clamp-first order makes the new test fail with 147483647, the value from your report. The existing saturation case still passes, and the full suite matches the baseline: meson test -C build: 270 ok, 0 fail, 7 skipped.
Leaving the thread open for you to resolve.
get_glyph_contour_point clamped the slant shift to hb_position_t before adding it to x. When x and the shift have opposite signs and partly cancel, clamping first changes the result: a contour point at x = -2000000000, y = 2000000000 with a slant of 2 should land at x = 2000000000, but came out as 147483647, because the shift was clamped to INT32_MAX before the addition. Add the shift first and clamp the sum instead. The existing saturation case is unchanged: hb_clamp_to widens to double to do the clamp, so an out-of-range sum still saturates instead of hitting an undefined float-to-int conversion. The new test_synthetic_slant_cancelling covers the cancelling case. Verified by mutation: restoring the clamp-first order makes it fail with 147483647, the reported value. Full meson test -C build: 270 ok, 0 fail, 7 skipped, same as baseline. Assisted-by: Claude
Without a vmtx table, hb_ot_get_glyph_v_advances() and hb_ot_get_glyph_v_origins() derive the vertical advance from the horizontal font extents as descender - ascender (resp. ascender - descender) in plain int. Each extent is clamped to hb_position_t on its own, but MVAR 'hasc'/'hdsc' deltas are 32-bit, so a variable font can move the ascender to INT32_MAX and the descender to INT32_MIN at the default scale, and the subtraction overflows (UBSan signed-integer-overflow at hb-ot-font.cc:524 and :777). Reached from vertical shaping and hb_font_get_glyph_v_advance() / hb_font_get_glyph_v_origin(). #6585 saturated the corresponding hb_font_t arithmetic; use hb_saturate_sub() for these two as well. Results that fit do not change. Add an 824-byte generated font (no vmtx, MVAR with extreme hasc/hdsc deltas) and a test in test/api/test-font.c that hits both paths. Testing: meson test -C build (103 ok); an ASan+UBSan build (experimental_api, glib disabled) passes its suite, and the new test body run standalone against it fails before the fix and passes after. Assisted-by: Claude
After #6154 and #6296, a number of signed operations in hb_font_t's synthetic embolden and slant paths could still overflow:
roundf (abs (scale) * embolden)converted an unchecked float to int, and the result is later negated, which is undefined for INT32_MIN;add_offset/subtract_offset, and the variation-offset path that multiplied by ±1 (INT32_MIN * -1);em_multf, which rounded a float product straight into hb_position_t.All of these now go through
hb_saturate_add/hb_saturate_sub, and float results are clamped withhb_clamp_tofirst. The strength computation clamps to int32 and stays above INT32_MIN so the later negation is safe. Results that do not overflow are unchanged. Four new tests in test/api/test-font.c cover the strength, extents, advances and origins at extreme scales and embolden values.What I ran (Linux x86-64, gcc, meson 1.12.1):
meson setup build— glib, freetype and cairo found; introspection offmeson test -C build— 270 of 277 tests OK, 0 failures, 7 skipped (all subset+repack; the repacker dependency is not available in this environment). test-font passes with 21 subtests. The shape suites pass in this run: in-house 6346 subtests, aots 541, text-rendering 874.What I did not run: a UBSan build or the fail-before comparison myself — the commit message records the author's UBSan runs, where the new tests fail on the base commit at hb-font.hh:365, :848, :1154 and :1167 and pass with this change. I also did not run the gobject/introspection tests (disabled in this build configuration), MSVC or arm-none-eabi builds, subset tests beyond what the default suite above includes, or any performance measurements.