Skip to content

fix(board): do not consume a glyph the renderer refused to draw - #481

Merged
BitHighlander merged 1 commit into
release/7.14.2from
fix/draw-string-clipped-final-glyph
Aug 18, 2026
Merged

fix(board): do not consume a glyph the renderer refused to draw#481
BitHighlander merged 1 commit into
release/7.14.2from
fix/draw-string-clipped-final-glyph

Conversation

@BitHighlander

Copy link
Copy Markdown
Owner

This is an independent correctness fix, not part of the postponed UX work. It corrects an off-by-one I introduced in 7e07b2d92 (#428's implementation). Cherry-picked from #480, which found it; that PR's scrolling redesign is deliberately excluded.

The defect

draw_string_walk() advanced str_write unconditionally after draw_char_impl():

have_space = draw_char_impl(canvas, &char_params, &x_offset, NULL, img, measure);
str_write++;                    // runs even when the glyph was REJECTED

When the last glyph is the one that does not fit, have_space goes false but the pointer has already moved past it. The loop exits, *str_write == '\0', and the walk reports that every character was placed.

confirm_body_fits() is that same walk in measure mode. So a body overflowing by exactly one glyph measured as fitting — precisely the failure #428 exists to prevent, reintroduced by the implementation of it.

Why the earlier sweep missed it

When #428 landed I swept 3,510 adversarial bodies at both body widths looking for disagreements and found none. That sweep could not have caught this: every input in it overflowed by more than one character, or not at all. The window is one glyph wide.

The regression test pins the boundary directly rather than sampling near it — 117 digits fill three rows, the 118th cannot be placed:

EXPECT_TRUE (confirm_body_fits(digits.substr(0, 117).c_str(), BODY_WIDTH));
EXPECT_FALSE(confirm_body_fits(digits.c_str(), BODY_WIDTH));

Negative controlled: rebuilt against the pre-fix draw.c, BodyFits.ConfirmBodyFits FAILS. With the fix, the full suite passes.

Scope

Four lines in draw.c plus the test. No behaviour change other than refusing to over-report completeness — a body that previously measured as fitting by one glyph now correctly does not.

Explicitly not included: #480's replacement of the Cut Off screen with a hold-to-scroll state machine. That is a change to the security-relevant confirmation flow (new SCROLLING state, timer-driven page advance, altered approval gating) and it changes the button-flow expectations that #477's failure inventory was measured against. It belongs on alpha with its own CI, adversarial review and hardware round.

Verified

ARM cross-compile in the pinned builder image; full xunit green; cppcheck clean under CI's exact invocation (0 findings); clang-format clean.

draw_string_walk() advanced str_write unconditionally after draw_char_impl().
When the LAST glyph was rejected, have_space went false but the pointer had
already moved past it, so the loop exited with *str_write == '\0' and the walk
reported that every character was placed.

confirm_body_fits() is that walk in measure mode, so a body overflowing by
exactly one glyph measured as fitting -- the precise failure the #428 redesign
exists to prevent, reintroduced by the implementation of it. Mine, in
7e07b2d.

The window is one glyph wide, which is why the 3,510-body sweep run when that
landed did not catch it: every input in that corpus overflowed by more than one
character, or not at all. The regression test pins the boundary directly -- 117
digits fill three rows, the 118th cannot be placed -- and is negative
controlled: against the pre-fix draw.c, BodyFits.ConfirmBodyFits FAILS.

Cherry-picked from #480, which found it. Deliberately WITHOUT that PR's
scrolling consent redesign: this is an independent correctness fix, four lines,
no behaviour change beyond refusing to over-report completeness. #480 replaces
the Cut Off screen with a hold-to-scroll state machine, which is a change to the
security-relevant confirmation flow and belongs on alpha with its own CI,
adversarial review and OLED round -- not in a security point release.

Verified: ARM cross-compile in the pinned builder image; full xunit green;
cppcheck clean under CI's exact invocation (0 findings); clang-format clean.

Refs #428 #480
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.

1 participant