[niffiwan]

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.

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.

Code: [Select]

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]

[Echelon9]

Niffiwan second patch committed. I'm taking a look at Iss Mneur's patch.

[Zacam]

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).

[Goober5000]

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.

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:

Code: [Select]

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;

[Goober5000]

Also, this will need to be handled by portej05:

Code: [Select]

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:

Code: [Select]

/* 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
 */

[Iss Mneur]

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.

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 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:

Code: [Select]

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.

[niffiwan]

Here's a patch for these two:

Code: [Select]

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.

Code: [Select]

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]

[Echelon9]

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) and Echelon9.

As at revision 7626:
To clear: 59 (-13)
Level 1: 31 (0)
Level 2: 28 (-13)

[Echelon9]

...
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.

[Goober5000]

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 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.

[Iss Mneur]

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.

[niffiwan]

Code: [Select]

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".

Code: [Select]

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 

[attachment deleted by ninja]

[niffiwan]

Another one...

Code: [Select]

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.

Code: [Select]

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]

[The E]

Marked as a false alarm.

[Echelon9]

Here's a patch for these two:

Code: [Select]

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.

[Echelon9]

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)

[niffiwan]

I believe this is another false alarm:

Code: [Select]

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).

Code: [Select]

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]

[Echelon9]

I believe this is another false alarm:

Code: [Select]

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).

Code: [Select]

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.

[Echelon9]

Code: [Select]

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".

Code: [Select]

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 

Committed now that we have the receive side too

[niffiwan]

Code: [Select]

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.

Code: [Select]

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]