All Projects FSSCP  wxFRED

View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0002837FSSCPpublic2013-04-04 07:432013-05-10 13:52
ReporterEchelon9 Assigned ToEchelon9  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.6.19 
Target Version3.7.0 
Summary0002837: AddressSanitizer: heap-buffer-overflow in sexp_get_damage_caused() sexp.cpp:7480
DescriptionReported by AddressSanitizer, a memory error detector for C/C++, in FS2Open builds based on trunk r9620 and BP.patch in WiH2.

ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62c0001a00e0 at pc 0x101c331ae bp 0x7fff5fbf61f0 sp 0x7fff5fbf61e8
READ of size 4 at 0x62c0001a00e0 thread T0
    #0 0x101c331ad in sexp_get_damage_caused sexp.cpp:7480
    0000001 0x101bc14d6 in eval_sexp sexp.cpp:22030
    0000002 0x101bbaad0 in eval_sexp sexp.cpp:21593
    0000003 0x101be7142 in sexp_number_compare sexp.cpp:4388
    0000004 0x101bbcc3d in eval_sexp sexp.cpp:21714
    0000005 0x101be1c02 in is_sexp_true sexp.cpp:21374
    0000006 0x101be2337 in sexp_and sexp.cpp:4255
    0000007 0x101bbc90a in eval_sexp sexp.cpp:21697
    0000008 0x101c43b1d in eval_when sexp.cpp:8319
    0000009 0x101bc196a in eval_sexp sexp.cpp:22054
    0000010 0x101290f38 in mission_process_event missiongoals.cpp:925
    #11 0x1012952fa in mission_eval_goals missiongoals.cpp:1072
    0000012 0x1002ef0c2 in game_simulation_frame freespace.cpp:4115
    0000013 0x1002f3d11 in game_frame freespace.cpp:4506
    0000014 0x1002f94fb in game_do_frame freespace.cpp:4917
    0000015 0x10030505a in game_do_state freespace.cpp:6592
    0000016 0x100904a59 in gameseq_process_events gamesequence.cpp:405
    0000017 0x10030b8d8 in game_main freespace.cpp:7159
    0000018 0x10030cf46 in SDL_main freespace.cpp:7293
    ...
