Correct right-bound filling #80

Bennctu · 2024-12-08T15:40:46Z

Original:
Modified:

weihsinyeh

If so many things are being affected, there may be other issues that haven't been identified yet. Solving this problem would eliminate the need for customization every time a drawing is made. At the same time, it's difficult to understand why only the width is incorrect, but not the height.

weihsinyeh

If window->client.right = 1 and window->client.left = 0, the window's client object is only allowed to access one pixel, not two. If the window's client object is allowed to access two pixels within this range, then why isn't window->client.right initially set to 2? That way, we could directly determine the accessible range by calculating window->client.right - window->client.left.

huaxinliao · 2024-12-09T06:47:42Z

After conducting tests on the fbdev backend, I have found that @Bennctu's improvements contribute to resolving the issue of incorrect right bound filling.

The following are the test results for the four boundaries.
Right-bound:

Left-bound:

Upper-bound:

Lower-bound:

jserv

Compose Git commit messages in the third person along with the bound checks.

Bennctu · 2024-12-16T13:14:25Z

If so many things are being affected, there may be other issues that haven't been identified yet. Solving this problem would eliminate the need for customization every time a drawing is made. At the same time, it's difficult to understand why only the width is incorrect, but not the height.

As this reply, src/path.c#L615 can correct right-bound filling.
Yes, I consider that height bound checking should be adjusted with the same way.

Bennctu · 2024-12-16T13:21:54Z

If window->client.right = 1 and window->client.left = 0, the window's client object is only allowed to access one pixel, not two. If the window's client object is allowed to access two pixels within this range, then why isn't window->client.right initially set to 2? That way, we could directly determine the accessible range by calculating window->client.right - window->client.left.

So, does right not represent the rightmost index here?

weihsinyeh · 2024-12-17T17:59:54Z

src/path.c

