View Issue Details

IDProjectCategoryView StatusLast Update
0002818FSSCPpublic2013-03-26 08:34
ReporterEchelon9 Assigned ToEchelon9  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.6.19 
Target Version3.7.0Fixed in Version3.7.0 
Summary0002818: ERROR: AddressSanitizer: global-buffer-overflow in bm_is_valid() bmpman.cpp
DescriptionReported 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 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: 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
TagsNo tags attached.

Activities

Echelon9

2013-03-24 02:17

developer  

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);
 }
 

Echelon9

2013-03-24 02:17

developer   ~0014818

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.

chief1983

2013-03-24 02:24

administrator   ~0014819

This looks nice and neat, go for it.

Echelon9

2013-03-24 02:32

developer   ~0014820

Fix committed in r9596.

chief1983

2013-03-25 15:00

administrator   ~0014833

This ended up leading to a lot of failed assertions in debug. Apparently -1 is a default value or something.

Echelon9

2013-03-26 00:28

developer   ~0014834

Last edited: 2013-03-26 00:38

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?

chief1983

2013-03-26 00:37

administrator   ~0014835

Ah. I'd check with zookeeper then, FotG was throwing asserts every mission I loaded, and he wrote most of our lua stuff.

Echelon9

2013-03-26 00:40

developer   ~0014836

I'd suggest taking a look at those FotG scripts.

Media VPs triggered no such Assert with the extra validation checking in place.

chief1983

2013-03-26 00:47

administrator   ~0014837

OK I'll do that but we probably aren't the only mod with an issue so i'd consider a warning.

Echelon9

2013-03-26 01:13

developer   ~0014838

Okay, I'll recut the patch with a Warning()

niffiwan

2013-03-26 08:29

developer   ~0014840

Last edited: 2013-03-26 08:30

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?

Echelon9

2013-03-26 08:34

developer   ~0014841

Revised patch, using a Warning(), committed in r9601.

Issue History

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