Skip to content

Fix out-of-bounds write in drawChar() classic glcdfont path#807

Merged
moononournation merged 1 commit into
moononournation:masterfrom
94xhn:fix-glcdfont-negative-coord-oob
Jul 14, 2026
Merged

Fix out-of-bounds write in drawChar() classic glcdfont path#807
moononournation merged 1 commit into
moononournation:masterfrom
94xhn:fix-glcdfont-negative-coord-oob

Conversation

@94xhn

@94xhn 94xhn commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Problem

The classic built-in glcdfont branch in Arduino_GFX::drawChar() sends pixels directly to writePixelPreclipped(). Its loops checked _max_text_x and _max_text_y, but not the corresponding minimum text bounds.

When a partially visible character starts above or left of the text area, negative coordinates can therefore reach canvas implementations that index their framebuffer directly and rely on the caller to have clipped them.

Fix

Add the missing _min_text_x and _min_text_y checks to the existing column and row conditions. Visible pixels continue to use writePixelPreclipped() directly; only pixels outside the top or left text boundary are skipped.

Verification

I built a host harness against the real repository Arduino_G.cpp, Arduino_GFX.cpp, and Arduino_Canvas.cpp, using a ProbeCanvas subclass only to record coordinates before forwarding each call to the real canvas implementation.

  • Before the fix, setCursor(-3, -2); print("A") made 40 preclipped writes: 13 had framebuffer offsets outside [0, 4096), and 15 more had invalid coordinates that mapped to unrelated in-buffer pixels.
  • With the minimum-bound checks, the same case makes 12 visible preclipped writes and zero invalid-coordinate or out-of-buffer writes.
  • setCursor(-3, 5) likewise completes with 16 visible writes and zero invalid writes.
  • Fully visible and default canvas-clipped framebuffer hashes match the prior bounds-safe implementation.
  • A custom text bound starting at (4, 5) also clips pixels against its minimum x/y values.

The host harness compiles and runs successfully. Physical display hardware was not used.

@moononournation

Copy link
Copy Markdown
Owner

changing writePixelPreclipped to writePixel is not a good idea, please pay more effort to make it.

The built-in glcdfont path checked only the maximum text bounds before
calling writePixelPreclipped. A partially visible character starting above
or left of the text area could therefore pass negative coordinates to
canvas implementations that trust their caller and index the framebuffer
directly.

Add the missing minimum-text-bound checks in the column and row loops.
This keeps the direct preclipped write path for visible pixels while
skipping pixels outside the top or left text boundary.

Constraint: writePixelPreclipped intentionally performs no coordinate validation.
Rejected: Route every glyph pixel through writePixel | maintainer requested preserving the preclipped fast path.
Confidence: high
Scope-risk: narrow
Directive: Keep all coordinates validated against both text bounds before preclipped writes.
Tested: real Arduino_GFX/Arduino_Canvas host harness; partially visible negative cursor has 0 invalid writes; visible and canvas-clipped framebuffer hashes match the prior safe output; custom text-bound clipping verified; git diff --check
Not-tested: physical display hardware
@94xhn 94xhn force-pushed the fix-glcdfont-negative-coord-oob branch from ecc338f to 23a4da0 Compare July 14, 2026 15:34
@94xhn

94xhn commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Updated in 23a4da0. The glyph path now keeps writePixelPreclipped() and adds the missing _min_text_x/_min_text_y checks alongside the existing maximum-bound checks. The real Canvas harness reports zero invalid writes for the negative-cursor cases, while fully visible and canvas-clipped framebuffer hashes remain unchanged. I also corrected the PR description to match the measured results.

@moononournation moononournation merged commit b979395 into moononournation:master Jul 14, 2026
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