@@ -612,7 +612,7 @@ void twin_composite_path(twin_pixmap_t *dst,
    if (bounds.left >= bounds.right || bounds.top >= bounds.bottom)
        return;

-    twin_coord_t width = bounds.right - bounds.left;
+    twin_coord_t width = bounds.right - bounds.left + 1;


The width is added by one here. Then, twin_pixmap_create(TWIN_A8, width, height); passes the width as a parameter. However, in the function twin_pixmap_create() in src/pixmap.c

pixmap->width = width; pixmap->height = height; twin_matrix_identity(&pixmap->transform); pixmap->clip.left = pixmap->clip.top = 0; - pixmap->clip.right = pixmap->width; + pixmap->clip.right = pixmap->width - 1;

"The right clip boundary was set to pixmap->width - 1, ensuring correct clipping during rendering," was written in the commit log of ceb748f. Wouldn't this cause a heap-buffer-overflow again?

If we checkout 8ef78ba which is the previous commit of ceb748f, we use address sanitizer then:

================================================================= ==16909==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61200000cecc at pc 0x5a5e59f2ee5f bp 0x7ffc6cc434c0 sp 0x7ffc6cc434b0 READ of size 1 at 0x61200000cecc thread T0 #0 0x5a5e59f2ee5e in twin_fill_path (/home/ben/ben_ws/mado/demo-sdl+0x22e5e) #1 0x5a5e59f2b386 in twin_composite_path (/home/ben/ben_ws/mado/demo-sdl+0x1f386) #2 0x5a5e59f2bbb6 in twin_composite_stroke (/home/ben/ben_ws/mado/demo-sdl+0x1fbb6) #3 0x5a5e59f2bdee in twin_paint_stroke (/home/ben/ben_ws/mado/demo-sdl+0x1fdee) #4 0x5a5e59f4d672 in twin_icon_draw (/home/ben/ben_ws/mado/demo-sdl+0x41672) #5 0x5a5e59f4a45f in twin_window_draw (/home/ben/ben_ws/mado/demo-sdl+0x3e45f) #6 0x5a5e59f172c0 in apps_multi_start (/home/ben/ben_ws/mado/demo-sdl+0xb2c0) #7 0x5a5e59f134b7 in main (/home/ben/ben_ws/mado/demo-sdl+0x74b7) #8 0x7608f3029d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #9 0x7608f3029e3f in __libc_start_main_impl ../csu/libc-start.c:392 #10 0x5a5e59f13ec4 in _start (/home/ben/ben_ws/mado/demo-sdl+0x7ec4) 0x61200000cecc is located 0 bytes to the right of 268-byte region [0x61200000cdc0,0x61200000cecc) allocated by thread T0 here: #0 0x7608f34b4887 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145 #1 0x5a5e59f2bee7 in twin_pixmap_create (/home/ben/ben_ws/mado/demo-sdl+0x1fee7) SUMMARY: AddressSanitizer: heap-buffer-overflow (/home/ben/ben_ws/mado/demo-sdl+0x22e5e) in twin_fill_path Shadow bytes around the buggy address: 0x0c247fff9980: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00 0x0c247fff9990: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c247fff99a0: 00 00 00 00 fa fa fa fa fa fa fa fa fa fa fa fa 0x0c247fff99b0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00 0x0c247fff99c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x0c247fff99d0: 00 00 00 00 00 00 00 00 00[04]fa fa fa fa fa fa 0x0c247fff99e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c247fff99f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c247fff9a00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c247fff9a10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c247fff9a20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==16909==ABORTING

However, we checkout the current branch, the following is reported:

================================================================= ==18607==ERROR: LeakSanitizer: detected memory leaks Direct leak of 72 byte(s) in 1 object(s) allocated from: #0 0x78636eeb4887 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145 #1 0x5aad8dbfb93f in twin_path_create (/home/ben/ben_ws/mado/demo-sdl+0x2093f) Indirect leak of 32768 byte(s) in 1 object(s) allocated from: #0 0x78636eeb4c38 in __interceptor_realloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:164 #1 0x5aad8dbf8a28 in _twin_path_sdraw (/home/ben/ben_ws/mado/demo-sdl+0x1da28) SUMMARY: AddressSanitizer: 32840 byte(s) leaked in 2 allocation(s).

Base on the above, we can conclude that heap-buffer-overflow doesn't happen.

In fact, upon closely tracing twin_fill_path, we find that the right-bound clipping is implemented in _span_fill in poly.c#L173. Hence, even if the width of filling mask exceeds the width of the pixmap clip, it will not fill beyond the right bound, as ensured in _span_fill in poly.c#L209

weihsinyeh · 2024-12-29T17:43:54Z

The image below is a comparison of the display screens between two commits, 8ef78ba and ceb748f. Commit 8ef78ba is the previous commit to commit ceb748f.

The issue with the display not showing the rightmost pixel of the icon and clock is caused by commit ceb748f.
Therefore, in ceb748f, solving the heap-buffer-overflow issue by using width - 1 is incorrect.
The heap-buffer-overflow issue can be solved by

--- a/src/poly.c
+++ b/src/poly.c
@@ -213,7 +213,7 @@ static void _span_fill(twin_pixmap_t *pixmap,
     }
 
     /* last pixel */
-    if (right & TWIN_POLY_MASK) {
+    if (right & TWIN_POLY_MASK && x != right) {
         w = 0;
         col = 0;
         while (x < right) {

Bennctu · 2024-12-30T14:53:48Z

The image below is a comparison of the display screens between two commits, 8ef78ba and ceb748f. Commit 8ef78ba is the previous commit to commit ceb748f. The issue with the display not showing the rightmost pixel of the icon and clock is caused by commit ceb748f. Therefore, in ceb748f, solving the heap-buffer-overflow issue by using width - 1 is incorrect. The heap-buffer-overflow issue can be solved by
--- a/src/poly.c
+++ b/src/poly.c
@@ -213,7 +213,7 @@ static void _span_fill(twin_pixmap_t *pixmap,
     }
 
     /* last pixel */
-    if (right & TWIN_POLY_MASK) {
+    if (right & TWIN_POLY_MASK && x != right) {
         w = 0;
         col = 0;
         while (x < right) {

Your method can also resolve heap-buffer-overflow issue and includes a right-bound filling check. The code modification is concise.
Simply ensuring that the boundary is initialized correctly once can also resolve the above issues using my method.

jserv

The subject was not accurate. Refine it.

The issue with the display not showing the rightmost pixel of the icon and clock is caused by commit ceb748f. Therefore, in ceb748f, solving the heap-buffer-overflow issue by using width - 1 is incorrect. We revert the changes from commit ceb748f and instead perform a check in `_span_fill()`. The last pixel may have already been covered when processing the middle pixels. To address this, an additional condition is added when processing the last pixel to ensure it is not covered by the middle pixels, allowing us to handle the last pixel separately. See sysprog21#49 See sysprog21#51 Close sysprog21#78

jserv · 2025-01-01T15:05:23Z

Thank @Bennctu and @weihsinyeh for contributing!

jserv requested review from ndsl7109256 and weihsinyeh December 8, 2024 15:42

This comment was marked as resolved.

Sign in to view

jserv changed the title ~~Fix the right bound filling incorrectly~~ Correct right-Bound filling Dec 8, 2024

jserv changed the title ~~Correct right-Bound filling~~ Correct right-bound filling Dec 8, 2024

Bennctu force-pushed the fix_rightbound branch 2 times, most recently from 68420dc to 91bbeff Compare December 8, 2024 16:21

This comment was marked as resolved.

Sign in to view

weihsinyeh reviewed Dec 9, 2024

View reviewed changes

jserv requested changes Dec 15, 2024

View reviewed changes

jserv requested a review from weihsinyeh December 17, 2024 17:26

weihsinyeh reviewed Dec 17, 2024

View reviewed changes

jserv requested a review from weihsinyeh December 28, 2024 22:00

weihsinyeh force-pushed the fix_rightbound branch from 91bbeff to 606ebaf Compare January 1, 2025 09:33

jserv requested changes Jan 1, 2025

View reviewed changes

weihsinyeh force-pushed the fix_rightbound branch from 606ebaf to f4137f5 Compare January 1, 2025 10:05

jserv merged commit a2becfd into sysprog21:main Jan 1, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct right-bound filling #80

Correct right-bound filling #80

Bennctu commented Dec 8, 2024

This comment was marked as resolved.

This comment was marked as resolved.

weihsinyeh left a comment

weihsinyeh left a comment

huaxinliao commented Dec 9, 2024 •

edited

Loading

jserv left a comment

Bennctu commented Dec 16, 2024

Bennctu commented Dec 16, 2024 •

edited

Loading

weihsinyeh Dec 17, 2024 •

edited

Loading

Bennctu Dec 26, 2024

weihsinyeh commented Dec 29, 2024

Bennctu commented Dec 30, 2024

jserv left a comment

jserv commented Jan 1, 2025

Correct right-bound filling #80

Correct right-bound filling #80

Conversation

Bennctu commented Dec 8, 2024

This comment was marked as resolved.

This comment was marked as resolved.

weihsinyeh left a comment

Choose a reason for hiding this comment

weihsinyeh left a comment

Choose a reason for hiding this comment

huaxinliao commented Dec 9, 2024 • edited Loading

jserv left a comment

Choose a reason for hiding this comment

Bennctu commented Dec 16, 2024

Bennctu commented Dec 16, 2024 • edited Loading

weihsinyeh Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Bennctu Dec 26, 2024

Choose a reason for hiding this comment

weihsinyeh commented Dec 29, 2024

Bennctu commented Dec 30, 2024

jserv left a comment

Choose a reason for hiding this comment

jserv commented Jan 1, 2025

huaxinliao commented Dec 9, 2024 •

edited

Loading

Bennctu commented Dec 16, 2024 •

edited

Loading

weihsinyeh Dec 17, 2024 •

edited

Loading