View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002621 | FSSCP | math-related | public | 2012-03-03 00:40 | 2012-05-02 11:35 |
Reporter | Echelon9 | Assigned To | Echelon9 | ||
Priority | high | Severity | major | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.6.14 RC5 | ||||
Target Version | 3.6.14 | ||||
Summary | 0002621: PVS-Studio warning: A call of the 'memcpy' function will lead to underflow of the buffer 'sbi_new' (starfield.cpp) | ||||
Description | This 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. | ||||
Tags | No tags attached. | ||||
|
Patch attached that fixes this particular copy operation, as well as cleans up code via a proper constructor for starfield_bitmap_instance |
|
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); |
|
This patch looks fine to me |
|
Would someone mind committing to trunk? My dev system is down for a few days. |
|
Fix committed to trunk@8596. |
|
Fix committed to fs2_open_3_6_14@8720. |
fs2open: trunk r8596 2012-03-07 04:23 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 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 |
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 |