All Projects FSSCP  wxFRED

View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0002899FSSCPpublic2013-07-05 12:112014-09-25 02:27
ReporterEchelon9 Assigned ToCommanderDJ  
PrioritynormalSeverityminorReproducibilityhave not tried
Status resolvedResolutionfixed 
Product Version3.7.0 RC2 
Target Version3.7.2Fixed in Version3.7.2 
Summary0002899: AddressSanitizer: heap-buffer-overflow in snd_play() sound.cpp
DescriptionReported by AddressSanitizer, a memory error detector for C/C++, in FS2Open builds based on trunk r9669 + BP.patch for WiH.

ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62d0000143f4 at pc 0x1023279b4 bp 0x7fff5fbf5f30 sp 0x7fff5fbf5f28
READ of size 4 at 0x62d0000143f4 thread T0
    #0 0x1023279b3 in snd_play sound.cpp:525
    0000001 0x10277f9da in l_Audio_playGameSound_f lua.cpp:11013
    0000002 0x102afd91f in luaD_precall ldo.c:320
    0000003 0x102beb801 in luaV_execute lvm.c:591
    0000004 0x102b037dc in luaD_call ldo.c:378
    0000005 0x102a92b9e in f_call lapi.c:800
    0000006 0x102af78a2 in luaD_rawrunprotected ldo.c:116
    0000007 0x102b067cb in luaD_pcall ldo.c:464
    0000008 0x102a924f6 in lua_pcall lapi.c:821
    0000009 0x10286d0c9 in script_state::RunBytecodeSub scripting.cpp:818
    0000010 0x102865401 in script_state::RunBytecode scripting.cpp:859
    #11 0x102864f2a in ConditionedHook::Run scripting.cpp:465
    0000012 0x10286e1c4 in script_state::RunCondition scripting.cpp:870
    0000013 0x10097e6d7 in gr_flip 2d.cpp:1544
    0000014 0x1002f2dd7 in game_flip_page_and_time_it freespace.cpp:3992
    0000015 0x1002fb9c3 in game_frame freespace.cpp:4616
    0000016 0x1002ffa9b in game_do_frame freespace.cpp:4915
    0000017 0x10030b5f6 in game_do_state freespace.cpp:6591
    0000018 0x100914f68 in gameseq_process_events gamesequence.cpp:405
    0000019 0x100311e96 in game_main freespace.cpp:7158
    0000020 0x100313534 in SDL_main freespace.cpp:7292
    ...
0x62d0000143f4 is located 12 bytes to the left of 36864-byte region [0x62d000014400,0x62d00001d400)
allocated by thread T0 here:
    #0 0x105a588f1 in wrap_malloc _asan_rtl_
    0000001 0x102626948 in _vm_malloc stubs.cpp:571
    0000002 0x10092c6aa in SCP_vm_allocator<game_snd>::allocate vmallocator.h:59
    0000003 0x10092b5f0 in std::_Vector_base<game_snd, SCP_vm_allocator<game_snd> >::_M_allocate stl_vector.h:131
    0000004 0x100929da1 in std::vector<game_snd, SCP_vm_allocator<game_snd> >::_M_insert_aux vector.tcc:271
    0000005 0x100926dd2 in std::vector<game_snd, SCP_vm_allocator<game_snd> >::push_back stl_vector.h:608
    0000006 0x1009231c5 in parse_sound_table gamesnd.cpp:715
    0000007 0x100923e2a in gamesnd_parse_soundstbl gamesnd.cpp:783
    0000008 0x1002ce0e4 in game_init freespace.cpp:1927
    0000009 0x100311959 in game_main freespace.cpp:7101
    0000010 0x100313534 in SDL_main freespace.cpp:7292

