Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Memory bugs reported by AddressSanitizer #49

Closed
jserv opened this issue Sep 13, 2024 · 1 comment · Fixed by #54
Closed

Memory bugs reported by AddressSanitizer #49

jserv opened this issue Sep 13, 2024 · 1 comment · Fixed by #54

Comments

@jserv
Copy link
Contributor

jserv commented Sep 13, 2024 •
edited
Loading

Enable AddressSanitizer with the following changes:

--- a/mk/common.mk
    b/mk/common.mk
@@ -118,6  118,7 @@ MKDIR      := mkdir -p
 
 __CFLAGS := -Wall -Wextra -pipe
 __CFLAGS  = -O2 -pipe
 __CFLAGS  = -fsanitize=address
 __CFLAGS  = $(CFLAGS)
 
 uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')

After enabling, some heap-buffer-overflow error occurs:

==21064==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x000106d2cccc at pc 0x000102d175c4 bp 0x00016d10e630 sp 0x00016d10e628
READ of size 1 at 0x000106d2cccc thread T0
    #0 0x102d175c0 in twin_fill_path 0x1810 (demo-sdl:arm64 0x1000275c0)
    #1 0x102d12dc0 in twin_composite_path 0x494 (demo-sdl:arm64 0x100022dc0)
    #2 0x102d13354 in twin_composite_stroke 0x218 (demo-sdl:arm64 0x100023354)
    #3 0x102d135a0 in twin_paint_stroke 0x120 (demo-sdl:arm64 0x1000235a0)
    #4 0x102d07ee4 in twin_icon_draw 0x27c (demo-sdl:arm64 0x100017ee4)
    #5 0x102d2fa4c in twin_window_draw 0x524 (demo-sdl:arm64 0x10003fa4c)
    #6 0x102cf64e8 in apps_multi_start 0x298 (demo-sdl:arm64 0x1000064e8)
    #7 0x102cf272c in main 0x348 (demo-sdl:arm64 0x10000272c)
    #8 0x18628b150  (<unknown module>)

0x000106d2cccc is located 0 bytes after 268-byte region [0x000106d2cbc0,0x000106d2cccc)
allocated by thread T0 here:
    #0 0x1039cf124 in wrap_malloc 0x94 (libclang_rt.asan_osx_dynamic.dylib:arm64e 0x53124)
    #1 0x102d13740 in twin_pixmap_create 0x50 (demo-sdl:arm64 0x100023740)
    #2 0x102d12d94 in twin_composite_path 0x468 (demo-sdl:arm64 0x100022d94)
    #3 0x102d13354 in twin_composite_stroke 0x218 (demo-sdl:arm64 0x100023354)
    #4 0x102d135a0 in twin_paint_stroke 0x120 (demo-sdl:arm64 0x1000235a0)
    #5 0x102d07ee4 in twin_icon_draw 0x27c (demo-sdl:arm64 0x100017ee4)
    #6 0x102d2fa4c in twin_window_draw 0x524 (demo-sdl:arm64 0x10003fa4c)
    #7 0x102cf64e8 in apps_multi_start 0x298 (demo-sdl:arm64 0x1000064e8)
    #8 0x102cf272c in main 0x348 (demo-sdl:arm64 0x10000272c)
    #9 0x18628b150  (<unknown module>)

SUMMARY: AddressSanitizer: heap-buffer-overflow (demo-sdl:arm64 0x1000275c0) in twin_fill_path 0x1810
Bennctu added a commit to Bennctu/mado that referenced this issue Sep 14, 2024
@Bennctu
Add bounds check for pixel pointer in _span_fill
46f5247 
This commit adds a check to ensure the pointer 's' does not exceed
the boundaries of the pixmap. By comparing 's' with 'max_pixel_ptr',
we prevent potential heap overflow issues when iterating through
the pixel data. The check is now performed before updating 's' to
avoid unnecessary calculations if the pointer goes out of bounds.

This change helps improve memory safety and prevents out-of-bounds
access during pixel manipulation.

