View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002818 | FSSCP | public | 2013-03-23 03:52 | 2013-03-26 08:34 | |
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 | Fixed in Version | 3.7.0 | ||
Summary | 0002818: ERROR: AddressSanitizer: global-buffer-overflow in bm_is_valid() bmpman.cpp | ||||
Description | Reported by AddressSanitizer, a memory error detector for C/C++, in FS2Open builds based on trunk r9585. ERROR: AddressSanitizer: global-buffer-overflow on address 0x000103497a18 at pc 0x100726aa3 bp 0x7fff5fbf6230 sp 0x7fff5fbf6228 READ of size 4 at 0x000103497a18 thread T0 #0 0x100726aa2 in bm_is_valid bmpman.cpp:1098 0000001 0x10262cabe in l_Texture_isValid_f lua.cpp:3495 0000002 0x102a7e6e1 in luaD_precall ldo.c:320 0000003 0x102b6ad81 in luaV_execute lvm.c:591 0000004 0x102a8454d in luaD_call ldo.c:378 0000005 0x102a14760 in f_call lapi.c:800 ... 1095: int bm_is_valid(int handle) 1096: { 1097: if(!bm_inited) return 0; 1098: return (bm_bitmaps[handle % MAX_BITMAPS].handle == handle); 1099: } | ||||
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: global-buffer-overflow on address 0x000103497a18 at pc 0x100726aa3 bp 0x7fff5fbf6230 sp 0x7fff5fbf6228 READ of size 4 at 0x000103497a18 thread T0 #0 0x100726aa2 in bm_is_valid bmpman.cpp:1098 0000001 0x10262cabe in l_Texture_isValid_f lua.cpp:3495 0000002 0x102a7e6e1 in luaD_precall ldo.c:320 0000003 0x102b6ad81 in luaV_execute lvm.c:591 0000004 0x102a8454d in luaD_call ldo.c:378 0000005 0x102a14760 in f_call lapi.c:800 0000006 0x102a786e0 in luaD_rawrunprotected ldo.c:116 0000007 0x102a87496 in luaD_pcall ldo.c:464 0000008 0x102a140c5 in lua_pcall lapi.c:821 0000009 0x1027f3a46 in script_state::RunBytecodeSub scripting.cpp:818 0000010 0x1027ebdd2 in script_state::RunBytecode scripting.cpp:859 #11 0x1027eb908 in ConditionedHook::Run scripting.cpp:465 0000012 0x1027f4b82 in script_state::RunCondition scripting.cpp:870 0000013 0x1002be03c in game_post_level_init freespace.cpp:1409 0000014 0x1002befdc in game_start_mission freespace.cpp:1466 0000015 0x1002fc477 in game_enter_state freespace.cpp:5934 0000016 0x1008efb8e in gameseq_set_state gamesequence.cpp:280 0000017 0x1002f7162 in game_process_event freespace.cpp:5105 0000018 0x1008f13df in gameseq_process_events gamesequence.cpp:395 0000019 0x100306528 in game_main freespace.cpp:7045 0000020 0x100307b96 in SDL_main freespace.cpp:7179 0000021 0x10000335a in -[SDLMain applicationDidFinishLaunching:] SDLMain.m:300 0000022 0x7fff8ffe3ed9 in _CFXNotificationPost (in CoreFoundation) + 2553 0000023 0x7fff85fc6e25 in -[NSNotificationCenter postNotificationName:object:userInfo:] (in Foundation) + 63 0000024 0x7fff8b92855c in -[NSApplication _postDidFinishNotification] (in AppKit) + 291 0000025 0x7fff8b928295 in -[NSApplication _sendFinishLaunchingNotification] (in AppKit) + 215 0000026 0x7fff8b925481 in -[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] (in AppKit) + 565 0000027 0x7fff8b92507b in -[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] (in AppKit) + 350 0000028 0x7fff85fe070a in -[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] (in Foundation) + 307 0000029 0x7fff85fe056c in _NSAppleEventManagerGenericHandler (in Foundation) + 105 0000030 0x7fff8e3a9077 in aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) (in AE) + 306 0000031 0x7fff8e3a8ed8 in dispatchEventAndSendReply(AEDesc const*, AEDesc*) (in AE) + 36 0000032 0x7fff8e3a8d98 in aeProcessAppleEvent (in AE) + 317 0000033 0x7fff88b11708 in AEProcessAppleEvent (in HIToolbox) + 99 0000034 0x7fff8b921865 in _DPSNextEvent (in AppKit) + 1455 0000035 0x7fff8b920e21 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 127 0000036 0x7fff8b9181d2 in -[NSApplication run] (in AppKit) + 516 0000037 0x100005473 in CustomApplicationMain SDLMain.m:227 0000038 0x100004ef0 in main SDLMain.m:377 0000039 0x1000020c3 in start (in FS2_Open (debug)) + 51 0000040 0x0 in 0x0 0x000103497a18 is located 8 bytes to the left of global variable 'AutopilotMinAsteroidDistance' from 'fs2open/trunk/fs2_open/projects/Xcode4/../../code/autopilot/autopilot.cpp' (0x103497a20) of size 4 0x000103497a18 is located 52 bytes to the right of global variable 'AutopilotMinEnemyDistance' from 'fs2open/trunk/fs2_open/projects/Xcode4/../../code/autopilot/autopilot.cpp' (0x1034979e0) of size 4 Shadow bytes around the buggy address: 0x100020692ef0: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 0x100020692f00: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 0x100020692f10: 01 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 0x100020692f20: 00 04 f9 f9 f9 f9 f9 f9 00 04 f9 f9 f9 f9 f9 f9 0x100020692f30: 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 =>0x100020692f40: f9 f9 f9[f9]04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 0x100020692f50: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00 0x100020692f60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100020692f70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100020692f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100020692f90: 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 ==14313==ABORTING | ||||
Tags | No tags attached. | ||||
|
fix-for-mantis-2818-bm_is_valid.patch (1,114 bytes)
Index: code/bmpman/bmpman.cpp =================================================================== --- code/bmpman/bmpman.cpp (revision 9593) +++ code/bmpman/bmpman.cpp (working copy) @@ -1092,9 +1092,25 @@ return tidx; } +/** + * Sanity check to ensure invalid bitmap handles are picked up early + * + * Some early known false or out of range handles (such as negative) are checked within + * an initial routine pass, followed by a more thorough check routine to ensure the + * series of bitmaps within the bm_bitmaps[] structure have not become invalid. + * + * @param handle Bitmap handle to check for validity + * @return 1 for valid, 0 for invalid + */ int bm_is_valid(int handle) { + Assertion( handle >= 0, "Out of range bitmap handle %d passed to bm_is_valid().\n", handle ); + + // Ensure that certain known false or out of range handles are quickly returned as invalid, + // prior to utilising the handle in a way which leads to memory access outside bm_bitmaps[] if(!bm_inited) return 0; + if(handle < 0) return 0; + return (bm_bitmaps[handle % MAX_BITMAPS].handle == handle); } |
|
This is caused by the absence of an initial routine pass within bm_is_valid() for known false or out of range handles (such as a negative handle value), prior to a more thorough check routine to ensure the series of bitmaps within the bm_bitmaps[] structure have not become invalid. Ironically, under the previous code, a negative handle would cause memory read access outside of the bm_bitmaps[] array, in and of itself leading to undefined behaviour. |
|
This looks nice and neat, go for it. |
|
Fix committed in r9596. |
|
This ended up leading to a lot of failed assertions in debug. Apparently -1 is a default value or something. |
|
Well not quite, in this case Debug is working correctly. If the handle being tested for validity is -1, then prima facie the handle has to be invalid. The correct fix in this scenario is to properly hunt through the Lua code that is causing invalid handles, and to then fix that instead. The bm_is_valid() deliberately returns 0 on Release for an invalid handle, but developers should want Debug to assert sufficiently on invalid data or invalid Lua scripts. So perhaps the discussion should be: Do we want it to call an Assertion() or a Warning() upon encountering an obviously invalid handle in Debug? |
|
Ah. I'd check with zookeeper then, FotG was throwing asserts every mission I loaded, and he wrote most of our lua stuff. |
|
I'd suggest taking a look at those FotG scripts. Media VPs triggered no such Assert with the extra validation checking in place. |
|
OK I'll do that but we probably aren't the only mod with an issue so i'd consider a warning. |
|
Okay, I'll recut the patch with a Warning() |
|
Please note, I'm getting this assertion while trying to test with Silent Threat & ST:R campaigns. ASSERTION FAILED: "handle >= 0" at bmpman/bmpman.cpp:1107 Out of range bitmap handle -1 passed to bm_is_valid(). (gdb) bt #0 0x00007ffff58ca425 in __GI_raise (sig=<optimised out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 0000001 0x00007ffff58cdb8b in __GI_abort () at abort.c:91 0000002 0x0000000000876227 in WinAssert (text=0x8ef940 "handle >= 0", filename=0x8ef348 "bmpman/bmpman.cpp", line=1107, format=0x8ef908 "Out of range bitmap handle %d passed to bm_is_valid().\n") at windows_stub/stubs.cpp:161 0000003 0x0000000000474070 in bm_is_valid (handle=-1) at bmpman/bmpman.cpp:1107 0000004 0x0000000000478451 in bm_get_filename (bitmapnum=-1, filename=0x7fffffffd090 "capital01-02.dds") at bmpman/bmpman.cpp:2533 0000005 0x000000000065236c in texture_map::FindTexture (this=0x376e0a4, fname=0x3e22034 "capital01-05a") at model/modelinterp.cpp:4956 0000006 0x00000000005f32e4 in parse_create_object_sub (p_objp=0x3e21e20) at mission/missionparse.cpp:1933 0000007 0x00000000005f279d in parse_create_object (pobjp=0x3e21e20) at mission/missionparse.cpp:1728 0000008 0x00000000005f7ab9 in mission_parse_maybe_create_parse_object (pobjp=0x3e21e20) at mission/missionparse.cpp:3347 0000009 0x00000000005fabfb in post_process_ships_wings () at mission/missionparse.cpp:4589 0000010 0x00000000005fd3ba in post_process_mission () at mission/missionparse.cpp:5499 #11 0x00000000005fd2cb in parse_mission (pm=0x10496e0, flags=0) at mission/missionparse.cpp:5477 0000012 0x00000000005fe3a4 in parse_main (mission_name=0x7fffffffde20 "str01.fs2", flags=0) at mission/missionparse.cpp:5852 0000013 0x00000000005e3212 in mission_load (filename_ext=0xc912a0 "str01.fs2") at mission/missionload.cpp:107 0000014 0x000000000040babf in game_start_mission () at freespace2/freespace.cpp:1442 0000015 0x000000000041571f in game_enter_state (old_state=1, new_state=52) at freespace2/freespace.cpp:5934 0000016 0x00000000004bc381 in gameseq_set_state (new_state=52, override=0) at gamesequence/gamesequence.cpp:280 0000017 0x0000000000414584 in game_process_event (current_state=1, event=1) at freespace2/freespace.cpp:5105 0000018 0x00000000004bc865 in gameseq_process_events () at gamesequence/gamesequence.cpp:395 0000019 0x0000000000416eed in game_main (cmdline=0x24e9880 "") at freespace2/freespace.cpp:7045 0000020 0x00000000004170ed in main (argc=1, argv=0x7fffffffe268) at freespace2/freespace.cpp:7179 (gdb) So I guess this means that FSO can't find capital01-02.dds? However, from reading the comments at model/model.h, it seems that this texture (glow in this case) is optional, so by design these are allowed to be missing, and by design their handle is set to -1. For this model, only base, specular and normal textures are being used. #define TM_BASE_TYPE 0 // the standard base map #define TM_GLOW_TYPE 1 // optional glow map #define TM_SPECULAR_TYPE 2 // optional specular map #define TM_NORMAL_TYPE 3 // optional normal map #define TM_HEIGHT_TYPE 4 // optional height map (for parallax mapping) #define TM_MISC_TYPE 5 // optional utility map #define TM_NUM_TYPES 6 //WMC - Number of texture_info objects in texture_map Anyway, -1 *is* a valid "missing bitmap" setting, but obviously not one we want to bash into the bm_bitmaps array. Do we just remove the assertion/warning completely? Or perhaps, handle -1 as a special case, but assert/warn on other negative values? |
|
Revised patch, using a Warning(), committed in r9601. |
Date Modified | Username | Field | Change |
---|---|---|---|
2013-03-23 03:52 | Echelon9 | New Issue | |
2013-03-23 03:54 | Echelon9 | Additional Information Updated | |
2013-03-23 03:58 | Echelon9 | Steps to Reproduce Updated | |
2013-03-24 02:17 | Echelon9 | File Added: fix-for-mantis-2818-bm_is_valid.patch | |
2013-03-24 02:17 | Echelon9 | Note Added: 0014818 | |
2013-03-24 02:17 | Echelon9 | Assigned To | => chief1983 |
2013-03-24 02:17 | Echelon9 | Status | new => code review |
2013-03-24 02:24 | chief1983 | Note Added: 0014819 | |
2013-03-24 02:30 | Echelon9 | Assigned To | chief1983 => Echelon9 |
2013-03-24 02:30 | Echelon9 | Status | code review => assigned |
2013-03-24 02:32 | Echelon9 | Note Added: 0014820 | |
2013-03-24 02:32 | Echelon9 | Status | assigned => resolved |
2013-03-24 02:32 | Echelon9 | Fixed in Version | => 3.7.0 |
2013-03-24 02:32 | Echelon9 | Resolution | open => fixed |
2013-03-25 15:00 | chief1983 | Note Added: 0014833 | |
2013-03-25 15:00 | chief1983 | Status | resolved => acknowledged |
2013-03-26 00:28 | Echelon9 | Note Added: 0014834 | |
2013-03-26 00:31 | Echelon9 | Note Edited: 0014834 | |
2013-03-26 00:37 | chief1983 | Note Added: 0014835 | |
2013-03-26 00:38 | Echelon9 | Note Edited: 0014834 | |
2013-03-26 00:40 | Echelon9 | Note Added: 0014836 | |
2013-03-26 00:47 | chief1983 | Note Added: 0014837 | |
2013-03-26 01:13 | Echelon9 | Note Added: 0014838 | |
2013-03-26 08:29 | niffiwan | Note Added: 0014840 | |
2013-03-26 08:30 | niffiwan | Note Edited: 0014840 | |
2013-03-26 08:34 | Echelon9 | Note Added: 0014841 | |
2013-03-26 08:34 | Echelon9 | Status | acknowledged => resolved |