0x62c0001a00e0 is located 288 bytes to the left of 32000-byte region [0x62c0001a0200,0x62c0001a7f00)
allocated by thread T0 here:
    #0 0x105b146d3 in wrap_malloc (in libclang_rt.asan_osx_dynamic.dylib) + 51
    0000001 0x1025e745f in _vm_malloc stubs.cpp:571
    0000002 0x10222776f in SCP_vm_allocator<exited_ship>::allocate vmallocator.h:59
    0000003 0x102225e14 in std::_Vector_base<exited_ship, SCP_vm_allocator<exited_ship> >::_M_allocate stl_vector.h:131
    0000004 0x102228ca1 in exited_ship* std::vector<exited_ship, SCP_vm_allocator<exited_ship> >::_M_allocate_and_copy<exited_ship*> stl_vector.h:766
    0000005 0x1021c4887 in std::vector<exited_ship, SCP_vm_allocator<exited_ship> >::reserve vector.tcc:76
    0000006 0x102064df2 in ship_level_init ship.cpp:4483
    0000007 0x102064cc5 in ship_init ship.cpp:4401
    0000008 0x1002c7fc1 in game_init freespace.cpp:1962
    0000009 0x10030b39b in game_main freespace.cpp:7102
    0000010 0x10030cf46 in SDL_main freespace.cpp:7293
    ...


    // go through the list of ships who we think may have attacked the ship
    for ( ; node != -1; node = CDR(node) ) {
        name = CTEXT(node);
        sindex = ship_name_lookup(name);
        if (sindex < 0) {
            sindex = ship_find_exited_ship_by_name(name);
            attacker_sig = Ships_exited[sindex].obj_signature; <==
        }
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 WiH2, progress through to flight in 'Everything in Permitted'
Additional InformationERROR: AddressSanitizer: heap-buffer-overflow on address 0x62c0001a00e0 at pc 0x101c331ae bp 0x7fff5fbf61f0 sp 0x7fff5fbf61e8
READ of size 4 at 0x62c0001a00e0 thread T0
    #0 0x101c331ad in sexp_get_damage_caused sexp.cpp:7480
    0000001 0x101bc14d6 in eval_sexp sexp.cpp:22030
    0000002 0x101bbaad0 in eval_sexp sexp.cpp:21593
    0000003 0x101be7142 in sexp_number_compare sexp.cpp:4388
    0000004 0x101bbcc3d in eval_sexp sexp.cpp:21714
    0000005 0x101be1c02 in is_sexp_true sexp.cpp:21374
    0000006 0x101be2337 in sexp_and sexp.cpp:4255
    0000007 0x101bbc90a in eval_sexp sexp.cpp:21697
    0000008 0x101c43b1d in eval_when sexp.cpp:8319
    0000009 0x101bc196a in eval_sexp sexp.cpp:22054
    0000010 0x101290f38 in mission_process_event missiongoals.cpp:925
    #11 0x1012952fa in mission_eval_goals missiongoals.cpp:1072
    0000012 0x1002ef0c2 in game_simulation_frame freespace.cpp:4115
    0000013 0x1002f3d11 in game_frame freespace.cpp:4506
    0000014 0x1002f94fb in game_do_frame freespace.cpp:4917
    0000015 0x10030505a in game_do_state freespace.cpp:6592
    0000016 0x100904a59 in gameseq_process_events gamesequence.cpp:405
    0000017 0x10030b8d8 in game_main freespace.cpp:7159
    0000018 0x10030cf46 in SDL_main freespace.cpp:7293
    0000019 0x100002d2a in -[SDLMain applicationDidFinishLaunching:] SDLMain.m:300
    0000020 0x7fff9502aed9 in _CFXNotificationPost (in CoreFoundation) + 2553
    0000021 0x7fff8b00de25 in -[NSNotificationCenter postNotificationName:object:userInfo:] (in Foundation) + 63
    0000022 0x7fff9096f55c in -[NSApplication _postDidFinishNotification] (in AppKit) + 291
    0000023 0x7fff9096f295 in -[NSApplication _sendFinishLaunchingNotification] (in AppKit) + 215
    0000024 0x7fff9096c481 in -[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] (in AppKit) + 565
    0000025 0x7fff9096c07b in -[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] (in AppKit) + 350
    0000026 0x7fff8b02770a in -[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] (in Foundation) + 307
    0000027 0x7fff8b02756c in _NSAppleEventManagerGenericHandler (in Foundation) + 105
    0000028 0x7fff933f0077 in aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) (in AE) + 306
    0000029 0x7fff933efed8 in dispatchEventAndSendReply(AEDesc const*, AEDesc*) (in AE) + 36
    0000030 0x7fff933efd98 in aeProcessAppleEvent (in AE) + 317
    0000031 0x7fff8db58708 in AEProcessAppleEvent (in HIToolbox) + 99
    0000032 0x7fff90968865 in _DPSNextEvent (in AppKit) + 1455
    0000033 0x7fff90967e21 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 127
    0000034 0x7fff9095f1d2 in -[NSApplication run] (in AppKit) + 516
    0000035 0x100004e43 in CustomApplicationMain SDLMain.m:227
    0000036 0x1000048c0 in main SDLMain.m:377
    0000037 0x100001a93 in start (in FS2_Open (debug)) + 51
    0000038 0x0 in 0x0
