View Issue Details

IDProjectCategoryView StatusLast Update
0002621FSSCPmath-relatedpublic2012-05-02 11:35
ReporterEchelon9 Assigned ToEchelon9  
PriorityhighSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.6.14 RC5 
Target Version3.6.14 
Summary0002621: PVS-Studio warning: A call of the 'memcpy' function will lead to underflow of the buffer 'sbi_new' (starfield.cpp)
DescriptionThis line is starfield.cpp is just plain wrong:

--------
// modify an existing starfield bitmap instance, or add a new one if needed
void stars_modify_entry_FRED(int index, const char *name, starfield_list_entry *sbi_new, bool is_a_sun)
{
   ...
   starfield_bitmap_instance sbi;
   ...
   memcpy( &sbi, sbi_new, sizeof(starfield_bitmap_instance) );
--------

The variables and layout of starfield_bitmap_instance struct is NOT the same as the starfield_list_entry struct, although there is some overlap.
TagsNo tags attached.

Activities

Echelon9

2012-03-04 09:33

developer   ~0013379

Patch attached that fixes this particular copy operation, as well as cleans up code via a proper constructor for starfield_bitmap_instance

Echelon9

2012-03-04 09:36

developer  

mantis-2621-starfield_bitmap_instance.diff (1,980 bytes)   
Index: code/starfield/starfield.cpp
===================================================================
--- code/starfield/starfield.cpp	(revision 8561)
+++ code/starfield/starfield.cpp	(working copy)
@@ -103,7 +103,9 @@
 	int n_verts;
 	vertex *verts;
 
-	starfield_bitmap_instance() { memset(this, 0, sizeof(starfield_bitmap_instance)); star_bitmap_index = -1; };
+	starfield_bitmap_instance() : scale_x(1.0f), scale_y(1.0f), div_x(1), div_y(1), star_bitmap_index(-1), n_verts(0), verts(NULL) {
+        angles ang = { 0.0f, 0.0f, 0.0f };
+    }
 } starfield_bitmap_instance;
 
 // for drawing cool stuff on the background - comes from a table
@@ -819,14 +821,8 @@
 			mprintf(("Adding default sun.\n"));
 
 			starfield_bitmap_instance def_sun;
-		//	memset( &def_sun, 0, sizeof(starfield_bitmap_instance) );
 
 			// stuff some values
-			def_sun.star_bitmap_index = 0;
-			def_sun.scale_x = 1.0f;
-			def_sun.scale_y = 1.0f;
-			def_sun.div_x = 1;
-			def_sun.div_y = 1;
 			def_sun.ang.h = fl_radians(60.0f);
 
 			Suns.push_back(def_sun);
@@ -2199,7 +2195,6 @@
 	Assert(sun_ptr != NULL);
 
 	// copy information
-	memset(&sbi, 0, sizeof(starfield_bitmap_instance));
 	sbi.ang.p = sun_ptr->ang.p;
 	sbi.ang.b = sun_ptr->ang.b;
 	sbi.ang.h = sun_ptr->ang.h;
@@ -2287,10 +2282,9 @@
 	int idx;
 	starfield_bitmap_instance sbi;
 
-	Assert( sle != NULL );
+	Assert(sle != NULL);
 
 	// copy information
-	memset(&sbi, 0, sizeof(starfield_bitmap_instance));
 	sbi.ang.p = sle->ang.p;
 	sbi.ang.b = sle->ang.b;
 	sbi.ang.h = sle->ang.h;
@@ -2510,7 +2504,14 @@
 	Assert( index >= 0 );
 	Assert( sbi_new != NULL );
 
-	memcpy( &sbi, sbi_new, sizeof(starfield_bitmap_instance) );
+    // copy information
+    sbi.ang.p = sbi_new->ang.p;
+	sbi.ang.b = sbi_new->ang.b;
+	sbi.ang.h = sbi_new->ang.h;
+	sbi.scale_x = sbi_new->scale_x;
+	sbi.scale_y = sbi_new->scale_y;
+	sbi.div_x = sbi_new->div_x;
+	sbi.div_y = sbi_new->div_y;
 
 	if (is_a_sun) {
 		idx = stars_find_sun((char*)name);

niffiwan

2012-03-06 09:54

developer   ~0013395

This patch looks fine to me

Echelon9

2012-03-06 13:01

developer   ~0013397

Would someone mind committing to trunk? My dev system is down for a few days.

niffiwan

2012-03-07 09:22

developer   ~0013407

Fix committed to trunk@8596.

niffiwan

2012-05-02 11:35

developer   ~0013503

Fix committed to fs2_open_3_6_14@8720.

Related Changesets

fs2open: trunk r8596

2012-03-07 04:23

niffiwan


Ported: N/A

Details Diff
From Echelon9, fix for mantis 0002621 Affected Issues
0002621
mod - /trunk/fs2_open/code/starfield/starfield.cpp Diff File

fs2open: fs2_open_3_6_14 r8720

2012-05-02 07:37

niffiwan


Ported: N/A

Details Diff
Backport: Trunk r8596 + r8597; From Echelon9, fix for mantis 0002621 && fix a derp by Echelon9 (local variable was hiding the member field) Affected Issues
0002621
mod - /branches/fs2_open_3_6_14/code/starfield/starfield.cpp Diff File

Issue History

Date Modified Username Field Change
2012-03-03 00:40 Echelon9 New Issue
2012-03-03 00:40 Echelon9 Status new => assigned
2012-03-03 00:40 Echelon9 Assigned To => Echelon9
2012-03-04 09:32 Echelon9 File Added: mantis-2621-starfield_bitmap_instance.diff
2012-03-04 09:33 Echelon9 Note Added: 0013379
2012-03-04 09:33 Echelon9 Status assigned => code review
2012-03-04 09:36 Echelon9 File Deleted: mantis-2621-starfield_bitmap_instance.diff
2012-03-04 09:36 Echelon9 File Added: mantis-2621-starfield_bitmap_instance.diff
2012-03-06 09:54 niffiwan Note Added: 0013395
2012-03-06 13:01 Echelon9 Note Added: 0013397
2012-03-07 09:22 niffiwan Changeset attached => fs2open trunk r8596
2012-03-07 09:22 niffiwan Note Added: 0013407
2012-03-07 09:22 niffiwan Status code review => resolved
2012-03-07 09:22 niffiwan Resolution open => fixed
2012-05-02 11:35 niffiwan Changeset attached => fs2open fs2_open_3_6_14 r8720
2012-05-02 11:35 niffiwan Note Added: 0013503