View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002819 | FSSCP | public | 2013-03-23 04:03 | 2013-05-04 06:26 | |
Reporter | Echelon9 | Assigned To | Echelon9 | ||
Priority | normal | Severity | major | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.6.19 | ||||
Target Version | 3.7.0 | ||||
Summary | 0002819: AddressSanitizer: stack-buffer-overflow in l_File_read_f() lua.cpp:1207 | ||||
Description | Reported by AddressSanitizer, a memory error detector for C/C++, in FS2Open builds based on trunk r9585. ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff5fbf3bdf at pc 0x1025f3b2e bp 0x7fff5fbf35f0 sp 0x7fff5fbf35e8 READ of size 1 at 0x7fff5fbf3bdf thread T0 #0 0x1025f3b2d in l_File_read_f lua.cpp:1207 0000001 0x102a7e6c1 in luaD_precall ldo.c:320 0000002 0x102b6ad61 in luaV_execute lvm.c:591 0000003 0x102a8452d in luaD_call ldo.c:378 0000004 0x102a14740 in f_call lapi.c:800 ... ADE_FUNC(read, l_File, "number or string, ...", /*...*/ ) { ... else if(!stricmp(fmt, "*l")) { char buf[10240]; if(cfgets(buf, (int)(sizeof(buf)/sizeof(char)), cfp) == NULL) { lua_pushnil(L); } else { //WMC - strip all newlines so this works like the Lua original char *pos = buf + strlen(buf); while(*--pos == '\r' || *pos == '\n') *pos = '\0'; <========== lua_pushstring(L, buf); } num_returned++; } | ||||
Steps To Reproduce | 1. Utilise a version of Clang that supports AddressSantizer (https://code.google.com/p/address-sanitizer/wiki/AddressSanitizer). 2. Build with -fsanitize=address C/C++ flag 3. Run the game with mediavps 3.6.12 | ||||
Additional Information | ==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff5fbf3bdf at pc 0x1025f3b2e bp 0x7fff5fbf35f0 sp 0x7fff5fbf35e8 READ of size 1 at 0x7fff5fbf3bdf thread T0 #0 0x1025f3b2d in l_File_read_f lua.cpp:1207 0000001 0x102a7e6c1 in luaD_precall ldo.c:320 0000002 0x102b6ad61 in luaV_execute lvm.c:591 0000003 0x102a8452d in luaD_call ldo.c:378 0000004 0x102a14740 in f_call lapi.c:800 0000005 0x102a786c0 in luaD_rawrunprotected ldo.c:116 0000006 0x102a87476 in luaD_pcall ldo.c:464 0000007 0x102a140a5 in lua_pcall lapi.c:821 0000008 0x1027f3a26 in script_state::RunBytecodeSub scripting.cpp:818 0000009 0x1027ebdb2 in script_state::RunBytecode scripting.cpp:859 0000010 0x1027eb8e8 in ConditionedHook::Run scripting.cpp:465 #11 0x1027f4b62 in script_state::RunCondition scripting.cpp:870 0000012 0x1002bdd5c in game_post_level_init freespace.cpp:1409 0000013 0x1002becfc in game_start_mission freespace.cpp:1466 0000014 0x1002fc197 in game_enter_state freespace.cpp:5934 0000015 0x1008ef8ae in gameseq_set_state gamesequence.cpp:280 0000016 0x1002f6e82 in game_process_event freespace.cpp:5105 0000017 0x1008f10ff in gameseq_process_events gamesequence.cpp:395 0000018 0x100306248 in game_main freespace.cpp:7045 0000019 0x1003078b6 in SDL_main freespace.cpp:7179 0000020 0x10000307a in -[SDLMain applicationDidFinishLaunching:] SDLMain.m:300 0000021 0x7fff8ffe3ed9 in _CFXNotificationPost (in CoreFoundation) + 2553 0000022 0x7fff85fc6e25 in -[NSNotificationCenter postNotificationName:object:userInfo:] (in Foundation) + 63 0000023 0x7fff8b92855c in -[NSApplication _postDidFinishNotification] (in AppKit) + 291 0000024 0x7fff8b928295 in -[NSApplication _sendFinishLaunchingNotification] (in AppKit) + 215 0000025 0x7fff8b925481 in -[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] (in AppKit) + 565 0000026 0x7fff8b92507b in -[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] (in AppKit) + 350 0000027 0x7fff85fe070a in -[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] (in Foundation) + 307 0000028 0x7fff85fe056c in _NSAppleEventManagerGenericHandler (in Foundation) + 105 0000029 0x7fff8e3a9077 in aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) (in AE) + 306 0000030 0x7fff8e3a8ed8 in dispatchEventAndSendReply(AEDesc const*, AEDesc*) (in AE) + 36 0000031 0x7fff8e3a8d98 in aeProcessAppleEvent (in AE) + 317 0000032 0x7fff88b11708 in AEProcessAppleEvent (in HIToolbox) + 99 0000033 0x7fff8b921865 in _DPSNextEvent (in AppKit) + 1455 0000034 0x7fff8b920e21 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 127 0000035 0x7fff8b9181d2 in -[NSApplication run] (in AppKit) + 516 0000036 0x100005193 in CustomApplicationMain SDLMain.m:227 0000037 0x100004c10 in main SDLMain.m:377 0000038 0x100001de3 in start (in FS2_Open (debug)) + 51 0000039 0x0 in 0x0 Address 0x7fff5fbf3bdf is located at offset 927 in frame <l_File_read_f(lua_State*)> of T0'short stack: This frame has 19 object(short): [32, 36) 'retval' [96, 104) 'L.addr' [160, 168) 'cfp' [224, 256) 'agg.tmp' [288, 292) 'int' [352, 356) 'num_returned' [416, 420) 'type' [480, 484) 'lastarg' [544, 552) 'fmt' [608, 616) 'double' [672, 676) 'tell_res' [736, 740) 'read_len' [800, 808) 'buf' [864, 868) 'final_len' [928, 11168) 'buf32' [11200, 11208) 'pos' [11264, 11268) 'num' [11328, 11336) 'buf70' [11392, 11396) 'total_read' HINT: this may be signed char false positive if your program uses some custom stack unwind mechanism or swapcontext (longjmp and C++ exceptions *are* supported) Shadow bytes around the buggy address: 0x1fffebf7e720: f2 f2 f2 f2 00 00 00 00 f2 f2 f2 f2 04 f4 f4 f4 0x1fffebf7e730: f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2 04 f4 f4 f4 0x1fffebf7e740: f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 0x1fffebf7e750: f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2 04 f4 f4 f4 0x1fffebf7e760: f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 =>0x1fffebf7e770: f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2[f2]00 00 00 00 0x1fffebf7e780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1fffebf7e790: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1fffebf7e7a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1fffebf7e7b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1fffebf7e7c0: 00 00 00 00 00 00 00 00 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 Heap righ redzone: fb Freed Heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 ASan internal: fe ==24425==ABORTING | ||||
Tags | No tags attached. | ||||
|
This function is looking to re-implement similar behaviour to http://www.lua.org/source/5.1/liolib.c.html#g_read Particularly the "*l" branch as noted here http://pgl.yoyo.org/luai/i/file%3Aread |
|
Would appreciate some in depth testing of this patch by Lua scripters. Plenty of ways this could appear to work, while silently causing some failure to parse certain Lua scripts. |
|
fix-mantis-2819v2.patch (912 bytes)
Index: code/parse/lua.cpp =================================================================== --- code/parse/lua.cpp (revision 9668) +++ code/parse/lua.cpp (working copy) @@ -1196,15 +1196,21 @@ else if(!stricmp(fmt, "*l")) { char buf[10240]; + size_t i; if(cfgets(buf, (int)(sizeof(buf)/sizeof(char)), cfp) == NULL) { lua_pushnil(L); } else { - //WMC - strip all newlines so this works like the Lua original - char *pos = buf + strlen(buf); - while(*--pos == '\r' || *pos == '\n') *pos = '\0'; + // Strip all newlines so this works like the Lua original + // http://www.lua.org/source/5.1/liolib.c.html#g_read + // Note: we also strip carriage return in WMC's implementation + for (i = 0; i < strlen(buf); i++) + { + if ( buf[i] == '\n' || buf[i] == '\r' ) + buf[i] = '\0'; + } lua_pushstring(L, buf); } |
|
Revised patch attached, which addresses the signed/unsigned mismatch. |
|
\o/ Looks good. |
|
Fix committed to trunk@9671. |
Date Modified | Username | Field | Change |
---|---|---|---|
2013-03-23 04:03 | Echelon9 | New Issue | |
2013-03-26 11:23 | Echelon9 | Summary | ERROR: AddressSanitizer: stack-buffer-overflow in l_File_read_f() lua.cpp:1207 => AddressSanitizer: stack-buffer-overflow in l_File_read_f() lua.cpp:1207 |
2013-04-13 04:16 | Echelon9 | Assigned To | => Echelon9 |
2013-04-13 04:16 | Echelon9 | Status | new => assigned |
2013-04-30 11:56 | Echelon9 | Note Added: 0014999 | |
2013-04-30 12:44 | Echelon9 | File Added: fix-mantis-2819.patch | |
2013-04-30 12:45 | Echelon9 | Note Added: 0015000 | |
2013-04-30 12:45 | Echelon9 | Assigned To | Echelon9 => FUBAR-BDHR |
2013-04-30 12:45 | Echelon9 | Status | assigned => code review |
2013-04-30 15:14 | FUBAR-BDHR | Assigned To | FUBAR-BDHR => Echelon9 |
2013-04-30 15:14 | FUBAR-BDHR | Status | code review => assigned |
2013-04-30 15:14 | FUBAR-BDHR | Status | assigned => code review |
2013-05-04 00:30 | Echelon9 | File Deleted: fix-mantis-2819.patch | |
2013-05-04 00:30 | Echelon9 | File Added: fix-mantis-2819v2.patch | |
2013-05-04 00:31 | Echelon9 | Note Added: 0015024 | |
2013-05-04 02:01 | Zacam | Note Added: 0015025 | |
2013-05-04 06:26 | Echelon9 | Changeset attached | => fs2open trunk r9671 |
2013-05-04 06:26 | Echelon9 | Note Added: 0015029 | |
2013-05-04 06:26 | Echelon9 | Status | code review => resolved |
2013-05-04 06:26 | Echelon9 | Resolution | open => fixed |