int snd_play( game_snd *gs, float pan, float vol_scale, int priority, bool is_voice_msg )
{
        ...

    if (gs == NULL) {
        Int3();
        return -1;
    }

    MONITOR_INC( NumSoundsStarted, 1 );

    if ( gs->id == -1 ) { <=========
        gs->id = snd_load(gs);
        MONITOR_INC( NumSoundsLoaded, 1);
    } else if ( gs->id_sig != Sounds[gs->id].sig ) {
        gs->id = snd_load(gs);
    }
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 and BP WiH. Reproducible in mission Everything is Permitted. Get noticed by the guards, being under radar lock warning, and act dead until crash occurs on sufficient weapon hits.
Additional InformationERROR: AddressSanitizer: heap-buffer-overflow on address 0x62d0000143f4 at pc 0x1023279b4 bp 0x7fff5fbf5f30 sp 0x7fff5fbf5f28
READ of size 4 at 0x62d0000143f4 thread T0
    #0 0x1023279b3 in snd_play sound.cpp:525
    0000001 0x10277f9da in l_Audio_playGameSound_f lua.cpp:11013
    0000002 0x102afd91f in luaD_precall ldo.c:320
    0000003 0x102beb801 in luaV_execute lvm.c:591
    0000004 0x102b037dc in luaD_call ldo.c:378
    0000005 0x102a92b9e in f_call lapi.c:800
    0000006 0x102af78a2 in luaD_rawrunprotected ldo.c:116
    0000007 0x102b067cb in luaD_pcall ldo.c:464
    0000008 0x102a924f6 in lua_pcall lapi.c:821
    0000009 0x10286d0c9 in script_state::RunBytecodeSub scripting.cpp:818
    0000010 0x102865401 in script_state::RunBytecode scripting.cpp:859
    #11 0x102864f2a in ConditionedHook::Run scripting.cpp:465
    0000012 0x10286e1c4 in script_state::RunCondition scripting.cpp:870
    0000013 0x10097e6d7 in gr_flip 2d.cpp:1544
    0000014 0x1002f2dd7 in game_flip_page_and_time_it freespace.cpp:3992
    0000015 0x1002fb9c3 in game_frame freespace.cpp:4616
    0000016 0x1002ffa9b in game_do_frame freespace.cpp:4915
    0000017 0x10030b5f6 in game_do_state freespace.cpp:6591
    0000018 0x100914f68 in gameseq_process_events gamesequence.cpp:405
    0000019 0x100311e96 in game_main freespace.cpp:7158
    0000020 0x100313534 in SDL_main freespace.cpp:7292
    0000021 0x100003203 in -[SDLMain applicationDidFinishLaunching:] SDLMain.m:300
    0000022 0x7fff95444ed9 in _CFXNotificationPost (in CoreFoundation) + 2553
    0000023 0x7fff8f5c67b5 in -[NSNotificationCenter postNotificationName:object:userInfo:] (in Foundation) + 63
    0000024 0x7fff98e5c52c in -[NSApplication _postDidFinishNotification] (in AppKit) + 291
    0000025 0x7fff98e5c265 in -[NSApplication _sendFinishLaunchingNotification] (in AppKit) + 215
    0000026 0x7fff98e59451 in -[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] (in AppKit) + 565
    0000027 0x7fff98e5904b in -[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] (in AppKit) + 350
    0000028 0x7fff8f5e007a in -[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] (in Foundation) + 307
    0000029 0x7fff8f5dfedc in _NSAppleEventManagerGenericHandler (in Foundation) + 105
    0000030 0x7fff94e91077 in aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) (in AE) + 306
    0000031 0x7fff94e90ed8 in dispatchEventAndSendReply(AEDesc const*, AEDesc*) (in AE) + 36
    0000032 0x7fff94e90d98 in aeProcessAppleEvent (in AE) + 317
    0000033 0x7fff93c24708 in AEProcessAppleEvent (in HIToolbox) + 99
    0000034 0x7fff98e55835 in _DPSNextEvent (in AppKit) + 1455
    0000035 0x7fff98e54df1 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 127
    0000036 0x7fff98e4c1a2 in -[NSApplication run] (in AppKit) + 516
    0000037 0x100005372 in CustomApplicationMain SDLMain.m:227
    0000038 0x100004ddf in main SDLMain.m:377
    0000039 0x100001f23 in start (in FS2_Open (debug)) + 51
    0000040 0x0 in 0x0
