Hard Light Productions Forums
Modding, Mission Design, and Coding => FS2 Open Coding - The Source Code Project (SCP) => Topic started by: Echelon9 on August 28, 2011, 05:45:00 am
-
Those who follow the nightly builds or the SVN commit logs will have seen many recently with the description "PVS-Studio fix: ...". These fixes are the result of a trial licence to PVS-Studio which the Freespace 2 Open SCP has been granted recently.
I've copied below the background from the internal forums.
Because the license will expire on 8 September, I'm going to post the remaining 12 89 issues (Level 1: 3 48, Level 2: 9 41) to the public forum so that others can review and have a go at submitting patches. This is based on the code at revision 7855 7561.
How can we help improve the FS2_Open code with this?
Best outcome would be for each of those 89 issues to be triaged as legitimate or a false positive. For those which are legitimate, having committed to trunk before 8 September as many fixes as possible will let a final run confirm they are indeed fixed.
How to read each of the reports
In each of the attached text files, the reports are line-by-line, in the format:
'Starred' 'Level' 'ID' 'Issue Type' 'Message' 'Project' 'Filename' 'Line Number' 'Enabled/Disabled'
The additional description of each 'Issue Type' and sugested solutions can be found at the address with structure www.viva64.com/en/[Issued Type] e.g. www.viva64.com/en/V514
Please do keep in mind that there are some false positives within the reports.
Background from the internal
So listening to John Carmack's keynote from Quakecon, I was introduced to PVS-Studio (http://www.viva64.com/en/pvs-studio/), which is a commercial static code analysis tool. While its main aim is the migration of 32bit apps to 64bit, it also includes a general source code analysis module for common unsafe, unsavoury or undesirable coding mistakes.
While commercial products like this are normally outside of FS2_Open's interest, they do provide temp licenses for open-source* projects to test the tool. PVS-Studio has recently found bugs in Apache HTTP Server (http://www.viva64.com/en/b/0105/), Clang (http://www.viva64.com/en/b/0108/), Qt (http://www.viva64.com/en/a/0075/), Chromium (http://www.viva64.com/en/a/0074/) and TortoiseSVN (http://www.viva64.com/en/b/0085/).
So I wrote them a quick email explaining what FS2_Open SCP is, and was graced with a trial licence!
Hello,
Thank your interest to PVS-Studio. And thanks for detailed introduction to your project. Here is your license:
Registration Name: XXXXX
Registration Key: XXXX-XXXX-XXXX
License Valid Thru: XX.XX.2011
If you find an interesting bugs please write a post to blog about it and about your impression for our tool.
Now about your questions for license. You can use results of analyzer how you want but please don't distribute license key.
Let me know about other questions or issues.
Evgeniy Ryzhkov
General Director (CEO)
OOO "Program Verification System" (Co Ltd)
I'm going to run the full text output from the PVS-Studio tool past some of the SCP devs over the next few days -- unfortunately I can't share the licence key to the binaries. I've attached a patch for a subset of issues which were found and fixed with PVS-Studio.
Such examples include, mistakes copying and pasting:
Index: code/ai/aicode.cpp
===================================================================
--- code/ai/aicode.cpp (revision 7435)
+++ code/ai/aicode.cpp (working copy)
@@ -3099,7 +3099,7 @@
docker_aip->support_ship_objnum = -1;
dockee_aip->support_ship_objnum = -1;
docker_aip->support_ship_signature = -1;
- docker_aip->support_ship_signature = -1;
+ dockee_aip->support_ship_signature = -1;
}
// unlink the two objects
Examples of mixing up x and y co-ordinates:
Index: code/hud/hudlock.cpp
===================================================================
--- code/hud/hudlock.cpp (revision 7435)
+++ code/hud/hudlock.cpp (working copy)
@@ -1093,7 +1093,7 @@
target_objp = &Objects[Player_ai->target_objnum];
Player->current_target_sx = -1;
- Player->current_target_sx = -1;
+ Player->current_target_sy = -1;
swp = &Player_ship->weapons;
wip = &Weapon_info[swp->secondary_bank_weapons[swp->current_secondary_bank]];
...And again:
Index: code/ship/ship.cpp
===================================================================
--- code/ship/ship.cpp (revision 7435)
+++ code/ship/ship.cpp (working copy)
@@ -3462,7 +3462,7 @@
current_trigger->angle.xyz.x = fl_radians(current_trigger->angle.xyz.x);
current_trigger->angle.xyz.y = fl_radians(current_trigger->angle.xyz.y);
- current_trigger->angle.xyz.y = fl_radians(current_trigger->angle.xyz.z);
+ current_trigger->angle.xyz.z = fl_radians(current_trigger->angle.xyz.z);
}else{
current_trigger->absolute = false;
if(!optional_string("+relative_angle:"))
@@ -3509,7 +3509,7 @@
current_trigger->angle.xyz.x = fl_radians(current_trigger->angle.xyz.x);
current_trigger->angle.xyz.y = fl_radians(current_trigger->angle.xyz.y);
- current_trigger->angle.xyz.y = fl_radians(current_trigger->angle.xyz.z);
+ current_trigger->angle.xyz.z = fl_radians(current_trigger->angle.xyz.z);
}else{
current_trigger->absolute = false;
required_string("+relative_angle:");
Note: * and open-source "-esque" projects.
[attachment deleted by ninja]
-
To get this bug-hunt started, here is an example:
False 1 4213 V515 The 'delete' operator is applied to non-pointer. code hud.cpp 1216 False
From V515. The 'delete' operator is applied to non-pointer (http://www.viva64.com/en/V515)
In code, the delete operator is applied to a class object instead of the pointer. It is most likely to be an error.
Consider a code sample:
CString str;
...
delete str;
The 'delete' operator can be applied to an object of the CString type since the CString class can be automatically cast to the pointer. Such code might cause an exception or unexpected program behavior.
hud.cpp:1216
/**
* @brief Delete all HUD gauge objects, for all ships
*/
void hud_close()
{
...
for(j = 0; j < num_gauges; j++) {
delete Ship_info[i].hud_gauges[j]; // <---- The reported issue
Ship_info[i].hud_gauges[j] = NULL;
}
...
}
-
Here's a patch to deal with essentially the same issue in two places:
False 1 1841 V523 The 'then' statement is equivalent to the 'else' statement. code barracks.cpp 1192 False
False 1 7608 V523 The 'then' statement is equivalent to the 'else' statement. code missionhotkey.cpp 1223 False
The problem is that two different defines evaluate to the same colour:
globalincs/alphacolors.h:33
// text that indicates line is active item
#define Color_text_active Color_bright_white
// text that indicates line is active item, and line is selected or subselected
#define Color_text_active_hi Color_bright_white
From the SVN logs, it seems that this hasn't changed from :v-old:'s code.
Here's the patch which removes the conditional statements and replaces each of them with a single statement.
Index: code/mission/missionhotkey.cpp
===================================================================
--- code/mission/missionhotkey.cpp (revision 7564)
+++ code/mission/missionhotkey.cpp (working copy)
@@ -1217,10 +1217,11 @@
List_buttons[line - Scroll_offset].update_dimensions(Hotkey_list_coords[gr_screen.res][0], y, Hotkey_list_coords[gr_screen.res][0] + Hotkey_list_coords[gr_screen.res][2] - Hotkey_list_coords[gr_screen.res][0], font_height);
List_buttons[line - Scroll_offset].enable();
if (hotkeys & (1 << Cur_hotkey)) {
- if (line == Selected_line)
+ /*if (line == Selected_line)
gr_set_color_fast(&Color_text_active_hi);
else
- gr_set_color_fast(&Color_text_active);
+ gr_set_color_fast(&Color_text_active);*/
+ gr_set_color_fast(&Color_text_active);
} else {
if (line == Selected_line)
Index: code/menuui/barracks.cpp
===================================================================
--- code/menuui/barracks.cpp (revision 7564)
+++ code/menuui/barracks.cpp (working copy)
@@ -1187,11 +1187,12 @@
break;
if (!stricmp(Cur_pilot->callsign, Pilots[cur_pilot_idx]) && (is_pilot_multi(Cur_pilot) == multi)) {
- if ((cur_pilot_idx == Selected_line) || (cur_pilot_idx == prospective_pilot)) {
+ /*if ((cur_pilot_idx == Selected_line) || (cur_pilot_idx == prospective_pilot)) {
gr_set_color_fast(&Color_text_active_hi);
} else {
gr_set_color_fast(&Color_text_active);
- }
+ }*/
+ gr_set_color_fast(&Color_text_active);
} else {
if (cur_pilot_idx == Selected_line) {
gr_set_color_fast(&Color_text_selected);
An alternative way of resolving the problem would be to change the defines so that they are different colours. However, given that it's always been this way I thought it would be best to maintain the current look.
What do you think?
If this a good, an obvious followup would be to replace every instance of Color_text_active_hi with Color_text_active, but I'll wait for feedback before submitting a patch for that.
[attachment deleted by ninja]
-
And another one for a typo / cut-n-paste error:
False 1 8209 V523 The 'then' statement is equivalent to the 'else' statement. code missiontraining.cpp 486 False
Index: mission/missiontraining.cpp
===================================================================
--- mission/missiontraining.cpp (revision 7564)
+++ mission/missiontraining.cpp (working copy)
@@ -484,7 +484,7 @@
if (f2i(Missiontime - Mission_events[TRAINING_OBJ_LINES_MASK(i)].satisfied_time) < MIN_FAILED_TIME) {
Training_obj_lines[i] |= TRAINING_OBJ_STATUS_UNKNOWN;
} else {
- Training_obj_lines[i] |= TRAINING_OBJ_STATUS_UNKNOWN;
+ Training_obj_lines[i] |= TRAINING_OBJ_STATUS_KNOWN;
}
}
}
[attachment deleted by ninja]
-
Thanks niffiwan! The patch from the first post, which addresses the below two reports, has been committed to trunk as of r7565 (http://svn.icculus.org/fs2open?view=rev&revision=7565).
False 1 1841 V523 The 'then' statement is equivalent to the 'else' statement. code barracks.cpp 1192 False
False 1 7608 V523 The 'then' statement is equivalent to the 'else' statement. code missionhotkey.cpp 1223 False
On that second one in mission/missiontraining.cpp, I think that is the right response but I'll wait for some other coders to test or review before committing.
False 1 8209 V523 The 'then' statement is equivalent to the 'else' statement. code missiontraining.cpp 486 False
That change may have other unintended consequences. It did garner a bit of discussion on IRC. Happy to hear your views.
-
I think the intent of the code is to display correct training event message lines in the 5 slots defined on the HUD. A line which is marked as UNKNOWN should be displayed if possible. A line which is KNOWN does not need to be displayed. In order that the player can read all the messages, success messages are supposed to remain onscreen for 5 secs (I think, 5 units of some time ;)) and fail messages are supposed to remain onscreen for 7 secs.
Therefore I believe the effect of the if-then in question (with both results being UNKNOWN) means that a failed training event will never be removed from the HUD display. This would only be a real problem if the player failed at least 5 events such that they can't see any new training events, and I guess this wouldn't come up very often.
Is this what you think, or have I made a mistake somewhere?
-
Fixed a bug in my code.
I'll fix the bugs in other people's code tomorrow. :p
-
Another simple fix, in this case it's the scroll offset in the techroom info box (i.e. how far you can scroll the text up the screen when the text size exceeds the normal display window). The if-then allows a different offset for the ship tab vs the weapon & intelligence tab. I can't see a good reason to have the scroll offset different in each of these tabs and given that the code currently has them the same, I just removed the if-then statement.
Also note that this file seemed to be missing a EOL character on the last line, vim added it in for me on saving. If this gives you grief when trying to apply the patch, just remove the second chunk.
False 1 14823 V523 The 'then' statement is equivalent to the 'else' statement. code techmenu.cpp 614 False
Index: code/menuui/techmenu.cpp
===================================================================
--- code/menuui/techmenu.cpp (revision 7571)
+++ code/menuui/techmenu.cpp (working copy)
@@ -609,11 +609,12 @@
{
int h;
- if (Tab == SHIPS_DATA_TAB){
+ /*if (Tab == SHIPS_DATA_TAB){
h = Tech_desc_coords[gr_screen.res][SHIP_H_COORD];
} else {
h = Tech_desc_coords[gr_screen.res][3];
- }
+ }*/
+ h = Tech_desc_coords[gr_screen.res][SHIP_H_COORD];
if (Text_offset + h / gr_get_font_height() < Text_size) {
Text_offset++;
@@ -1424,4 +1425,4 @@
else
Intel_info[i].flags &= ~IIF_IN_TECH_DATABASE;
}
-}
\ No newline at end of file
+}
[attachment deleted by ninja]
-
I think the intent of the code is to display correct training event message lines in the 5 slots defined on the HUD. A line which is marked as UNKNOWN should be displayed if possible. A line which is KNOWN does not need to be displayed. In order that the player can read all the messages, success messages are supposed to remain onscreen for 5 secs (I think, 5 units of some time ;)) and fail messages are supposed to remain onscreen for 7 secs.
Therefore I believe the effect of the if-then in question (with both results being UNKNOWN) means that a failed training event will never be removed from the HUD display. This would only be a real problem if the player failed at least 5 events such that they can't see any new training events, and I guess this wouldn't come up very often.
Is this what you think, or have I made a mistake somewhere?
Yup, you've convinced me :) Thanks niffiwan, committed to trunk.
False 1 8209 V523 The 'then' statement is equivalent to the 'else' statement. code missiontraining.cpp 486 False
-
Another simple fix, in this case it's the scroll offset in the techroom info box....
False 1 14823 V523 The 'then' statement is equivalent to the 'else' statement. code techmenu.cpp 614 False
Cheers. On a roll niffiwan! Committed to trunk as well.
-
Why are we just commenting out the code rather than outright removing it from the repo?
-
Great progress is being seen in the SCP trunk (http://svn.icculus.org/fs2open?view=rev), with a number of coders supplying fixes to these issues, including Karajorma, niffiwan and The E.
I've updated the original post with latest PVS-Studio output logs with only the remaining issues listed.
The impact so far has been to triage and resolve the highest priority issues (Level 1).
As at revision 7584:
To clear: 79 (-10)
Level 1: 38 (-10)
Level 2: 41 (0)
-
Why are we just commenting out the code rather than outright removing it from the repo?
Good point - call it a bad habit from not having used source revision control that much before. I'll create new patches tonight to remove the commented out code.
-
A quick patch to remove code I'd previously left commented out.
Echelon9 seems to have already done this for my most recent patch - thanks!
Index: code/mission/missionhotkey.cpp
===================================================================
--- code/mission/missionhotkey.cpp (revision 7596)
+++ code/mission/missionhotkey.cpp (working copy)
@@ -1217,12 +1217,7 @@
List_buttons[line - Scroll_offset].update_dimensions(Hotkey_list_coords[gr_screen.res][0], y, Hotkey_list_coords[gr_screen.res][0] + Hotkey_list_coords[gr_screen.res][2] - Hotkey_list_coords[gr_screen.res][0], font_height);
List_buttons[line - Scroll_offset].enable();
if (hotkeys & (1 << Cur_hotkey)) {
- /*if (line == Selected_line)
- gr_set_color_fast(&Color_text_active_hi);
- else
- gr_set_color_fast(&Color_text_active);*/
gr_set_color_fast(&Color_text_active);
-
} else {
if (line == Selected_line)
gr_set_color_fast(&Color_text_selected);
Index: code/menuui/barracks.cpp
===================================================================
--- code/menuui/barracks.cpp (revision 7596)
+++ code/menuui/barracks.cpp (working copy)
@@ -1187,11 +1187,6 @@
break;
if (!stricmp(Cur_pilot->callsign, Pilots[cur_pilot_idx]) && (is_pilot_multi(Cur_pilot) == multi)) {
- /*if ((cur_pilot_idx == Selected_line) || (cur_pilot_idx == prospective_pilot)) {
- gr_set_color_fast(&Color_text_active_hi);
- } else {
- gr_set_color_fast(&Color_text_active);
- }*/
gr_set_color_fast(&Color_text_active);
} else {
if (cur_pilot_idx == Selected_line) {
[attachment deleted by ninja]
-
Committed that for ya. r7599.
-
Original post updated with PVS-Studio output from the latest r7599 trunk.
The_E did a great job triaging those issues which are false positive reports. Changes since last update are:
As at revision 7599:
To clear: 74 (-5)
Level 1: 33 (-5)
Level 2: 41 (0)
-
Thanks Zacam
Next up is a fix that I'd really like other coders to have a look over.
False 1 11608 V523 The 'then' statement is equivalent to the 'else' statement. code psnet2.cpp 1627 False
The if-then check seems to imply that if we receive more data than should be in the buffer (i.e. max_len/NETBUFFERSIZE), we should only process up to the max size. Currently, the code always copies all the data. This (in theory) could result in a buffer overflow in the next memcpy because the code only mallocs max_len/NETBUFFERSIZE (600).
However, if we truncate the data from the buffer it could result in a corrupt network message and there doesn't seem to be anything to catch this. There is a mention of checksums in the comments, but I can't see any implemented in the code. I *think* a better approach is to not ACK the oversize packet and to wait for a retransmit. Of course, if the reason for the oversize packet is a problem at the sending end, rather than a "wire" issue then the retransmitted packet is likely to have the same problem.
Anyway, I'm not 100% sure about this one and I'd appreciate any input others have about it...
Index: code/network/psnet2.cpp
===================================================================
--- code/network/psnet2.cpp (revision 7599)
+++ code/network/psnet2.cpp (working copy)
@@ -1623,7 +1623,9 @@
for(i=0; i<MAXNETBUFFERS; i++){
if(NULL == rsocket->rbuffers[i]){
if(rcv_buff.data_len>max_len){
- rsocket->recv_len[i] = rcv_buff.data_len;
+ ml_string("Received oversized reliable packet!");
+ //don't ack it, which will mean we will get it again soon.
+ continue;
} else {
rsocket->recv_len[i] = rcv_buff.data_len;
}
[attachment deleted by ninja]
-
Thanks Zacam
Next up is a fix that I'd really like other coders to have a look over.
False 1 11608 V523 The 'then' statement is equivalent to the 'else' statement. code psnet2.cpp 1627 False
Update: whoops, I screwed up the if / else in r7603. I'll reverse it and use niffiwan's.
I just commmitted niffiwan's patch for this one (http://svn.icculus.org/fs2open?view=rev&revision=7605), for at least the short term.
-
Continued progress on tracking down the priority one bugs.
As at revision 7606:
To clear: 72 (-2)
Level 1: 31 (-2)
Level 2: 41 (0)
-
So I took a look at
False 1 4 V512 A call of the 'memcpy' function will lead to overflow of the buffer '& x'. code pstypes.h 131 False
I can either fix it the easy way and just expand the memcpy into three assignments or fix it properly and convert vertex[/tt] to using vec3ds. I started to do the conversion, but when I realized that this is going to touch most of the graphics code I figured I should ask for an opinion? Do it right or fix it quick? The layout of this struct is abused all over the code in similar fashion os the one the PVS-Studio is complaining about.
Index: code/globalincs/pstypes.h
===================================================================
--- code/globalincs/pstypes.h (revision 7609)
+++ code/globalincs/pstypes.h (working copy)
@@ -81,6 +81,9 @@
struct {
float x,y,z;
} xyz;
+ struct {
+ float x,y,w;
+ } xyw;
float a1d[3];
};
inline void operator= (vertex&vert);
@@ -89,7 +92,7 @@
bool operator == (const vec3d &other);
} vec3d;
-inline bool vec3d::operator == (const vec3d &other)
+inline bool vec3d::operator== (const vec3d &other)
{
return ( (a1d[0] == other.a1d[0]) && (a1d[1] == other.a1d[1]) && (a1d[2] == other.a1d[2]) );
}
@@ -114,21 +117,28 @@
typedef struct uv_pair {
float u,v;
+
+ bool operator==(const uv_pair &other);
} uv_pair;
+bool uv_pair::operator==(const uv_pair &other)
+{
+ return (u == other.u) && (v == other.v);
+}
+
// Used to store rotated points for mines.
// Has flag to indicate if projected.
typedef struct vertex {
- float x, y, z; // world space position
- float sx, sy, sw; // screen space position (sw == 1/z)
- float u, v; // texture position
+ vec3d world_pos; // world space position
+ vec3d screen_pos; // screen space position (sw == 1/z)
+ uv_pair uv; // texture position
ubyte r, g, b, a; // color. Use b for darkening;
ubyte spec_r, spec_g, spec_b, spec_a; //specular highlights -Bobboau
ubyte codes; // what sides of view pyramid this point is on/off. 0 = Inside view pyramid.
ubyte flags; // Projection flags. Indicates whether it is projected or not or if projection overflowed.
ubyte pad[2]; // pad structure to be 4 byte aligned.
- void operator=(vec3d&vec) {
- memcpy(&x,&vec, sizeof(vec3d));
+ void operator=(vec3d& vec) {
+ memcpy(&world_pos,&vec, sizeof(vec3d));
}
bool operator == (const vertex &other);
@@ -137,17 +147,17 @@
inline bool vertex::operator == (const vertex &other)
{
// NOTE: this is checking position and uv only!
- return ( (x == other.x) && (y == other.y) && (z == other.z)
- && (u == other.u) && (v == other.v) );
+ return ( ( world_pos == other.world_pos )
+ && (uv == other.uv) );
}
inline void vec3d::operator= (vertex&vert) {
- memcpy(this,&vert.x,sizeof(vec3d));
+ memcpy(this,&vert.world_pos,sizeof(vec3d));
}
//set the vector to the vertex screen position
inline void vec3d::set_screen_vert(vertex&vert) {
- memcpy(this,&vert.sx,sizeof(vec3d));
+ memcpy(this,&vert.screen_pos,sizeof(vec3d));
}
//def_list
-
Thanks Zacam
Next up is a fix that I'd really like other coders to have a look over.
False 1 11608 V523 The 'then' statement is equivalent to the 'else' statement. code psnet2.cpp 1627 False
Update: whoops, I screwed up the if / else in r7603. I'll reverse it and use niffiwan's.
I just commmitted niffiwan's patch for this one (http://svn.icculus.org/fs2open?view=rev&revision=7605), for at least the short term.
Well, I screwed up my patch too :)
The continue in that place will only break out of the for-MAXNETBUFFERS loop, not the do-while loop it's meant to break out of. It protects the memcpy, but it also sends an ACK, as well as running through the for loop up to 149 times too many. Here's the fix - I looked through the code above this point and I this seems to be the first place in this function that memcpy is called on rcv_buff.data.
Index: code/network/psnet2.cpp
===================================================================
--- code/network/psnet2.cpp (revision 7609)
+++ code/network/psnet2.cpp (working copy)
@@ -1620,15 +1620,14 @@
}
}
if(savepacket){
+ if(rcv_buff.data_len>max_len){
+ ml_string("Received oversized reliable packet!");
+ //don't ack it, which will mean we will get it again soon.
+ continue;
+ }
for(i=0; i<MAXNETBUFFERS; i++){
if(NULL == rsocket->rbuffers[i]){
- if(rcv_buff.data_len>max_len){
- ml_string("Received oversized reliable packet!");
- //don't ack it, which will mean we will get it again soon.
- continue;
- } else {
- rsocket->recv_len[i] = rcv_buff.data_len;
- }
+ rsocket->recv_len[i] = rcv_buff.data_len;
rsocket->rbuffers[i] = (reliable_net_rcvbuffer *)vm_malloc(sizeof(reliable_net_rcvbuffer));
memcpy(rsocket->rbuffers[i]->buffer,rcv_buff.data,rsocket->recv_len[i]);
rsocket->rsequence[i] = rcv_buff.seq;
[attachment deleted by ninja]
-
Niffiwan second patch committed. I'm taking a look at Iss Mneur's patch.
-
Making a note here: Antipodes was patched differently based on internal data for this issue so it doesn't match to trunk, however I like this revision of niffiwans, so it should replace the one currently in Antipodes. I'm away from home possibly for the entire weekend, so I don't have the ability to do it (unless I go on a spree to update the software on this laptop).
-
So I took a look at False 1 4 V512 A call of the 'memcpy' function will lead to overflow of the buffer '& x'. code pstypes.h 131 False
I can either fix it the easy way and just expand the memcpy into three assignments or fix it properly and convert vertex[/tt] to using vec3ds. I started to do the conversion, but when I realized that this is going to touch most of the graphics code I figured I should ask for an opinion? Do it right or fix it quick? The layout of this struct is abused all over the code in similar fashion os the one the PVS-Studio is complaining about.
I just took a look at this myself. In general, when there is a choice between "do it right" and "do it quick", I say "do it right". However, in this case, the "quick" fix isn't wrong. It really depends on how widespread the struct abuse is. (Keep in mind that the union of a1d and xyz isn't abuse; it's two ways of looking at the same data.)
On the other hand, zooming out one level, I don't even thing the = and == functions are supposed to be there. Notice that the struct includes padding for 4-byte alignment. But the struct isn't going to be the size the code thinks it is because it also needs to store information about the member functions. I would say, on the contrary, that the = and == functions should be removed (and from vec3d as well).
Let me therefore suggest this as the new version of the struct:
typedef struct vertex {
union {
struct {
float x, y, z; // world space position
} world;
float world_a1d[3];
}
union {
struct {
float sx, sy, sw; // screen space position (sw == 1/z)
} screen;
float screen_a1d[3];
}
float u, v; // texture position
ubyte r, g, b, a; // color. Use b for darkening;
ubyte spec_r, spec_g, spec_b, spec_a; //specular highlights -Bobboau
ubyte codes; // what sides of view pyramid this point is on/off. 0 = Inside view pyramid.
ubyte flags; // Projection flags. Indicates whether it is projected or not or if projection overflowed.
ubyte pad[2]; // pad structure to be 4 byte aligned.
} vertex;
-
Also, this will need to be handled by portej05:
False 2 9302 V507 Pointer to local array 'sym' is stored outside the scope of this array. Such a pointer will become invalid. code mspdb_callstack.cpp 89 False
Here's his comment at the top:
/* This file contains debugging utilities used only under VC2005+
* Maintained by: portej05 (please run patches past him before committing here!)
*/
/* Based on the ATL headers, however, updated to fix one or two bugs in the ATL headers
* and moved to the Sym*64 functions
*/
-
So I took a look at False 1 4 V512 A call of the 'memcpy' function will lead to overflow of the buffer '& x'. code pstypes.h 131 False
I can either fix it the easy way and just expand the memcpy into three assignments or fix it properly and convert vertex[/tt] to using vec3ds. I started to do the conversion, but when I realized that this is going to touch most of the graphics code I figured I should ask for an opinion? Do it right or fix it quick? The layout of this struct is abused all over the code in similar fashion os the one the PVS-Studio is complaining about.
I just took a look at this myself. In general, when there is a choice between "do it right" and "do it quick", I say "do it right". However, in this case, the "quick" fix isn't wrong. It really depends on how widespread the struct abuse is. (Keep in mind that the union of a1d and xyz isn't abuse; it's two ways of looking at the same data.)
The specific abuse that PVS-Studio complains about is only in one place because the the code in question is conveniently a function. On the other hand, the class of abuse, where the three floats are cast to/from a vec3d is pretty common, especially in the 3d graphics code.
On the other hand, zooming out one level, I don't even thing the = and == functions are supposed to be there. Notice that the struct includes padding for 4-byte alignment. But the struct isn't going to be the size the code thinks it is because it also needs to store information about the member functions. I would say, on the contrary, that the = and == functions should be removed (and from vec3d as well).
To be honest, its a question of code style and paradigm. We both know that :v: and the SCP have been fast and loose about style and paradigm. Do we adopt the more C++ object style that my patch was going for or do we revert this back a more procedural C style.
I agree about the operators. The == operator can be moved to be a global function, as an operator or otherwise, which will remove it from the vtable. And now that I have thought about it, I don't see the point the assignment operator, it is disingenuous at best, and should be removed.
As for the member functions of the vec3d, set_screen_vert doesn't appear to be used at all. The assignment operator is another one of the places that the trio of floats get abused and memcpyed to a vec3d. The equality operator for vec3d, like vertex can be made into a global function to remove it from the vtable and unlike the == operator for vertex it does not mislead and is used heavily.
Regarding the padding. I think it is odd that they would be manually padding the structure (you ask the compiler to do it for you instead), and sadly they didn't leave a note as to why it needs to be padded. Either way, the vtable will not break the 4 byte alignment because the vtable entries are always pointers (4 or 8 bytes which doesn't hurt you there). But you are correct, the struct is has more information tagged on it that is not being accounted for, which is why this heavy use of memset/memcpy gets us into trouble, see the question about style and paradigm above. On the other hand, empirically (VS2010 and the sizeof operator) the vertex struct is 44 bytes and the vect3d struct is 12 bytes, exactly as expected, so...
Let me therefore suggest this as the new version of the struct:
typedef struct vertex {
union {
struct {
float x, y, z; // world space position
} world;
float world_a1d[3];
}
union {
struct {
float sx, sy, sw; // screen space position (sw == 1/z)
} screen;
float screen_a1d[3];
}
float u, v; // texture position
ubyte r, g, b, a; // color. Use b for darkening;
ubyte spec_r, spec_g, spec_b, spec_a; //specular highlights -Bobboau
ubyte codes; // what sides of view pyramid this point is on/off. 0 = Inside view pyramid.
ubyte flags; // Projection flags. Indicates whether it is projected or not or if projection overflowed.
ubyte pad[2]; // pad structure to be 4 byte aligned.
} vertex;
I think it is a bit daft to just ignore vect3d in the vertex structure. They seem to go hand in hand through the code. It seems (based on the = operators and some of the code I have seen), vertex is the graphics code equivalent of vec3d.
-
Here's a patch for these two:
False 1 2570 V535 The variable 'i' is being used for this loop and for the outer loop. code controlsconfig.cpp 1543 False
False 1 2571 V535 The variable 'j' is being used for this loop and for the outer loop. code controlsconfig.cpp 1565 False
The use of i & j in the inner and outer loops is not a problem in these cases because the inner loop never returns to the outer loop, it always exits via break statements. I've added comments to suppress the error.
Index: code/controlconfig/controlsconfig.cpp
===================================================================
--- code/controlconfig/controlsconfig.cpp (revision 7626)
+++ code/controlconfig/controlsconfig.cpp (working copy)
@@ -1540,7 +1540,7 @@
for (i=0; i<JOY_TOTAL_BUTTONS; i++)
if (joy_down_count(i, 1)) {
j = i;
- for (i=0; i<CCFG_MAX; i++)
+ for (i=0; i<CCFG_MAX; i++) //-V535
if (Control_config[i].joy_id == j) {
z = i;
break;
@@ -1562,7 +1562,7 @@
for (i=0; i<CCFG_MAX; i++)
if (Control_config[i].joy_id == j) {
z = i;
- for (j=0; j<NUM_BUTTONS; j++){
+ for (j=0; j<NUM_BUTTONS; j++){ //-V535
CC_Buttons[gr_screen.res][j].button.reset();
}
break;
EDIT: updated to include both similar issues reported in controlsconfig.cpp
[attachment deleted by ninja]
-
Original post and output logs updated for r7626. With only a few more days of my PVS-Studio trial licence, looking to close out as many issues this weekend as possible.
Since the last update, a good number of level 2 bugs were fixed by Goober5000 (including a fix for a Volition bug in Campaigns (http://svn.icculus.org/fs2open?view=rev&revision=7618)) and Echelon9.
As at revision 7626:
To clear: 59 (-13)
Level 1: 31 (0)
Level 2: 28 (-13)
-
...
Regarding the padding. I think it is odd that they would be manually padding the structure (you ask the compiler to do it for you instead), and sadly they didn't leave a note as to why it needs to be padded. Either way, the vtable will not break the 4 byte alignment because the vtable entries are always pointers (4 or 8 bytes which doesn't hurt you there). But you are correct, the struct is has more information tagged on it that is not being accounted for, which is why this heavy use of memset/memcpy gets us into trouble, see the question about style and paradigm above. On the other hand, empirically (VS2010 and the sizeof operator) the vertex struct is 44 bytes and the vect3d struct is 12 bytes, exactly as expected, so...
Yes, it is odd that it is manually padded, although being fully aligned can help performance. Keep in mind that data structure alignment can change 32bit vs 64bit, so the manual pad may not fit all scenarios.
-
On the other hand, the class of abuse, where the three floats are cast to/from a vec3d is pretty common, especially in the 3d graphics code.
What exactly do you mean by the cast? If you mean the union of xyz and a1d, that's perfectly valid. If you mean the memcpy of a vec3d to the address of x, that's bad style, but it does take advantage of the struct layout. It would be better to add an a1d representation in a union and target the memcpy to that, as I illustrated in my previous post.
To be honest, its a question of code style and paradigm. We both know that :v: and the SCP have been fast and loose about style and paradigm. Do we adopt the more C++ object style that my patch was going for or do we revert this back a more procedural C style.
I would say to leave it as C. Generally, the rule of thumb is to leave something in the style that it was written in unless there's a compelling reason to do it otherwise or you're completely rewriting it. So I would move the == functions into a global function and remove the assignment and set_screen_vert functions, as you say.
Regarding the padding. I think it is odd that they would be manually padding the structure (you ask the compiler to do it for you instead), and sadly they didn't leave a note as to why it needs to be padded. Either way, the vtable will not break the 4 byte alignment because the vtable entries are always pointers (4 or 8 bytes which doesn't hurt you there). But you are correct, the struct is has more information tagged on it that is not being accounted for, which is why this heavy use of memset/memcpy gets us into trouble, see the question about style and paradigm above. On the other hand, empirically (VS2010 and the sizeof operator) the vertex struct is 44 bytes and the vect3d struct is 12 bytes, exactly as expected, so...
That's good news about the vtable; I wasn't sure whether it included anything in addition to the pointers. As for why they did it, I suspect there was some efficiency requirement, but I couldn't begin to guess where that came into play.
I think it is a bit daft to just ignore vect3d in the vertex structure. They seem to go hand in hand through the code. It seems (based on the = operators and some of the code I have seen), vertex is the graphics code equivalent of vec3d.
Well, the problem is that there are two sets of coordinates. Vec3d would seem to fit nicely with the world position, but the screen position isn't quite the same thing as a coordinate, due to the sw field (sw = 1/z). I would say that using a vec3d for those coordinates would be quite confusing to whoever works on the code in the future, so following the principle of least astonishment, it should remain as three floats. Once that was established, I wrote the two sets of coordinates in the same way so that the memcpy could be used in the same way for each.
-
On the other hand, the class of abuse, where the three floats are cast to/from a vec3d is pretty common, especially in the 3d graphics code.
What exactly do you mean by the cast? If you mean the union of xyz and a1d, that's perfectly valid. If you mean the memcpy of a vec3d to the address of x, that's bad style, but it does take advantage of the struct layout. It would be better to add an a1d representation in a union and target the memcpy to that, as I illustrated in my previous post.
I mean the creating/copying a vec3d to/from the address of the float x. I realize that is exactly what a union does, but the thing with a union is it documents the intention. Yes, I agree about the union.
Regarding the padding. I think it is odd that they would be manually padding the structure (you ask the compiler to do it for you instead), and sadly they didn't leave a note as to why it needs to be padded. Either way, the vtable will not break the 4 byte alignment because the vtable entries are always pointers (4 or 8 bytes which doesn't hurt you there). But you are correct, the struct is has more information tagged on it that is not being accounted for, which is why this heavy use of memset/memcpy gets us into trouble, see the question about style and paradigm above. On the other hand, empirically (VS2010 and the sizeof operator) the vertex struct is 44 bytes and the vect3d struct is 12 bytes, exactly as expected, so...
That's good news about the vtable; I wasn't sure whether it included anything in addition to the pointers. As for why they did it, I suspect there was some efficiency requirement, but I couldn't begin to guess where that came into play.
I would assume it has something to do with performance as well, but, damned if I know what they were trying to get at. And at that, as E9 articulated better than I did, alignment requirements change depending on your architecture.
I think it is a bit daft to just ignore vect3d in the vertex structure. They seem to go hand in hand through the code. It seems (based on the = operators and some of the code I have seen), vertex is the graphics code equivalent of vec3d.
Well, the problem is that there are two sets of coordinates. Vec3d would seem to fit nicely with the world position, but the screen position isn't quite the same thing as a coordinate, due to the sw field (sw = 1/z). I would say that using a vec3d for those coordinates would be quite confusing to whoever works on the code in the future, so following the principle of least astonishment, it should remain as three floats. Once that was established, I wrote the two sets of coordinates in the same way so that the memcpy could be used in the same way for each.
Okay. I will do something similar then. I will post the patch before I apply it.
-
False 1 10012 V568 It's odd that the argument of sizeof() operator is the '(short) method' expression. code multimsgs.cpp 2783 False
The variable "method" is defined as an int and its contents are flags such as SHIP_VANISHED, SHIP_DEPARTED_WARP, etc (4 flags total). The "(short)" cast could (depending on the compiler & architecture implementation) truncate the int. This isn't a problem currently because of the small number of flags used, however if more flags are added in future then those flags could be silently truncated.
Given that this packet is going to be small (i.e. much less than MAX_PACKET_SIZE) I've changed ADD_SHORT to ADD_INT and removed the cast.
In addition, I've updated the receiving end to match, i.e. use an int instead of a short for the "s_method".
Index: code/network/multimsgs.cpp
===================================================================
--- code/network/multimsgs.cpp (revision 7648)
+++ code/network/multimsgs.cpp (working copy)
@@ -2780,7 +2780,7 @@
BUILD_HEADER(SHIP_DEPART);
ADD_USHORT( signature );
- ADD_SHORT( (short) method);
+ ADD_INT( method );
multi_io_send_to_all_reliable(data, packet_size);
}
@@ -2791,11 +2791,11 @@
int offset;
object *objp;
ushort signature;
- short s_method;
+ int s_method;
offset = HEADER_LENGTH;
GET_USHORT( signature );
- GET_SHORT(s_method);
+ GET_INT(s_method);
PACKET_SET_SIZE();
// find the object which is departing
edit: fix typo - also remembered that when you send a packet, someone is supposed to receive it as well :D
[attachment deleted by ninja]
-
Another one...
False 1 3485 V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 1366, 1381. code fs2netd_client.cpp 1381 False
This is a false alarm. The vector "FS2NetD_file_list" is modified by the function "FS2NetD_GetMissionsList" (passed by reference when run from function "fs2netd_get_valid_missions_do") in between the .clears() at lines 1366 & 1381.
Index: code/fs2netd/fs2netd_client.cpp
===================================================================
--- code/fs2netd/fs2netd_client.cpp (revision 7631)
+++ code/fs2netd/fs2netd_client.cpp (working copy)
@@ -1378,7 +1378,7 @@
In_process = false;
Local_timeout = -1;
- FS2NetD_file_list.clear();
+ FS2NetD_file_list.clear(); //-V586
switch (rc) {
// canceled by popup
EDIT: added the attachment
[attachment deleted by ninja]
-
Marked as a false alarm.
-
Here's a patch for these two:
False 1 2570 V535 The variable 'i' is being used for this loop and for the outer loop. code controlsconfig.cpp 1543 False
False 1 2571 V535 The variable 'j' is being used for this loop and for the outer loop. code controlsconfig.cpp 1565 False
Committed a false positive mark for the second one.
-
The_E and niffiwan are continuing to plow through the remaining reports. Original post has been updated for r7644.
Less than thirty instances of each priority level to address!
As at revision 7644:
To clear: 57 (-2)
Level 1: 29 (-2)
Level 2: 28 (0)
-
I believe this is another false alarm:
False 1 11773 V512 A call of the 'memset' function will lead to overflow of the buffer '& Sexp_nodes [old_size]'. code sexp.cpp 1038 False
While a case could be made for using STL Vectors, the code as is seems to be correctly writing to the intended memory locations. Num_sexp_nodes is incremented by SEXP_NODE_INCREMENT just before the vm_realloc, so there are always going to be exactly 250 new nodes to zero.
I also ran this through the debugger and checked the value of Sexp_nodes[250] during the 1st resize, both before and after the memset it contained random data, not zeros (unlike Sexp_nodes[249] which changed from random data to all zeros).
Index: code/parse/sexp.cpp
===================================================================
--- code/parse/sexp.cpp (revision 7648)
+++ code/parse/sexp.cpp (working copy)
@@ -1035,7 +1035,7 @@
nprintf(("SEXP", "Bumping dynamic sexp node limit from %d to %d...\n", old_size, Num_sexp_nodes));
// clear all the new sexp nodes we just allocated
- memset(&Sexp_nodes[old_size], 0, sizeof(sexp_node) * SEXP_NODE_INCREMENT);
+ memset(&Sexp_nodes[old_size], 0, sizeof(sexp_node) * SEXP_NODE_INCREMENT); //-V512
// our new sexp is the first out of the ones we just created
node = old_size;
[attachment deleted by ninja]
-
I believe this is another false alarm:
False 1 11773 V512 A call of the 'memset' function will lead to overflow of the buffer '& Sexp_nodes [old_size]'. code sexp.cpp 1038 False
While a case could be made for using STL Vectors, the code as is seems to be correctly writing to the intended memory locations. Num_sexp_nodes is incremented by SEXP_NODE_INCREMENT just before the vm_realloc, so there are always going to be exactly 250 new nodes to zero.
I also ran this through the debugger and checked the value of Sexp_nodes[250] during the 1st resize, both before and after the memset it contained random data, not zeros (unlike Sexp_nodes[249] which changed from random data to all zeros).
Index: code/parse/sexp.cpp
===================================================================
--- code/parse/sexp.cpp (revision 7648)
+++ code/parse/sexp.cpp (working copy)
@@ -1035,7 +1035,7 @@
nprintf(("SEXP", "Bumping dynamic sexp node limit from %d to %d...\n", old_size, Num_sexp_nodes));
// clear all the new sexp nodes we just allocated
- memset(&Sexp_nodes[old_size], 0, sizeof(sexp_node) * SEXP_NODE_INCREMENT);
+ memset(&Sexp_nodes[old_size], 0, sizeof(sexp_node) * SEXP_NODE_INCREMENT); //-V512
// our new sexp is the first out of the ones we just created
node = old_size;
I agree. Committed.
(A) Yes, it should be rewritten with an STL container
(B) A false positive for now.
-
False 1 10012 V568 It's odd that the argument of sizeof() operator is the '(short) method' expression. code multimsgs.cpp 2783 False
The variable "method" is defined as an int and its contents are flags such as SHIP_VANISHED, SHIP_DEPARTED_WARP, etc (4 flags total). The "(short)" cast could (depending on the compiler & architecture implementation) truncate the int. This isn't a problem currently because of the small number of flags used, however if more flags are added in future then those flags could be silently truncated.
Given that this packet is going to be small (i.e. much less than MAX_PACKET_SIZE) I've changed ADD_SHORT to ADD_INT and removed the cast.
In addition, I've updated the receiving end to match, i.e. use an int instead of a short for the "s_method".
Index: code/network/multimsgs.cpp
===================================================================
--- code/network/multimsgs.cpp (revision 7648)
+++ code/network/multimsgs.cpp (working copy)
@@ -2780,7 +2780,7 @@
BUILD_HEADER(SHIP_DEPART);
ADD_USHORT( signature );
- ADD_SHORT( (short) method);
+ ADD_INT( method );
multi_io_send_to_all_reliable(data, packet_size);
}
@@ -2791,11 +2791,11 @@
int offset;
object *objp;
ushort signature;
- short s_method;
+ int s_method;
offset = HEADER_LENGTH;
GET_USHORT( signature );
- GET_SHORT(s_method);
+ GET_INT(s_method);
PACKET_SET_SIZE();
// find the object which is departing
edit: fix typo - also remembered that when you send a packet, someone is supposed to receive it as well :D
Committed (http://svn.icculus.org/fs2open?view=rev&revision=7650) now that we have the receive side too :)
-
False 1 11540 V512 A call of the 'memcpy' function will lead to overflow of the buffer '& ip_addr.sin_addr'. code psnet2.cpp 733 False
From checking the headers that define the struct "ip_addr" on Linux (uint32_t) & Windows (u_long) & reading up on int/long/longlong/(etc) sizes I've come to the conclusion that the struct size (for 32bit & 64bit) is 4 bytes. But, given that the size *could* vary for other platforms, rather than hard-code it I thought using a sizeof() was the best approach.
Index: code/network/psnet2.cpp
===================================================================
--- code/network/psnet2.cpp (revision 7660)
+++ code/network/psnet2.cpp (working copy)
@@ -730,7 +730,7 @@
#else
if ( (custom_address.sin_addr.s_addr = inet_addr(custom_ip)) != INADDR_NONE ) {
#endif
- memcpy(&ip_addr.sin_addr, &custom_address.sin_addr, 6);
+ memcpy(&ip_addr.sin_addr, &custom_address.sin_addr, sizeof(custom_address.sin_addr));
} else {
ml_printf("WARNING => psnet_get_ip() custom IP is invalid: %s", custom_ip);
}
[attachment deleted by ninja]
-
False 1 11540 V512 A call of the 'memcpy' function will lead to overflow of the buffer '& ip_addr.sin_addr'. code psnet2.cpp 733 False
From checking the headers that define the struct "ip_addr" on Linux (uint32_t) & Windows (u_long) & reading up on int/long/longlong/(etc) sizes I've come to the conclusion that the struct size (for 32bit & 64bit) is 4 bytes. But, given that the size *could* vary for other platforms, rather than hard-code it I thought using a sizeof() was the best approach.
Committed r7665. keep up the good work!
Now that I'm back home, I'll look at tackling some of these too.
False 2 11499 V802 On 32-bit platform, structure size can be reduced from 20 to 16 bytes by rearranging the fields according to their sizes in decreasing order. code png.h 659 False
This one can not only be ignored, but it can't safely be commented out since that involves changing a provided component of an external library. I can pass it on to the PNG folks and let them do it though.
-
Thanks Zacam! Getting closer to dealing with them all before the 8th :)
These next 5 issues are all for the same reason, and are false positives.
False 1 11536 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'from_addr.addr'. code psnet2.cpp 454 False
False 1 11545 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'address->addr'. code psnet2.cpp 875 False
False 1 11564 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'd3_rcv_addr.addr'. code psnet2.cpp 1443 False
False 1 11582 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'from_addr->addr'. code psnet2.cpp 1787 False
False 1 11587 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'd3_rcv_addr.addr'. code psnet2.cpp 1928 False
The struct net_addr has a field (addr) with a dual purpose. 4 bytes are used for TCP, 6 bytes are used for IPX. Naturally when copying TCP data into this field, we underflow the buffer. However, in all cases listed either the entire net_addr struct or just net_addr.addr has been memset to 0x00 (or 0) prior to writing the 4 bytes of data into net_addr.addr. i.e. there's no chance for garbage data to be in the field.
Index: code/network/psnet2.cpp
===================================================================
--- code/network/psnet2.cpp (revision 7665)
+++ code/network/psnet2.cpp (working copy)
@@ -451,9 +451,9 @@
from_addr.port = ntohs( ip_addr.sin_port );
memset(from_addr.addr, 0x00, 6);
#ifdef _WIN32
- memcpy(from_addr.addr, &ip_addr.sin_addr.S_un.S_addr, 4);
+ memcpy(from_addr.addr, &ip_addr.sin_addr.S_un.S_addr, 4); //-V512
#else
- memcpy(from_addr.addr, &ip_addr.sin_addr.s_addr, 4);
+ memcpy(from_addr.addr, &ip_addr.sin_addr.s_addr, 4); //-V512
#endif
break;
@@ -872,7 +872,7 @@
}
memset(address->addr, 0x00, 6);
- memcpy(address->addr, &addr.s_addr, 4);
+ memcpy(address->addr, &addr.s_addr, 4); //-V512
if ( port ){
address->port = (ushort)(atoi(port));
}
@@ -1440,7 +1440,7 @@
rcv_buff.seq = INTEL_SHORT( rcv_buff.seq ); //-V570
rcv_buff.data_len = INTEL_SHORT( rcv_buff.data_len ); //-V570
rcv_buff.send_time = INTEL_FLOAT( &rcv_buff.send_time );
- memcpy(d3_rcv_addr.addr, &tcp_addr->sin_addr.s_addr, 4);
+ memcpy(d3_rcv_addr.addr, &tcp_addr->sin_addr.s_addr, 4); //-V512
d3_rcv_addr.port = tcp_addr->sin_port;
d3_rcv_addr.type = NET_TCP;
link_type = NET_TCP;
@@ -1784,9 +1784,9 @@
from_addr->port = ntohs( ip_addr->sin_port );
from_addr->type = NET_TCP;
#ifdef _WIN32
- memcpy(from_addr->addr, &ip_addr->sin_addr.S_un.S_addr, 4);
+ memcpy(from_addr->addr, &ip_addr->sin_addr.S_un.S_addr, 4); //-V512
#else
- memcpy(from_addr->addr, &ip_addr->sin_addr.s_addr, 4);
+ memcpy(from_addr->addr, &ip_addr->sin_addr.s_addr, 4); //-V512
#endif
break;
@@ -1925,7 +1925,7 @@
ml_printf("Unable to send UDP packet in nw_ConnectToServer()! -- %d",WSAGetLastError());
return;
}
- memcpy(d3_rcv_addr.addr, &sockaddr.sin_addr.s_addr, 4);
+ memcpy(d3_rcv_addr.addr, &sockaddr.sin_addr.s_addr, 4); //-V512
d3_rcv_addr.port = sockaddr.sin_port;
d3_rcv_addr.type = NET_TCP;
typeless_sock = Unreliable_socket;
Lastly, the way that PVS studio is not reporting errors on nearly identical lines of code is making me think that it is only evaluating #ifdef _WIN32 sections of the code - meaning that Linux/Mac specific code is probably not being checked. e.g. PVS studio reported the 1st memcpy as an error, but not the 2nd.
#ifdef _WIN32
memcpy(from_addr->addr, &ip_addr->sin_addr.S_un.S_addr, 4); //-V512
#else
memcpy(from_addr->addr, &ip_addr->sin_addr.s_addr, 4); //-V512
#endif
Probably too late to worry about this now though!
edit: fix typo
[attachment deleted by ninja]
-
Two more false positives.
False 1 11107 V512 A call of the 'memcpy' function will lead to underflow of the buffer '& bit_24'. code packunpack.cpp 546 False
False 2 11112 V512 A call of the 'memcpy' function will lead to underflow of the buffer '& bit_24'. code packunpack.cpp 630 False
In both cases, the last byte (alpha channel) is not memcpy'd. However, the function "convert_24_to_16" ignores the alpha byte it is passed and always sets it to 1, so the garbage data is never used.
Index: code/anim/packunpack.cpp
===================================================================
--- code/anim/packunpack.cpp (revision 7666)
+++ code/anim/packunpack.cpp (working copy)
@@ -543,7 +543,7 @@
}
} else {
// stuff the 24 bit value
- memcpy(&bit_24, &ai->parent->palette[pix * 3], 3);
+ memcpy(&bit_24, &ai->parent->palette[pix * 3], 3); //-V512
// convert to 16 bit
convert_24_to_16(bit_24, &bit_16);
@@ -627,7 +627,7 @@
}
} else {
// stuff the 24 bit value
- memcpy(&bit_24, &ai->parent->palette[pix * 3], 3);
+ memcpy(&bit_24, &ai->parent->palette[pix * 3], 3); //-V512
// convert to 16 bit
convert_24_to_16(bit_24, &bit_16);
[attachment deleted by ninja]
-
Niffiwan's two most recent patches committed.
-
Almost there! The_E and niffiwan are continuing to plow through the remaining reports. Original post has been updated for r7670.
Upon review of the remaining issues, it seems that PVS-Studio may count subsequent line items that cause the same issue twice. A direct count of the issues reported at each of the levels arrives at a lower number.
As at revision 7670:
To clear: 26 (-31)
Level 1: 13 (-16)
Level 2: 13 (-15)
The actual effective count is even lower, as pointed out above there are false reports within third party library headers which we can't surpress.
My view of the most important to fix -- due to memory instability linked issues are:
Level 1
False 1 4 V512 A call of the 'memcpy' function will lead to overflow of the buffer '& x'. code pstypes.h 131 False
Level 2
False 2 181 V506 Pointer to local variable 'Default_video_settings' is stored outside the scope of this variable. Such a pointer will become invalid. code 2d.cpp 507 False
False 2 13999 V506 Pointer to local variable 'pos' is stored outside the scope of this variable. Such a pointer will become invalid. code shipfx.cpp 1075 False
False 2 9288 V507 Pointer to local array 'sym' is stored outside the scope of this array. Such a pointer will become invalid. code mspdb_callstack.cpp 89 False
False 2 14348 V507 Pointer to local array 'title_str' is stored outside the scope of this array. Such a pointer will become invalid. code stand_gui.cpp 2085 False
False 2 14590 V507 Pointer to local array 'rgdwAxes' is stored outside the scope of this array. Such a pointer will become invalid. code swff_lib.cpp 1909 False
-
Level 1
False 1 4 V512 A call of the 'memcpy' function will lead to overflow of the buffer '& x'. code pstypes.h 131 False
This is the issue that Goober and I were discussing previously in this thread. I hope to have a patch posted for reviewcommitted by tonight.
EDIT: Ran the patch by Zacam, neither of us found any issues with it. Committed as 7677.
-
Thanks Iss Mneur
-
I think we're just about there. A few considerable fixes in the last day from Iss Mneur and Goober5000 addressed a long standing issue with the vertex code.
As at revision 7681:
To clear: 23 (-3)
Level 1: 12 (-1)
Level 2: 11 (-2) -- this includes two false positives which cannot be surpressed as they sit in an external library
Level 1
False 1 2868 V512 A call of the 'memcpy' function will lead to underflow of the buffer '& stlen'. code demo.cpp 555 False
False 1 2873 V512 A call of the 'memcpy' function will lead to underflow of the buffer '& stlen'. code demo.cpp 556 False
False 1 2895 V512 A call of the 'memcpy' function will lead to underflow of the buffer '& stlen'. code demo.cpp 592 False
False 1 2949 V512 A call of the 'memcpy' function will lead to underflow of the buffer '& stlen'. code demo.cpp 846 False
False 1 2953 V512 A call of the 'memcpy' function will lead to underflow of the buffer '& stlen'. code demo.cpp 847 False
False 1 2969 V512 A call of the 'memcpy' function will lead to underflow of the buffer '& stlen'. code demo.cpp 888 False
False 1 3279 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'lvl1_block'. code encrypt.cpp 330 False
False 1 3291 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'lvl1_block'. code encrypt.cpp 386 False
False 1 9282 V512 A call of the 'memset' function will lead to underflow of the buffer '& stackFrame'. code mspdb_callstack.cpp 189 False
False 1 9526 V512 A call of the 'memmove' function will lead to underflow of the buffer '& oo_arrive_time [shipp - Ships][0]'. code multi_obj.cpp 786 False
False 1 9834 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'from'. code multilag.cpp 265 False
False 1 10535 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'Multi_autojoin_addr.addr'. code multiui.cpp 961 False
Level 2
False 2 9279 V507 Pointer to local array 'sym' is stored outside the scope of this array. Such a pointer will become invalid. code mspdb_callstack.cpp 89 False
False 2 14339 V507 Pointer to local array 'title_str' is stored outside the scope of this array. Such a pointer will become invalid. code stand_gui.cpp 2085 False
False 2 14581 V507 Pointer to local array 'rgdwAxes' is stored outside the scope of this array. Such a pointer will become invalid. code swff_lib.cpp 1909 False
False 2 4910 V513 Use _beginthreadex/_endthreadex functions instead of CreateThread/ExitThread functions. code joy.cpp 413 False
False 2 9283 V513 Use _beginthreadex/_endthreadex functions instead of CreateThread/ExitThread functions. code mspdb_callstack.cpp 301 False
False 2 14346 V513 Use _beginthreadex/_endthreadex functions instead of CreateThread/ExitThread functions. code stand_gui.cpp 2419 False
False 2 11 V802 On 32-bit platform, structure size can be reduced from 28 to 24 bytes by rearranging the fields according to their sizes in decreasing order. code 2d.h 47 False
False 2 167 V802 On 32-bit platform, structure size can be reduced from 24 to 20 bytes by rearranging the fields according to their sizes in decreasing order. code controlsconfig.h 39 False
False 2 1602 V802 On 32-bit platform, structure size can be reduced from 36 to 32 bytes by rearranging the fields according to their sizes in decreasing order. code mmreg.h 1841 False
False 2 11491 V802 On 32-bit platform, structure size can be reduced from 20 to 16 bytes by rearranging the fields according to their sizes in decreasing order. code png.h 659 False
False 2 14777 V802 On 32-bit platform, structure size can be reduced from 20 to 18 bytes by rearranging the fields according to their sizes in decreasing order. code tgautils.cpp 43 False
-
Coming right down, in latest trunk r7695.
As at revision 7695:
To clear: 18 (-5)
Level 1: 7 (-5)
Level 2: 11 (0) -- this includes three false positives which cannot be surpressed as they sit in an external library
Level 1
False 1 3150 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'lvl1_block'. code encrypt.cpp 330 False
False 1 3162 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'lvl1_block'. code encrypt.cpp 386 False
False 1 9153 V512 A call of the 'memset' function will lead to underflow of the buffer '& stackFrame'. code mspdb_callstack.cpp 189 False
False 1 9397 V512 A call of the 'memmove' function will lead to underflow of the buffer '& oo_arrive_time [shipp - Ships][0]'. code multi_obj.cpp 786 False
False 1 9705 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'from'. code multilag.cpp 265 False
False 1 10406 V512 A call of the 'memcpy' function will lead to underflow of the buffer 'Multi_autojoin_addr.addr'. code multiui.cpp 961 False
False 1 2590 V535 The variable 'j' is being used for this loop and for the outer loop. code controlsconfig.cpp 1613 False
Level 2
False 2 9150 V507 Pointer to local array 'sym' is stored outside the scope of this array. Such a pointer will become invalid. code mspdb_callstack.cpp 89 False
False 2 14210 V507 Pointer to local array 'title_str' is stored outside the scope of this array. Such a pointer will become invalid. code stand_gui.cpp 2085 False
False 2 14452 V507 Pointer to local array 'rgdwAxes' is stored outside the scope of this array. Such a pointer will become invalid. code swff_lib.cpp 1909 False
False 2 4781 V513 Use _beginthreadex/_endthreadex functions instead of CreateThread/ExitThread functions. code joy.cpp 396 False
False 2 9154 V513 Use _beginthreadex/_endthreadex functions instead of CreateThread/ExitThread functions. code mspdb_callstack.cpp 301 False
False 2 14217 V513 Use _beginthreadex/_endthreadex functions instead of CreateThread/ExitThread functions. code stand_gui.cpp 2419 False
False 2 11 V802 On 32-bit platform, structure size can be reduced from 28 to 24 bytes by rearranging the fields according to their sizes in decreasing order. code 2d.h 47 False
False 2 167 V802 On 32-bit platform, structure size can be reduced from 24 to 20 bytes by rearranging the fields according to their sizes in decreasing order. code controlsconfig.h 39 False
False 2 1602 V802 On 32-bit platform, structure size can be reduced from 36 to 32 bytes by rearranging the fields according to their sizes in decreasing order. code mmreg.h 1841 False
False 2 11362 V802 On 32-bit platform, structure size can be reduced from 20 to 16 bytes by rearranging the fields according to their sizes in decreasing order. code png.h 659 False
False 2 14648 V802 On 32-bit platform, structure size can be reduced from 20 to 18 bytes by rearranging the fields according to their sizes in decreasing order. code tgautils.cpp 43 False
-
Level 1
False 1 4 V512 A call of the 'memcpy' function will lead to overflow of the buffer '& x'. code pstypes.h 131 False
This is the issue that Goober and I were discussing previously in this thread. I hope to have a patch posted for reviewcommitted by tonight.
EDIT: Ran the patch by Zacam, neither of us found any issues with it. Committed as 7677.
Unfortunately, there is now a Mantis Issue (http://scp.indiegames.us/mantis/view.php?id=2499) as a result. Never thought to check models by rebuilding the cache files. *sighs*
I'm hoping this one can be quickly resolved though.
**Edit: Issue resolved as of r7697.
-
**Edit: Issue resolved as of r7697.
Great to hear IssMneur and Zacam.
-
The thanks is to IssMneur who figured it out, I was just in the position to commit ti after testing it.
Oh, and I was off by one, 7697 was the AP commit, 7698 was the Trunk commit.
-
Updated original post
As at revision 7781:
To clear: 17 (-1)
Level 1: 6 (-1)
Level 2: 11 (0) -- this includes three false positives which cannot be surpressed as they sit in an external library
-
Updated original post
As at revision 7837:
To clear: 15 (-2)
Level 1: 6 (0)
Level 2: 9 (-2) -- this includes three false positives which cannot be surpressed as they sit in an external library