codenamecpp / carnage3d

Reimplementation of Grand Theft Auto 1 [GTA1]
MIT License
470 stars 38 forks source link

Use the standard vsnprintf #36

Closed jetomit closed 4 years ago

jetomit commented 4 years ago

Hi,

enabling the address sanitizer causes a crash (on x86_64):

==22750==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000009c4550 at pc 0x0000008a9ec5 bp 0x7ffe5c4d4230 sp 0x7ffe5c4d4228
READ of size 4 at 0x0000009c4550 thread T0
    #0 0x8a9ec4 in stbsp_vsprintfcb ../src/stb_sprintf.h:339
    #1 0x8b509e in stbsp_vsnprintf ../src/stb_sprintf.h:1402
    #2 0x8bb84d in cxx::f_vsnprintf(char*, int, char const*, __va_list_tag*) ../src/strings.cpp:21
    #3 0x41c5a7 in Console::LogMessage(eLogMessage, char const*, ...) ../src/Console.cpp:29
    #4 0x611a52 in System::Initialize() ../src/System.cpp:106
    #5 0x61122d in System::Execute(SysStartupParameters const&) ../src/System.cpp:60
    #6 0x52c4eb in main ../src/Main.cpp:34
    #7 0x7fba2b039b36 in __libc_start_main (/gnu/store/ahqgl4h89xqj695lgqvsaf6zh2nhy4pj-glibc-2.29/lib/libc.so.6+0x22b36)
    #8 0x40e829 in _start (../src/carnage3d/gamedata/carnage3d+0x40e829)

0x0000009c4550 is located 48 bytes to the left of global variable '*.LC13' defined in '../src/System.cpp' (0x9c4580) of size 29
  '*.LC13' is ascii string 'Cannot initialize filesystem'
0x0000009c4552 is located 0 bytes to the right of global variable '*.LC12' defined in '../src/System.cpp' (0x9c4540) of size 18
  '*.LC12' is ascii string 'System initialize'
SUMMARY: AddressSanitizer: global-buffer-overflow ../src/stb_sprintf.h:339 in stbsp_vsprintfcb
Shadow bytes around the buggy address:
  0x000080130850: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
  0x000080130860: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
  0x000080130870: 04 f9 f9 f9 f9 f9 f9 f9 00 00 07 f9 f9 f9 f9 f9
  0x000080130880: 00 00 02 f9 f9 f9 f9 f9 00 00 f9 f9 f9 f9 f9 f9
  0x000080130890: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0000801308a0: 06 f9 f9 f9 f9 f9 f9 f9 00 00[02]f9 f9 f9 f9 f9
  0x0000801308b0: 00 00 00 05 f9 f9 f9 f9 00 00 00 02 f9 f9 f9 f9
  0x0000801308c0: 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
  0x0000801308d0: 02 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9
  0x0000801308e0: 00 00 00 05 f9 f9 f9 f9 00 00 07 f9 f9 f9 f9 f9
  0x0000801308f0: 00 00 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
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
==22750==ABORTING

Offending line is src/stb_sprintf.h:339 in the custom implementation of vsnprintf, which uses some bit magic that trips up the sanitizer. This patch replaces the two (indirect) calls to this function with the standard version.

codenamecpp commented 4 years ago

Too bad that using stbsp_vsnprintf leads to such thing. OK.