0x62d0000143f4 is located 12 bytes to the left of 36864-byte region [0x62d000014400,0x62d00001d400)
allocated by thread T0 here:
    #0 0x105a588f1 in wrap_malloc _asan_rtl_
    0000001 0x102626948 in _vm_malloc stubs.cpp:571
    0000002 0x10092c6aa in SCP_vm_allocator<game_snd>::allocate vmallocator.h:59
    0000003 0x10092b5f0 in std::_Vector_base<game_snd, SCP_vm_allocator<game_snd> >::_M_allocate stl_vector.h:131
    0000004 0x100929da1 in std::vector<game_snd, SCP_vm_allocator<game_snd> >::_M_insert_aux vector.tcc:271
    0000005 0x100926dd2 in std::vector<game_snd, SCP_vm_allocator<game_snd> >::push_back stl_vector.h:608
    0000006 0x1009231c5 in parse_sound_table gamesnd.cpp:715
    0000007 0x100923e2a in gamesnd_parse_soundstbl gamesnd.cpp:783
    0000008 0x1002ce0e4 in game_init freespace.cpp:1927
    0000009 0x100311959 in game_main freespace.cpp:7101
    0000010 0x100313534 in SDL_main freespace.cpp:7292
    #11 0x100003203 in -[SDLMain applicationDidFinishLaunching:] SDLMain.m:300
    0000012 0x7fff95444ed9 in _CFXNotificationPost (in CoreFoundation) + 2553
    0000013 0x7fff8f5c67b5 in -[NSNotificationCenter postNotificationName:object:userInfo:] (in Foundation) + 63
    0000014 0x7fff98e5c52c in -[NSApplication _postDidFinishNotification] (in AppKit) + 291
    0000015 0x7fff98e5c265 in -[NSApplication _sendFinishLaunchingNotification] (in AppKit) + 215
    0000016 0x7fff98e59451 in -[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] (in AppKit) + 565
    0000017 0x7fff98e5904b in -[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] (in AppKit) + 350
    0000018 0x7fff8f5e007a in -[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] (in Foundation) + 307
    0000019 0x7fff8f5dfedc in _NSAppleEventManagerGenericHandler (in Foundation) + 105
    0000020 0x7fff94e91077 in aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) (in AE) + 306
    0000021 0x7fff94e90ed8 in dispatchEventAndSendReply(AEDesc const*, AEDesc*) (in AE) + 36
    0000022 0x7fff94e90d98 in aeProcessAppleEvent (in AE) + 317
    0000023 0x7fff93c24708 in AEProcessAppleEvent (in HIToolbox) + 99
    0000024 0x7fff98e55835 in _DPSNextEvent (in AppKit) + 1455
    0000025 0x7fff98e54df1 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 127
    0000026 0x7fff98e4c1a2 in -[NSApplication run] (in AppKit) + 516
    0000027 0x100005372 in CustomApplicationMain SDLMain.m:227
    0000028 0x100004ddf in main SDLMain.m:377
    0000029 0x100001f23 in start (in FS2_Open (debug)) + 51
Shadow bytes around the buggy address:
  0x1c5a00002820: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c5a00002830: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c5a00002840: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c5a00002850: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c5a00002860: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x1c5a00002870: fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]fa
  0x1c5a00002880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c5a00002890: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c5a000028a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c5a000028b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c5a000028c0: 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 right 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
==1923==ABORTING
TagsNo tags attached.

Activities

Echelon9

2013-07-05 12:27

developer   ~0015155

Resolution patch ready for code review.
Was caused by a negative -1 in-band error reporting making its way to be an index into the Snds[] array.

m_m

2013-07-05 12:41

developer   ~0015156

