Author Topic: PVS-Studio: C/C++ source code analysis and checking (updated for r7855)  (Read 14546 times)

0 Members and 1 Guest are viewing this topic.

Offline Echelon9

  • 210
PVS-Studio: C/C++ source code analysis and checking (updated for r7855)
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:

Code: [Select]
'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, 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, Clang, Qt, Chromium and TortoiseSVN.

So I wrote them a quick email explaining what FS2_Open SCP is, and was graced with a trial licence!

Quote
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:
Code: [Select]
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:
Code: [Select]
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:
Code: [Select]
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]
« Last Edit: October 03, 2011, 08:04:20 am by Echelon9 »

 

Offline Echelon9

  • 210
Re: PVS-Studio: C/C++ source code analysis and checking
To get this bug-hunt started, here is an example:

Code: [Select]
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
Quote
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.


Code: [Select]
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;
   }
   ...
}

 

Offline niffiwan

  • 211
  • Eluder Class
Re: PVS-Studio: C/C++ source code analysis and checking
Here's a patch to deal with essentially the same issue in two places:

Code: [Select]
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:

Code: [Select]
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.

Code: [Select]
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]
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline niffiwan

  • 211
  • Eluder Class
Re: PVS-Studio: C/C++ source code analysis and checking
And another one for a typo / cut-n-paste error:

Code: [Select]
False 1 8209 V523 The 'then' statement is equivalent to the 'else' statement. code missiontraining.cpp 486 False

Code: [Select]
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]
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline Echelon9

  • 210
Re: PVS-Studio: C/C++ source code analysis and checking
Thanks niffiwan! The patch from the first post, which addresses the below two reports, has been committed to trunk as of r7565.

Code: [Select]
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.

Code: [Select]
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.

 

Offline niffiwan

  • 211
  • Eluder Class
Re: PVS-Studio: C/C++ source code analysis and checking
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?
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline karajorma

  • King Louie - Jungle VIP
  • Administrator
  • 214
    • Karajorma's Freespace FAQ
Re: PVS-Studio: C/C++ source code analysis and checking
Fixed a bug in my code.

I'll fix the bugs in other people's code tomorrow. :p
Karajorma's Freespace FAQ. It's almost like asking me yourself.

[ Diaspora ] - [ Seeds Of Rebellion ] - [ Mind Games ]

 

Offline niffiwan

  • 211
  • Eluder Class
Re: PVS-Studio: C/C++ source code analysis and checking
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.

Code: [Select]
False 1 14823 V523 The 'then' statement is equivalent to the 'else' statement. code techmenu.cpp 614 False

Code: [Select]
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]
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline Echelon9

  • 210
Re: PVS-Studio: C/C++ source code analysis and checking
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.

Code: [Select]
False 1 8209 V523 The 'then' statement is equivalent to the 'else' statement. code missiontraining.cpp 486 False

 

Offline Echelon9

  • 210
Re: PVS-Studio: C/C++ source code analysis and checking
Another simple fix, in this case it's the scroll offset in the techroom info box....

Code: [Select]
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.

 

Offline Iss Mneur

  • 210
  • TODO:
Re: PVS-Studio: C/C++ source code analysis and checking
Why are we just commenting out the code rather than outright removing it from the repo?
"I love deadlines. I like the whooshing sound they make as they fly by." -Douglas Adams
wxLauncher 0.9.4 public beta (now with no config file editing for FRED) | wxLauncher 2.0 Request for Comments

 

Offline Echelon9

  • 210
Re: PVS-Studio: C/C++ source code analysis and checking (updated for r7584)
Great progress is being seen in the SCP trunk, 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)

 

Offline niffiwan

  • 211
  • Eluder Class
Re: PVS-Studio: C/C++ source code analysis and checking
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.
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline niffiwan

  • 211
  • Eluder Class
Re: PVS-Studio: C/C++ source code analysis and checking (updated for r7584)
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!

Code: [Select]
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]
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline Zacam

  • Magnificent Bastard
  • Administrator
  • 211
  • I go Sledge-O-Matic on Spammers
    • Minecraft
    • Steam
    • Twitter
    • ModDB Feature
Re: PVS-Studio: C/C++ source code analysis and checking (updated for r7584)

Committed that for ya. r7599.
Report MediaVP issues, now on the MediaVP Mantis! Read all about it Here!
Talk with the community on Discord
"If you can keep a level head in all this confusion, you just don't understand the situation"

¤[D+¬>

[08/01 16:53:11] <sigtau> EveningTea: I have decided that I am a 32-bit registerkin.  Pronouns are eax, ebx, ecx, edx.
[08/01 16:53:31] <EveningTea> dhauidahh
[08/01 16:53:32] <EveningTea> sak
[08/01 16:53:40] * EveningTea froths at the mouth
[08/01 16:53:40] <sigtau> i broke him, boys

 

Offline Echelon9

  • 210
Re: PVS-Studio: C/C++ source code analysis and checking (updated for 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)

« Last Edit: August 31, 2011, 09:41:19 am by Echelon9 »

 

Offline niffiwan

  • 211
  • Eluder Class
Re: PVS-Studio: C/C++ source code analysis and checking (updated for r7584)
Thanks Zacam

Next up is a fix that I'd really like other coders to have a look over.

Code: [Select]
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...

Code: [Select]
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]
Creating a fs2_open.log | Red Alert Bug = Hex Edit | MediaVPs 2014: Bigger HUD gauges | 32bit libs for 64bit Ubuntu
----
Debian Packages (testing/unstable): Freespace2 | wxLauncher
----
m|m: I think I'm suffering from Stockholm syndrome. Bmpman is starting to make sense and it's actually written reasonably well...

 

Offline Echelon9

  • 210
Re: PVS-Studio: C/C++ source code analysis and checking (updated for r7584)
Thanks Zacam

Next up is a fix that I'd really like other coders to have a look over.

Code: [Select]
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, for at least the short term.
« Last Edit: August 31, 2011, 08:48:41 am by Echelon9 »

 

Offline Echelon9

  • 210
Re: PVS-Studio: C/C++ source code analysis and checking (updated for r7606)
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)


 

Offline Iss Mneur

  • 210
  • TODO:
Re: PVS-Studio: C/C++ source code analysis and checking (updated for r7606)
So I took a look at
Code: [Select]
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.

Code: [Select]
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
"I love deadlines. I like the whooshing sound they make as they fly by." -Douglas Adams
wxLauncher 0.9.4 public beta (now with no config file editing for FRED) | wxLauncher 2.0 Request for Comments