View Issue Details

IDProjectCategoryView StatusLast Update
0002819FSSCPpublic2013-05-04 06:26
ReporterEchelon9 Assigned ToEchelon9  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.6.19 
Target Version3.7.0 
Summary0002819: AddressSanitizer: stack-buffer-overflow in l_File_read_f() lua.cpp:1207
DescriptionReported 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 Reproduce1. 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
TagsNo tags attached.

Activities

Echelon9

2013-04-30 11:56

developer   ~0014999

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

Echelon9

2013-04-30 12:45

developer   ~0015000

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.

Echelon9

2013-05-04 00:30

developer  

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);
 				}
fix-mantis-2819v2.patch (912 bytes)   

Echelon9

2013-05-04 00:31

developer   ~0015024

Revised patch attached, which addresses the signed/unsigned mismatch.

Zacam

2013-05-04 02:01

administrator   ~0015025

\o/

Looks good.

Echelon9

2013-05-04 06:26

developer   ~0015029

Fix committed to trunk@9671.

Related Changesets

fs2open: trunk r9671

2013-05-04 03:19

Echelon9


Ported: N/A

Details Diff
Fix Mantis 2819: AddressSanitizer: stack-buffer-overflow in l_File_read_f() Affected Issues
0002819
mod - /trunk/fs2_open/code/parse/lua.cpp Diff File

Issue History

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