Looks good, although I would recommend to add a LuaError in the else branch as an invalid sound index should be considered a script-error which would be dropped silently otherwise.

Echelon9

2013-07-05 13:09

developer  

fix-mantis-2899-v2.patch (882 bytes)    
Index: code/parse/lua.cpp
===================================================================
--- code/parse/lua.cpp	(revision 9699)
+++ code/parse/lua.cpp	(working copy)
@@ -11010,9 +11010,13 @@
     CLAMP(pan, -1.0f, 1.0f);
     CLAMP(vol, 0.0f, 100.0f);
 
-	idx = snd_play(&Snds[gamesnd_get_by_tbl_index(idx)], pan, vol*0.01f, pri, voice_msg);
-
-	return ade_set_args(L, "b", idx > -1);
+	if (gamesnd_get_by_tbl_index(idx) >= 0) {
+		idx = snd_play(&Snds[gamesnd_get_by_tbl_index(idx)], pan, vol*0.01f, pri, voice_msg);
+		return ade_set_args(L, "b", idx > -1);
+	} else {
+		LuaError(L, "Invalid sound index %i (Snds[%i]) in playGameSound()", idx, gamesnd_get_by_tbl_index(idx));
+		return ADE_RETURN_FALSE;
+	}
 }
 
 ADE_FUNC(playInterfaceSound, l_Audio, "Sound index", "Plays a sound from #Interface Sounds in sounds.tbl", "boolean", "True if sound was played, false if not")
fix-mantis-2899-v2.patch (882 bytes)   

Echelon9

2013-07-05 13:10

developer   ~0015157

From IRC #scp:

[12:10am] Echelon9: actually, I'm not so sure
[12:10am] Echelon9: gamesnd_get_by_tbl_index() seems really odd
[12:11am] Echelon9: although it says the lookup is by "tbl_index", it converts the integer to a string and then does a name lookup?!?
[12:11am] Echelon9: it in fact looks like a direct numerical index into Snds[] is being passed as first argument to playGameSound
[12:12am] Echelon9: which would appear consistent with the function description
[12:12am] Echelon9: similar uncertainty between description and implementation applies to gamesnd_get_by_iface_tbl_index()

m_m

2013-07-05 13:24

developer   ~0015158

The problem with gamesnd_get_by_tbl_index() is that it merely exists for legacy support to make the transition from indexed sounds to named sounds easier and to not break existing missions and scripts as the function is only used to look up sounds indexes in these parts of the code.

Echelon9

2013-07-06 01:54

developer   ~0015159

Yes, but isn't the issue the inconsistency with the Lua API description and a differing internal implementation?

The Lua exposed playGameSound() documentation states the argument is a "Sound index" -- to note this appears to be the way WiH has used the Lua API call.
The implementation uses the variable as a name lookup

I'm not clear how this ever worked?

m_m

2013-07-06 10:35

developer   ~0015160

It worked because it is only there to maintain backwards-compatibility. When the game_snd code was changed to use names instead of ints for sound identifying the numbers were used as names so the present references in other tables would not get invalid and the same was needed for the present scripting functions.

The documentation should probably be updated to note that the function is only present for backwards-compatibility and the newer getSoundentry() API should be used for future work.

Echelon9

2013-07-07 03:41

developer   ~0015161

Thanks for that clarification, makes it much clearer.

On that basis though, we should be marking the Lua API documentation for that function with DEPRECATED somehow.

Also, while it would be nice to use a LuaError(), a hard error, because it has been used in a number of mods as is, I think it would be inappropriate to adopt now. i.e. WiH would stop working with trunk builds.

I think this is an unfortunate case of code deficit we will have to keep.

m_m

2013-07-07 06:52

developer   ~0015162

I didn't realize that this error already happens in released code. Then we will have to use a simple log message along with a modified documentation.

Goober5000

2013-08-09 17:20

administrator   ~0015216

