Skip to content

Fix off-by-one bounds checks in Paint_SetPixel() (out-of-bounds write) - #426

Open
timmg wants to merge 1 commit into
waveshareteam:masterfrom
timmg:fix-setpixel-bounds
Open

Fix off-by-one bounds checks in Paint_SetPixel() (out-of-bounds write)#426
timmg wants to merge 1 commit into
waveshareteam:masterfrom
timmg:fix-setpixel-bounds

Conversation

@timmg

@timmg timmg commented Aug 5, 2026

Copy link
Copy Markdown

Both bounds checks in Paint_SetPixel() use > where they need >=, so a coordinate equal to the width or height passes validation and writes outside the caller's buffer.

Paint.Width / Paint.Height (and Paint.WidthMemory / Paint.HeightMemory) are counts, so the last valid index is one less than the value.

-    if(Xpoint > Paint.Width || Ypoint > Paint.Height){
+    if(Xpoint >= Paint.Width || Ypoint >= Paint.Height){
...
-    if(X > Paint.WidthMemory || Y > Paint.HeightMemory){
+    if(X >= Paint.WidthMemory || Y >= Paint.HeightMemory){

This is reachable from the public API

Paint_DrawRectangle() validates its own arguments with > too, so Xend == Paint.Width is accepted. At DOT_PIXEL_2X2 or larger, Paint_DrawPoint()'s loop reaches Paint_SetPixel(Xpoint + XDir_Num - 1, ...) with XDir_Num == 1, i.e. Xpoint itself — so Paint_SetPixel() is called with Xpoint == Paint.Width.

(At DOT_PIXEL_1X1 the - 1 happens to pull it back in bounds, which is why this rarely shows up.)

Compiling the unmodified RaspberryPi_JetsonNano/c/lib/GUI/GUI_Paint.c with AddressSanitizer and an exactly-sized buffer:

UBYTE *img = malloc((128/8) * 296);          /* 4736 bytes, as the docs prescribe */
Paint_NewImage(img, 128, 296, ROTATE_0, WHITE);
Paint_SelectImage(img); Paint_SetScale(2); Paint_Clear(WHITE);
Paint_DrawRectangle(0, 0, 128, 296, BLACK, DOT_PIXEL_2X2, DRAW_FILL_EMPTY);
ERROR: AddressSanitizer: heap-buffer-overflow
    #0 Paint_SetPixel     GUI_Paint.c:241
    #1 Paint_DrawLine     GUI_Paint.c:388
    #2 Paint_DrawRectangle GUI_Paint.c:432
    #3 main
0x621000001380 is located 0 bytes after 4736-byte region

With this change applied, the same program is ASan-clean.

Rendering is unchanged

The guard now rejects only writes that were already out of bounds, so no legitimate pixel is lost. Verified by rendering a scene using in-range coordinates only — nested rectangles filled and unfilled, diagonal lines at DOT_PIXEL_3X3 and dotted, filled and unfilled circles, and a row of DOT_FILL_AROUND points along the last row — then comparing the output buffers:

cmp out_orig.bin out_fixed.bin  ->  identical, 4736 bytes

Scope

The repository contains 42 copies of GUI_Paint.c and all 42 have both bugs. The change is applied to every copy and is byte-identical in each — reviewing one file covers all of them:

42 -    if(X > Paint.WidthMemory || Y > Paint.HeightMemory){
42 -    if(Xpoint > Paint.Width || Ypoint > Paint.Height){
42 +    if(X >= Paint.WidthMemory || Y >= Paint.HeightMemory){
42 +    if(Xpoint >= Paint.Width || Ypoint >= Paint.Height){

Paint_SetPixel() is the single point every pixel write passes through, so fixing it there protects all the drawing primitives without touching them individually.

Note: callers that relied on Paint_DrawRectangle(0, 0, W, H, ...) will now have that last row/column clipped rather than written out of bounds. W-1, H-1 is the correct call and is unaffected.

🤖 Generated with Claude Code

Fixes #427 — that issue has the full write-up and a standalone reproduction.

Paint.Width/Height and Paint.WidthMemory/HeightMemory are counts, so the
last valid index is one less. Both guards used '>', which let
Xpoint == Paint.Width and Ypoint == Paint.Height through and wrote
outside the caller's buffer.

This is reachable from the public API: Paint_DrawRectangle() validates
its own arguments with '>' as well, so passing Xend == Paint.Width is
accepted, and at DOT_PIXEL_2X2 or larger Paint_DrawPoint() reaches
Paint_SetPixel() with Xpoint == Paint.Width.

Paint_SetPixel() is the single point through which all pixel writes
pass, so correcting it here protects every drawing primitive.

Applied to all 42 copies of GUI_Paint.c in the repository; the change is
identical in each.
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.

Paint_SetPixel() bounds checks are off by one, allowing out-of-bounds writes

1 participant