Skip to content

Fix AddressSanitizer: global-buffer-overflow #3886

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.

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

Merged
merged 1 commit into from
Aug 20, 2019
Merged

Fix AddressSanitizer: global-buffer-overflow #3886

merged 1 commit into from
Aug 20, 2019

Conversation

khaledhosny
Copy link
Contributor

==10627==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00010e2239c1 at pc 0x000111258c3d bp 0x7ffee286c210 sp 0x7ffee286b988
WRITE of size 4 at 0x00010e2239c1 thread T0
    #0 0x111258c3c in scanf_common(void*, int, bool, char const*, __va_list_tag*) (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x27c3c)
    #1 0x111258d6d in wrap_vsscanf (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x27d6d)
    #2 0x11125902c in wrap_sscanf (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x2802c)
    #3 0x10de70b21 in PrefsUI_LoadPrefs prefs.c:1230
    #4 0x10e02e0ce in fontforge_main startui.c:1109
    #5 0x10d654b11 in main main.c:33
    #6 0x7fff62d7b3d4 in start (libdyld.dylib:x86_64+0x163d4)

0x00010e2239c1 is located 63 bytes to the left of global variable 'fvhintingneededcol' defined in '../fontforgeexe/fontview.c:123:14' (0x10e223a00) of size 4
0x00010e2239c1 is located 0 bytes to the right of global variable 'warn_script_unsaved' defined in '../fontforgeexe/fontview.c:83:6' (0x10e2239c0) of size 1
SUMMARY: AddressSanitizer: global-buffer-overflow (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x27c3c) in scanf_common(void*, int, bool, char const*, __va_list_tag*)

warn_script_unsaved is declared as bool, but prefs.c:1230 casts its pointer to int *, leading the issue above. Prefs of type pr_bool should be int as well, FontForge is pre-C99 and does not know bool.

Type of change

  • Bug fix

==10627==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00010e2239c1 at pc 0x000111258c3d bp 0x7ffee286c210 sp 0x7ffee286b988
WRITE of size 4 at 0x00010e2239c1 thread T0
    #0 0x111258c3c in scanf_common(void*, int, bool, char const*, __va_list_tag*) (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x27c3c)
    #1 0x111258d6d in wrap_vsscanf (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x27d6d)
    #2 0x11125902c in wrap_sscanf (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x2802c)
    #3 0x10de70b21 in PrefsUI_LoadPrefs prefs.c:1230
    #4 0x10e02e0ce in fontforge_main startui.c:1109
    #5 0x10d654b11 in main main.c:33
    #6 0x7fff62d7b3d4 in start (libdyld.dylib:x86_64+0x163d4)

0x00010e2239c1 is located 63 bytes to the left of global variable 'fvhintingneededcol' defined in '../fontforgeexe/fontview.c:123:14' (0x10e223a00) of size 4
0x00010e2239c1 is located 0 bytes to the right of global variable 'warn_script_unsaved' defined in '../fontforgeexe/fontview.c:83:6' (0x10e2239c0) of size 1
SUMMARY: AddressSanitizer: global-buffer-overflow (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x27c3c) in scanf_common(void*, int, bool, char const*, __va_list_tag*)

warn_script_unsaved is declared as bool, but prefs.c:1230 casts its
pointer to int *, leading the issue above. Prefs of type pr_bool should
be int as well, FontForge is pre-C99 and does not know bool.
@khaledhosny
Copy link
Contributor Author

It should be a good idea to build on CI with -fsanitize=address to catch such issues early on.

Copy link
Member

@ctrlcctrlv ctrlcctrlv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct due to the cast, but in another issue we agreed that using C99 features is OK, so bool is OK as long as it doesn't break anything. -fsanitize=address is a good idea in that regard.

@skef
Copy link
Contributor

skef commented Aug 20, 2019

using C99 features is OK

From the top-level CMakeLists.txt:

# Set any global defines
set(CMAKE_C_STANDARD 99)
set(CMAKE_C_STANDARD_REQUIRED TRUE)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED TRUE)

@khaledhosny
Copy link
Contributor Author

khaledhosny commented Aug 20, 2019 via email

@jtanx
Copy link
Contributor

jtanx commented Aug 20, 2019

Yeah, well having a less-crap preferences system is on my mind, but it's on the backlog.

@jtanx jtanx merged commit 07f502f into fontforge:master Aug 20, 2019
Omnikron13 pushed a commit to Omnikron13/fontforge that referenced this pull request May 31, 2022
Fix AddressSanitizer: global-buffer-overflow
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.

4 participants