Someone (CommanderDJ perhaps?) is going to need to clarify the reasoning behind the current API. I know there was a discussion about the way to handle integer indexing, but I don't remember the details or reasoning. It does seem counterintuitive to use the integer as the sound index "name" rather than an offset into the sounds array.

m_m

2013-08-10 09:32

developer   ~0015220

This decision was made in order to keep old missions and scripts compatible with the new gamesnd API. A documentation comment noting that these functions should not be used or renaming the functions could also be appropriate but the functionality of these functions must be preserved.

Goober5000

2014-03-26 01:32

administrator   ~0015682

*bump*

Has this been fixed in trunk?

Echelon9

2014-03-26 11:15

developer   ~0015688

Not that I am aware of.

CommanderDJ

2014-06-09 08:16

developer   ~0015825

I've been a little out of the loop, and I don't know if I'm really qualified to comment on this. I recall that jg18 was doing a comprehensive review on the sound code - maybe it would be prudent to draw this to his attention?

Echelon9

2014-08-17 03:07

developer   ~0016225

Most probably best a sound coder reviews, as there is functionality change to fix this properly (I think)

Goober5000

2014-09-25 02:26

administrator   ~0016299

Almost all of the comments on this ticket (including mine) have been about understanding the change from integer-based lookup to name-based lookup. With that resolved, I've examined at the patch, and it looks good. I've also added complementary behavior to playInterfaceSound which appears right below it.

Goober5000

2014-09-25 02:27

administrator   ~0016300

Patch committed.

Related Changesets

fs2open: trunk r11083

2014-09-24 23:03

Goober5000


Ported: N/A

Details Diff		 CommanderDJ's patch for Mantis 0002899 (AddressSanitizer: heap-buffer-overflow in snd_play() sound.cpp) Affected Issues
0002899
mod - /trunk/fs2_open/code/parse/lua.cpp Diff File

Issue History

Date Modified Username Field Change
2013-07-05 12:11 Echelon9 New Issue
2013-07-05 12:25 Echelon9 Assigned To => Echelon9
2013-07-05 12:25 Echelon9 Status new => assigned
2013-07-05 12:26 Echelon9 File Added: fix-mantis-2899.patch
2013-07-05 12:27 Echelon9 Note Added: 0015155
2013-07-05 12:27 Echelon9 Status assigned => code review
2013-07-05 12:41 m_m Note Added: 0015156
2013-07-05 13:09 Echelon9 File Deleted: fix-mantis-2899.patch
2013-07-05 13:09 Echelon9 File Added: fix-mantis-2899-v2.patch
2013-07-05 13:10 Echelon9 Note Added: 0015157
2013-07-05 13:24 m_m Note Added: 0015158
2013-07-06 01:54 Echelon9 Note Added: 0015159
2013-07-06 10:35 m_m Note Added: 0015160
2013-07-07 03:41 Echelon9 Note Added: 0015161
2013-07-07 06:52 m_m Note Added: 0015162
2013-08-09 17:20 Goober5000 Note Added: 0015216
2013-08-10 09:32 m_m Note Added: 0015220
2013-08-14 02:48 Echelon9 Assigned To Echelon9 => CommanderDJ
2013-08-14 02:48 Echelon9 Status code review => assigned
2014-03-26 01:32 Goober5000 Note Added: 0015682
2014-03-26 11:15 Echelon9 Note Added: 0015688
2014-06-09 08:16 CommanderDJ Note Added: 0015825
2014-06-29 23:10 Goober5000 Target Version 3.7.0 => 3.7.2
2014-08-17 03:07 Echelon9 Note Added: 0016225
2014-09-25 02:26 Goober5000 Note Added: 0016299
2014-09-25 02:27 Goober5000 Changeset attached => fs2open trunk r11083
2014-09-25 02:27 Goober5000 Note Added: 0016300
2014-09-25 02:27 Goober5000 Status assigned => resolved
2014-09-25 02:27 Goober5000 Resolution open => fixed
2014-09-25 02:27 Goober5000 Fixed in Version => 3.7.2