View Issue Details

IDProjectCategoryView StatusLast Update
0002880FSSCPPilot datapublic2013-05-31 22:40
Reporterniffiwan Assigned Toniffiwan  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.6.19 
Target Version3.7.0 
Summary0002880: the ship_class of dead/departed wingmen in a red alert mission is not read correctly
DescriptionAs reported by Deepstar:
http://www.hard-light.net/forums/index.php?topic=84619

In a red alert mission, dead/departed wingman with the red-alert-carry flag have their ship_class recorded as -1. This is written to the CSG. However when reading this data back, -1 is used as an index into an array, which results in semi-random memory being used as the ship_class.

The symptoms can either be a crash, or nothing at all, it depends on the contents of the -1 indexed memory.
Steps To ReproduceUse the attached mod.
Select the Red Alert Dead Wing campaign
Press 2/3/4 to destroy relevant wingmen
Press 1 to advance to the next mission
Exit the mission
Exit FSO
Use a hex editor on the CSG to view the 4-byte int starting at the 5th byte after the destroyed wingman's ship name. It should be -1. It will be some random number.
FSO *may* now crash when selecting the pilot, depending on the exact value of the ship_class
Additional InformationAddress Sanitizer helped to find the exact cause of the bug:

$ ./fs2_open_3.6.19_DEBUG_9679_asan 2>&1 | asan_symbolize.py | c++filt
=================================================================
==20176== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6052004ec978 at pc 0xb85395 bp 0x7fffdab5e280 sp 0x7fffdab5e278
READ of size 4 at 0x6052004ec978 thread T0
    #0 0xb85394 in pilotfile::csg_read_redalert() /home/me/src/git-fs2_open/code/pilotfile/csg.cpp:803
    0000001 0xb89fa9 in pilotfile::load_savefile(char const*) /home/me/src/git-fs2_open/code/pilotfile/csg.cpp:1401
    0000002 0x7ff4f5 in mission_campaign_load(char*, player*, int) /home/me/src/git-fs2_open/code/mission/missioncampaign.cpp:665
    0000003 0x8057ab in mission_load_up_campaign(player*) /home/me/src/git-fs2_open/code/mission/missioncampaign.cpp:1671
    0000004 0x41cdb9 in game_enter_state(int, int) /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:5889
    0000005 0x58f46d in gameseq_set_state(int, int) /home/me/src/git-fs2_open/code/gamesequence/gamesequence.cpp:280
    0000006 0x41bb7b in game_process_event(int, int) /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:5165
    0000007 0x58ff47 in gameseq_process_events() /home/me/src/git-fs2_open/code/gamesequence/gamesequence.cpp:395
    0000008 0x41f081 in game_main(char*) /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:7034
    0000009 0x41f535 in main /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:7168
    0000010 0x7fdb70cec76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
    #11 0x40b0f8 in _start ??:0
0x6052004ec978 is located 8 bytes to the left of 2048-byte region [0x6052004ec980,0x6052004ed180)
allocated by thread T0 here:
    #0 0x7fdb7325456a in __interceptor_malloc ??:0
    0000001 0xde131a in _vm_malloc(int, char*, int, int) ??:0
    0000002 0xb70949 in SCP_vm_allocator<pilotfile::index_list_t>::allocate(unsigned long) /home/me/src/git-fs2_open/code/./globalincs/vmallocator.h:59
    0000003 0xb7077b in std::_Vector_base<pilotfile::index_list_t, SCP_vm_allocator<pilotfile::index_list_t> >::_M_allocate(unsigned long) /usr/local/gcc-4.8.0/include/c++/4.8.0/bits/stl_vector.h:168
    0000004 0xb6ff6b in std::vector<pilotfile::index_list_t, SCP_vm_allocator<pilotfile::index_list_t> >::_M_insert_aux(__gnu_cxx::__normal_iterator<pilotfile::index_list_t*, std::vector<pilotfile::index_list_t, SCP_vm_allocator<pilotfile::index_list_t> > >, pilotfile::index_list_t const&) /usr/local/gcc-4.8.0/include/c++/4.8.0/bits/vector.tcc:345
    0000005 0xb6f972 in std::vector<pilotfile::index_list_t, SCP_vm_allocator<pilotfile::index_list_t> >::push_back(pilotfile::index_list_t const&) /usr/local/gcc-4.8.0/include/c++/4.8.0/bits/stl_vector.h:913
    0000006 0xb7de65 in pilotfile::csg_read_info() /home/me/src/git-fs2_open/code/pilotfile/csg.cpp:70
    0000007 0xb89f40 in pilotfile::load_savefile(char const*) /home/me/src/git-fs2_open/code/pilotfile/csg.cpp:1386
    0000008 0x7d1e36 in player_select_close() /home/me/src/git-fs2_open/code/menuui/playermenu.cpp:480
    0000009 0x41cb96 in game_leave_state(int, int) /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:5809
    0000010 0x58f31c in gameseq_set_state(int, int) /home/me/src/git-fs2_open/code/gamesequence/gamesequence.cpp:275
    #11 0x41b8b5 in game_process_event(int, int) /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:5070
    0000012 0x58ff47 in gameseq_process_events() /home/me/src/git-fs2_open/code/gamesequence/gamesequence.cpp:395
    0000013 0x41f081 in game_main(char*) /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:7034
    0000014 0x41f535 in main /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:7168
    0000015 0x7fdb70cec76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
