View Issue Details

IDProjectCategoryView StatusLast Update
0002855FSSCPPilot datapublic2013-04-30 07:42
Reporterniffiwan Assigned Toniffiwan  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.6.19 
Target Version3.7.0 
Summary0002855: off-by-one error in csg_read_loadout clamp code
DescriptionIn the following code, the index starts at zero, the vector.size() starts at 1. Therefore the > should be >= and the assignment should be vector.size() - 1 (or perhaps 0 in this case since ship index 0 is more likely to be a player ship/fighter than the highest index).

        if (idx > ship_list.size()) {
            //mprintf(("CSG => Parse Warning: Invalid value for ship index (%d), Clamping to valid range.\n", idx));
             idx = ship_list.size();
         }


Repeat these changes for primary & secondary weapon index clamp code.
Additional InformationDepends on 2753 only because 2753 changes the idx var name
TagsNo tags attached.

Relationships

related to 0002753 resolvedZacam Custom loadouts not loaded 

Activities

niffiwan

2013-04-29 08:51

developer  

mantis2855-svn.patch (1,838 bytes)   
Index: code/pilotfile/csg.cpp
===================================================================
--- code/pilotfile/csg.cpp	(revision 9657)
+++ code/pilotfile/csg.cpp	(working copy)
@@ -556,9 +556,9 @@
 		// ship
 		ship_idx = cfread_int(cfp);
 
-		if (ship_idx > (int)ship_list.size()) { // on the casts, assume that ship & weapon lists will never exceed ~2 billion
-			//mprintf(("CSG => Parse Warning: Invalid value for ship index (%d), Clamping to valid range.\n", ship_idx));
-			 ship_idx = ship_list.size();
+		if ( (ship_idx >= (int)ship_list.size()) || (ship_idx < -1) ) { // on the casts, assume that ship & weapon lists will never exceed ~2 billion
+			mprintf(("CSG => Parse Warning: Invalid value for ship index (%d), emptying slot.\n", ship_idx));
+			ship_idx = -1;
 		}
 
 		if (slot) {
@@ -575,9 +575,9 @@
 		for (j = 0; j < count; j++) {
 			wep_idx = cfread_int(cfp);
 
-			if (wep_idx > (int)weapon_list.size()) {
-				//mprintf(("CSG => Parse Warning: Invalid value for weapon index (%d), Clamping to valid range.\n", wep_idx));
-				wep_idx = weapon_list.size();
+			if ( (wep_idx >= (int)weapon_list.size()) || (wep_idx < -1) ) {
+				mprintf(("CSG => Parse Warning: Invalid value for primary weapon index (%d), emptying slot.\n", wep_idx));
+				wep_idx = -1;
 			}
 
 
@@ -602,9 +602,9 @@
 		for (j = 0; j < count; j++) {
 			wep_idx = cfread_int(cfp);
 
-			if (wep_idx > (int)weapon_list.size()) {
-				//mprintf(("CSG => Parse Warning: Invalid value for weapon index (%d), Clamping to valid range.\n", wep_idx));
-				wep_idx = weapon_list.size();
+			if ( (wep_idx >= (int)weapon_list.size()) || (wep_idx < -1) ) {
+				mprintf(("CSG => Parse Warning: Invalid value for secondary weapon index (%d), emptying slot.\n", wep_idx));
+				wep_idx = -1;
 			}
 
 			if ( slot && (j < MAX_SHIP_SECONDARY_BANKS) ) {
mantis2855-svn.patch (1,838 bytes)   

niffiwan

2013-04-29 08:59

developer   ~0014976

Patch is ready, please review.

In addition to fixing the off-by-one error, I
1) added bounds checking that should have been added in r9657
2) emptied ship/weapon saved loadout slots rather than assign values that may not make sense (i.e. max ship idx = VolitionBravos? max wep idx = S_Cluster Bomb Baby?)
3) enabled mprintfs

On 2) I had a corrupted pilotfile, when testing, the empty wingmen loadout slots were ignored & the wingmen got the default mission loadout.

Zacam

2013-04-30 06:54

administrator   ~0014989

I had a chance to look this over as part of the patch for 2753, but fo due diligence, tested the included patch here. Looks fine.

niffiwan

2013-04-30 07:42

developer   ~0014995

Fix committed to trunk@9664.

Related Changesets

fs2open: trunk r9664

2013-04-30 04:35

niffiwan


Ported: N/A

Details Diff
Fix for mantis 2855: fix off-by-one, add negative index check, empty invalid slots & enable mprintfs Affected Issues
0002855
mod - /trunk/fs2_open/code/pilotfile/csg.cpp Diff File

Issue History

Date Modified Username Field Change
2013-04-28 21:10 niffiwan New Issue
2013-04-28 21:10 niffiwan Status new => assigned
2013-04-28 21:10 niffiwan Assigned To => niffiwan
2013-04-28 21:11 niffiwan Relationship added related to 0002753
2013-04-29 08:51 niffiwan File Added: mantis2855-svn.patch
2013-04-29 08:59 niffiwan Note Added: 0014976
2013-04-29 08:59 niffiwan Status assigned => code review
2013-04-30 06:54 Zacam Note Added: 0014989
2013-04-30 07:42 niffiwan Changeset attached => fs2open trunk r9664
2013-04-30 07:42 niffiwan Note Added: 0014995
2013-04-30 07:42 niffiwan Status code review => resolved
2013-04-30 07:42 niffiwan Resolution open => fixed