0x62c0001a00e0 is located 288 bytes to the left of 32000-byte region [0x62c0001a0200,0x62c0001a7f00)
allocated by thread T0 here:
    #0 0x105b146d3 in wrap_malloc (in libclang_rt.asan_osx_dynamic.dylib) + 51
    0000001 0x1025e745f in _vm_malloc stubs.cpp:571
    0000002 0x10222776f in SCP_vm_allocator<exited_ship>::allocate vmallocator.h:59
    0000003 0x102225e14 in std::_Vector_base<exited_ship, SCP_vm_allocator<exited_ship> >::_M_allocate stl_vector.h:131
    0000004 0x102228ca1 in exited_ship* std::vector<exited_ship, SCP_vm_allocator<exited_ship> >::_M_allocate_and_copy<exited_ship*> stl_vector.h:766
    0000005 0x1021c4887 in std::vector<exited_ship, SCP_vm_allocator<exited_ship> >::reserve vector.tcc:76
    0000006 0x102064df2 in ship_level_init ship.cpp:4483
    0000007 0x102064cc5 in ship_init ship.cpp:4401
    0000008 0x1002c7fc1 in game_init freespace.cpp:1962
    0000009 0x10030b39b in game_main freespace.cpp:7102
    0000010 0x10030cf46 in SDL_main freespace.cpp:7293
    #11 0x100002d2a in -[SDLMain applicationDidFinishLaunching:] SDLMain.m:300
    0000012 0x7fff9502aed9 in _CFXNotificationPost (in CoreFoundation) + 2553
    0000013 0x7fff8b00de25 in -[NSNotificationCenter postNotificationName:object:userInfo:] (in Foundation) + 63
    0000014 0x7fff9096f55c in -[NSApplication _postDidFinishNotification] (in AppKit) + 291
    0000015 0x7fff9096f295 in -[NSApplication _sendFinishLaunchingNotification] (in AppKit) + 215
    0000016 0x7fff9096c481 in -[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] (in AppKit) + 565
    0000017 0x7fff9096c07b in -[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] (in AppKit) + 350
    0000018 0x7fff8b02770a in -[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] (in Foundation) + 307
    0000019 0x7fff8b02756c in _NSAppleEventManagerGenericHandler (in Foundation) + 105
    0000020 0x7fff933f0077 in aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) (in AE) + 306
    0000021 0x7fff933efed8 in dispatchEventAndSendReply(AEDesc const*, AEDesc*) (in AE) + 36
    0000022 0x7fff933efd98 in aeProcessAppleEvent (in AE) + 317
    0000023 0x7fff8db58708 in AEProcessAppleEvent (in HIToolbox) + 99
    0000024 0x7fff90968865 in _DPSNextEvent (in AppKit) + 1455
    0000025 0x7fff90967e21 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 127
    0000026 0x7fff9095f1d2 in -[NSApplication run] (in AppKit) + 516
    0000027 0x100004e43 in CustomApplicationMain SDLMain.m:227
    0000028 0x1000048c0 in main SDLMain.m:377
    0000029 0x100001a93 in start (in FS2_Open (debug)) + 51
Shadow bytes around the buggy address:
  0x1c5800033fc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c5800033fd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c5800033fe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c5800033ff0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c5800034000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x1c5800034010: fa fa fa fa fa fa fa fa fa fa fa fa[fa]fa fa fa
  0x1c5800034020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c5800034030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c5800034040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c5800034050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c5800034060: 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
==4677==ABORTING
TagsNo tags attached.

Activities

Echelon9

2013-04-04 07:48

developer  

fix-mantis-2837.patch (855 bytes)    
Index: parse/sexp.cpp
===================================================================
--- parse/sexp.cpp	(revision 9621)
+++ parse/sexp.cpp	(working copy)
@@ -7441,7 +7441,7 @@
 }
 
 // Karajorma