Shadow bytes around the buggy address:
  0x0c0ac00958d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0ac00958e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0ac00958f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0ac0095900: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0ac0095910: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c0ac0095920: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]
  0x0c0ac0095930:00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c0ac0095940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c0ac0095950: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c0ac0095960: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c0ac0095970: 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
==20176== ABORTING
TagsNo tags attached.

Relationships

related to 0001326 resolvedniffiwan Departed wingmen are displayed as destroyed after red alert 

Activities

niffiwan

2013-05-26 01:53

developer  

mantis2880.7z (10,758 bytes)

FUBAR-BDHR

2013-05-26 02:37

developer   ~0015087

This has been around for ages. I set it as a duplicates of the old ticket. Probably a good idea to merge them and close one.

niffiwan

2013-05-26 02:47

developer   ~0015088

Actually, I'm pretty sure they're slightly different (although related). I'll see about fixing 0001326 after this one - I think it'll require some changes in the red-alert "store" code as well as the "read" code.

niffiwan

2013-05-26 03:57

developer  

mantis2880-svn.patch (2,069 bytes)   
Index: code/pilotfile/csg.cpp
===================================================================
--- code/pilotfile/csg.cpp	(revision 9680)
+++ code/pilotfile/csg.cpp	(working copy)
@@ -800,7 +800,14 @@
 
 		// ship class, index into ship_list[]
 		i = cfread_int(cfp);
-		ras.ship_class = ship_list[i].index;
+		if ( (i >= (int)ship_list.size()) || (i < -1) ) {
+			mprintf(("CSG => Parse Warning: Invalid value for red alert ship index (%d), emptying slot.\n", i));
+			ras.ship_class = -1;
+		} else if ( i == -1 ) {  // ship destroyed/exited
+			ras.ship_class = i;
+		} else {
+			ras.ship_class = ship_list[i].index;
+		}
 
 		// subsystem hits
 		count = cfread_int(cfp);
@@ -850,8 +857,8 @@
 			ras.secondary_weapons.push_back( weapons );
 		}
 
-		// this is quite likely a *bad* thing if it happens
-		if (ras.ship_class >= 0) {
+		// this is quite likely a *bad* thing if it doesn't happen
+		if (ras.ship_class >= RED_ALERT_EXITED_SHIP_CLASS) {
 			Red_alert_wingman_status.push_back( ras );
 		}
 	}
Index: code/missionui/redalert.cpp
===================================================================
--- code/missionui/redalert.cpp	(revision 9680)
+++ code/missionui/redalert.cpp	(working copy)
@@ -44,10 +44,6 @@
 //static int Red_alert_num_slots_used = 0;
 static int Red_alert_voice_started;
 
-#define RED_ALERT_WARN_TIME		4000				// time to warn user that new orders are coming
-
-#define RED_ALERT_EXITED_SHIP_CLASS		-1
-
 SCP_vector<red_alert_ship_status> Red_alert_wingman_status;
 SCP_string Red_alert_precursor_mission;
 
Index: code/missionui/redalert.h
===================================================================
--- code/missionui/redalert.h	(revision 9680)
+++ code/missionui/redalert.h	(working copy)
@@ -34,6 +34,11 @@
 
 // should only ever be defined in redalert.cpp and the pilot file code!!
 #ifdef REDALERT_INTERNAL
+
+#define RED_ALERT_WARN_TIME		4000				// time to warn user that new orders are coming
+
+#define RED_ALERT_EXITED_SHIP_CLASS		-1
+
 typedef struct red_alert_ship_status {
 	SCP_string	name;
 	float		hull;
mantis2880-svn.patch (2,069 bytes)   

The_E

2013-05-31 21:49

administrator   ~0015108

Patch looks good from here

niffiwan

2013-05-31 22:40

developer   ~0015110

Fix committed to trunk@9688.

Related Changesets

fs2open: trunk r9688

2013-05-31 19:38

niffiwan


Ported: N/A

Details Diff
Fix for mantis 2880: handle -1 red alert departed ship indexes Affected Issues
0002880
mod - /trunk/fs2_open/code/missionui/redalert.cpp Diff File
mod - /trunk/fs2_open/code/missionui/redalert.h Diff File
mod - /trunk/fs2_open/code/pilotfile/csg.cpp Diff File

Issue History

Date Modified Username Field Change
2013-05-26 01:50 niffiwan New Issue
2013-05-26 01:50 niffiwan Status new => assigned
2013-05-26 01:50 niffiwan Assigned To => niffiwan
2013-05-26 01:53 niffiwan File Added: mantis2880.7z
2013-05-26 02:36 FUBAR-BDHR Relationship added duplicate of 0001326
2013-05-26 02:37 FUBAR-BDHR Note Added: 0015087
2013-05-26 02:47 niffiwan Note Added: 0015088
2013-05-26 02:47 niffiwan Relationship replaced related to 0001326
2013-05-26 03:57 niffiwan File Added: mantis2880-svn.patch
2013-05-26 03:57 niffiwan Status assigned => code review
2013-05-31 21:49 The_E Note Added: 0015108
2013-05-31 22:40 niffiwan Changeset attached => fs2open trunk r9688
2013-05-31 22:40 niffiwan Note Added: 0015110
2013-05-31 22:40 niffiwan Status code review => resolved
2013-05-31 22:40 niffiwan Resolution open => fixed