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

0 Members and 2 Guests are viewing this topic.

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

Committed r7665. keep up the good work!

Now that I'm back home, I'll look at tackling some of these too.

Code: [Select]
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.
« Last Edit: September 07, 2011, 05:11:11 am by Zacam »
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 niffiwan

  • 211
  • Eluder Class
Re: PVS-Studio: C/C++ source code analysis and checking (updated for r7655)
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.
Code: [Select]
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.

Code: [Select]
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.
Code: [Select]
#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]
« Last Edit: September 07, 2011, 06:27:16 am by niffiwan »
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 r7655)
Two more false positives.
Code: [Select]
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.

Code: [Select]
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]
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 r7655)
Niffiwan's two most recent patches committed.

 

Offline Echelon9

  • 210
Re: PVS-Studio: C/C++ source code analysis and checking (updated for r7670)
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
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
Level 2
Code: [Select]
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
« Last Edit: September 07, 2011, 08:33:36 am by Echelon9 »

 

Offline Iss Mneur

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

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.
« Last Edit: September 08, 2011, 12:40:59 am by Iss Mneur »
"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 r7670)
Thanks Iss Mneur

 

Offline Echelon9

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

 

Offline Echelon9

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

 

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 r7695)
Level 1
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

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 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.
« Last Edit: September 11, 2011, 01:21:06 am by Zacam »
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 r7695)
**Edit: Issue resolved as of r7697.
Great to hear IssMneur and Zacam.
« Last Edit: September 11, 2011, 08:56:21 am by Echelon9 »

 

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

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


  

Offline Echelon9

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