-int sexp_get_damage_caused(int node) 
+int sexp_get_damage_caused(int node)
 {
 	int sindex, damaged_sig, attacker_sig; 
 	float damage_caused = 0.0f;
@@ -7477,7 +7477,12 @@
 		sindex = ship_name_lookup(name); 
 		if (sindex < 0) {
 			sindex = ship_find_exited_ship_by_name(name);
-			attacker_sig = Ships_exited[sindex].obj_signature; 
+			if (sindex < 0) {
+				Warning(LOCATION,"Ship '%s' used in sexp_get_damage_caused() was found neither in current nor exited ship listings",name);
+				continue;
+			} else {
+				attacker_sig = Ships_exited[sindex].obj_signature;
+			}
 		}
 		else {
 			attacker_sig = Objects[Ships[sindex].objnum].signature ;
fix-mantis-2837.patch (855 bytes)   

Echelon9

2013-04-04 07:52

developer   ~0014885

Last edited: 2013-04-04 07:54

 Patch attached which resolves this heap overflow in WiH2's 'Everything is Permitted'.

Whilst the underlying bug is resolved, it remains unclear whether the presence of a ship that triggers the Warning() in my patch warrants a more stern indication in Debug to the FREDder. Alternatively, is there a reasonable basis on which it might trigger, and thus it should just be silently dropped?

i.e. in the 'Everything is Permitted' mission, I see warnings upon mission start for the following ships:
- Fortitude
- Warwick
- Lambda wing members
- Omega wing members
- Corvus wing members
- Cancer wing members
- Aquarius wing members, and
- Auriga wing members.

Can you please test if this Warning() triggers numerous times in other missions, or if it is caused by something unique to this WiH2 mission -- i.e. ships that are yet to jump in?

niffiwan

2013-04-30 09:41

developer   ~0014997

Here's a bunch of missions that use the "get-damage-caused" sexp, I'll test some of these & report about any warnings I get with the patch.

bp2-05.fs2
bp2-18.fs2
bp2-19.fs2
bp2-21.fs2
bp2-22.fs2
bp2-karunacommand.fs2
bta1d_m0_01.fs2 (Between the Ashes Demo)
jad222-5a.fs2
LCW-10.fs2
LCW-3.fs2
LCW-4.fs2
LCW-6.fs2
LCW-8.fs2
M15b-Exploit.fs2 (WoD)
M15-Exploit.fs2 (WoD)
M16-Holding the Line.fs2 (WoD)

Echelon9

2013-04-30 10:50

developer   ~0014998

Good idea - was that listing a grep-fu?

niffiwan

2013-05-01 09:15

developer   ~0015009

grep, yes, but not much fu :) I just extracted all the mission dirs from all the mod VPs I had then grep'd through those for the sexp name. I realised later that since VPs are just packed, it'd be less work to grep the VPs themselves, then extract and grep the mission files only from those VPs that matched.

niffiwan

2013-05-09 10:45

developer   ~0015048

Last edited: 2013-05-09 10:46

 finally got around to testing all these:

bp2-05.fs2
    nothing (Alpha 3 starts in mission)
bp2-18.fs2
    nothing (Falcata 1 starts in mission)
bp2-19.fs2
    lots, see 1st note (many of the "damagers" arrive during the mission)
bp2-21.fs2
    Toutatis (doesn't start in mission, not sure why other ships didn't trigger the warning, e.g. "Intercept Rho 1", maybe the "when" suppresses the 2nd/get-damage-caused section?)
bp2-22.fs2
    nothing (TankSummoner, Platforms & Falcata wing start in mission)
bp2-karunacommand.fs2
    nothing (Katana starts in mission)
bta1d_m0_01.fs2 (Between the Ashes Demo)
    nothing (Alpha 1 starts in mission)
jad222-5a.fs2
    nothing (Player# 1, Delta & Epsilon start in mission)
LCW-10.fs2
    nothing (Alpha 1 starts in mission)
LCW-3.fs2
    Taurus 1-4 (does not start in mission, Alpha 1 does)
LCW-4.fs2
    nothing (Alpha 1 starts in mission)
LCW-6.fs2
    nothing (Alpha 1 starts in mission)
LCW-8.fs2
    nothing (Alpha 1 starts in mission)

Unfortunately I can't test WoD, all three missions crash with the error below.
 
M15b-Exploit.fs2 (WoD)
M15-Exploit.fs2 (WoD)
M16-Holding the Line.fs2 (WoD)
    ASSERTION FAILED: "be->handle == handle" at bmpman/bmpman.cpp:1850 Invalid bitmap handle number 9500 (expected 0) for cursorweb.ani passed to bm_release()


Anyway, based on this info and based on the fact that if the "damagee" (1st arg) hasn't arrived there's no warning, then it's probably a good idea to not have a warning if any of the damagers haven't arrived yet.

Echelon9

2013-05-10 13:52

developer   ~0015050

Fix committed to trunk@9677.

Related Changesets

fs2open: trunk r9677

2013-05-10 10:46

Echelon9


Ported: N/A

Details Diff		 Fix Mantis 2837: AddressSanitizer: heap-buffer-overflow in sexp_get_damage_caused() Affected Issues
0002837
mod - /trunk/fs2_open/code/parse/sexp.cpp Diff File

Issue History

Date Modified Username Field Change
2013-04-04 07:43 Echelon9 New Issue
2013-04-04 07:43 Echelon9 Assigned To => Echelon9
2013-04-04 07:43 Echelon9 Status new => assigned
2013-04-04 07:48 Echelon9 File Added: fix-mantis-2837.patch
2013-04-04 07:52 Echelon9 Note Added: 0014885
2013-04-04 07:52 Echelon9 Assigned To Echelon9 => chief1983
2013-04-04 07:52 Echelon9 Status assigned => code review
2013-04-04 07:53 Echelon9 Note Edited: 0014885
2013-04-04 07:54 Echelon9 Note Edited: 0014885
2013-04-30 09:41 niffiwan Note Added: 0014997
2013-04-30 10:50 Echelon9 Note Added: 0014998
2013-05-01 09:15 niffiwan Note Added: 0015009
2013-05-09 10:45 niffiwan Note Added: 0015048
2013-05-09 10:46 niffiwan Note Edited: 0015048
2013-05-10 13:41 Echelon9 Assigned To chief1983 => Echelon9
2013-05-10 13:41 Echelon9 Status code review => assigned
2013-05-10 13:52 Echelon9 Changeset attached => fs2open trunk r9677
2013-05-10 13:52 Echelon9 Note Added: 0015050
2013-05-10 13:52 Echelon9 Status assigned => resolved
2013-05-10 13:52 Echelon9 Resolution open => fixed