Fix memory issues detected by AddressSanitizer:

See: sysprog21#49
@Bennctu Bennctu mentioned this issue Sep 14, 2024
Merged
Bennctu added a commit to Bennctu/mado that referenced this issue Sep 15, 2024
@Bennctu
Add bounds check for pixel pointer in _span_fill
f0c35f3 
This commit adds a check to ensure the pointer 's' does not exceed
the boundaries of the pixmap. By comparing 's' with 'max_pixel_ptr',
we prevent potential heap-buffer-overflow when iterating through
the pixel data. The check is now performed before updating 's' to
avoid unnecessary calculations if the pointer goes out of bounds.

This change helps improve memory safety and prevents out-of-bounds
access during pixel manipulation.

Close sysprog21#49
@jserv
Copy link
Contributor Author

jserv commented Sep 18, 2024

After #51, AddressSanitizer reports:

==86628==ERROR: AddressSanitizer: heap-use-after-free on address 0x00010801d968 at pc 0x00010470527c bp 0x00016b7330e0 sp 0x00016b7330d8
READ of size 2 at 0x00010801d968 thread T0
    #0 0x104705278 in twin_screen_damaged screen.c:115
    #1 0x1046d4008 in twin_sdl_work sdl.c:129
    #2 0x10470cadc in _twin_run_work work.c:36
    #3 0x1046d77a8 in twin_dispatch dispatch.c:13
    #4 0x1046cd044 in main main.c:131
    #5 0x18628b150  (<unknown module>)

0x00010801d968 is located 72 bytes inside of 144-byte region [0x00010801d920,0x00010801d9b0)
freed by thread T0 here:
    #0 0x1053b7260 in wrap_free 0x98 (libclang_rt.asan_osx_dynamic.dylib:arm64e 0x53260)
    #1 0x1046d3e9c in twin_sdl_read_events sdl.c:94
    #2 0x1046dabbc in _twin_run_file file.c:79
    #3 0x1046d77b0 in twin_dispatch dispatch.c:14
    #4 0x1046cd044 in main main.c:131
    #5 0x18628b150  (<unknown module>)

previously allocated by thread T0 here:
    #0 0x1053b74f0 in wrap_calloc 0x9c (libclang_rt.asan_osx_dynamic.dylib:arm64e 0x534f0)
    #1 0x1047047a4 in twin_screen_create screen.c:17
    #2 0x1046d2f48 in twin_sdl_init sdl.c:175
    #3 0x1046ccc8c in main main.c:86
    #4 0x18628b150  (<unknown module>)

SUMMARY: AddressSanitizer: heap-use-after-free screen.c:115 in twin_screen_damaged

weihsinyeh added a commit to weihsinyeh/mado that referenced this issue Oct 1, 2024
@weihsinyeh
Fix the heap-use-after-free and memory leak
5ff910c 
To fix the heap-use-after-free issue when `screen` is accessed by the
`twin_sdl_work()` function, remove the `_twin_sdl_destroy(screen, tx)`
operation. Additionally, to address the memory leak, add the
`twin_path_destroy(path)` function to ensure the unused path is properly
destroyed.

Close sysprog21#49
@weihsinyeh weihsinyeh mentioned this issue Oct 1, 2024
Merged
weihsinyeh added a commit to weihsinyeh/mado that referenced this issue Oct 2, 2024
@weihsinyeh
Fix the heap-use-after-free and memory leak
93d3b4a 
To fix the heap-use-after-free issue when 'screen' is accessed by the
'twin_sdl_work()' function, remove the '_twin_sdl_destroy(screen, tx)'
operation. Additionally, to address the memory leak, add the
'twin_path_destroy(path)' function to ensure the unused path is properly
destroyed. Add the operation to deallocate 'frame', as it will no longer
be used in the future.

See: sysprog21#49
@jserv jserv closed this as completed in #54 Oct 2, 2024
@jserv jserv closed this as completed in 06a8717 Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix the heap-use-after-free and memory leak weihsinyeh/mado
1 participant
@jserv