{"id":388239,"date":"2024-06-29T07:49:22","date_gmt":"2024-06-29T07:49:22","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=388239"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=388239","title":{"rendered":"<span>Top 10 Bugs Found in C++ Projects in 2020<\/span>"},"content":{"rendered":"<div><!--[--><!--]--><\/div>\n<div id=\"post-content-body\">\n<div>\n<div class=\"article-formatted-body article-formatted-body article-formatted-body_version-1\">\n<div xmlns=\"http:\/\/www.w3.org\/1999\/xhtml\">\n<div style=\"text-align:center;\"><img decoding=\"async\" src=\"https:\/\/habrastorage.org\/r\/w1560\/webt\/ed\/qz\/kt\/edqzktmzcrowmhowiz0-csjx1hy.png\" alt=\"image1.png\" data-src=\"https:\/\/habrastorage.org\/webt\/ed\/qz\/kt\/edqzktmzcrowmhowiz0-csjx1hy.png\"\/><\/div>\n<p>  It&#8217;s winter outside, the year is coming to an end, which means it&#8217;s time to review the most notable errors the PVS-Studio analyzer detected in 2020.<br \/>  <a name=\"habracut\"><\/a><br \/>  In the past year, we introduced many new diagnostic rules that detected these errors and placed them at the top. We&#8217;ve also enhanced the analyzer&#8217;s core and added new use case scenarios. You can learn more about this in our <a href=\"https:\/\/www.viva64.com\/en\/b\">blog<\/a>. Let me remind you that our analyzer also supports C# and Java. Check out my colleagues&#8217; articles for more information on those languages. Now let&#8217;s move on to the most memorable bugs PVS-Studio found in open source projects over the past year.<\/p>\n<h2>No. 10. Modulo division by one<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v1063\/\">V1063<\/a> The modulo by 1 operation is meaningless. The result will always be zero. llvm-stress.cpp 631<\/p>\n<pre><code class=\"cpp\">void Act() override {   ....   \/\/ If the value type is a vector, and we allow vector select,   \/\/ then in 50% of the cases generate a vector select.   if (isa&lt;FixedVectorType>(Val0->getType()) &amp;&amp; (getRandom() % 1)) {     unsigned NumElem =         cast&lt;FixedVectorType>(Val0->getType())->getNumElements();     CondTy = FixedVectorType::get(CondTy, NumElem);   }   .... }<\/code><\/pre>\n<p>  The developer intended to get a random value between 0 and 1 from a modulo operation. However, the operation of type <i>X%1<\/i> always returns 0. In this case, it would be correct to rewrite the condition as follows: <\/p>\n<pre><code class=\"cpp\">if (isa&lt;FixedVectorType>(Val0->getType()) &amp;&amp; (getRandom() % 2))<\/code><\/pre>\n<p>  More information on this bug is available in the following article: &#171;<a href=\"https:\/\/www.viva64.com\/en\/b\/0771\/\">Checking Clang 11 with PVS-Studio<\/a>&#171;.<\/p>\n<h2>No 9. Four checks<\/h2>\n<p>  After processing the code snippet below, PVS-Studio generated four warning messages: <\/p>\n<ul>\n<li><a href=\"https:\/\/www.viva64.com\/en\/w\/v560\/\">V560<\/a> A part of conditional expression is always true: x >= 0. editor.cpp 1137<\/li>\n<li><a href=\"https:\/\/www.viva64.com\/en\/w\/v560\/\">V560<\/a> A part of conditional expression is always true: y >= 0. editor.cpp 1137<\/li>\n<li><a href=\"https:\/\/www.viva64.com\/en\/w\/v560\/\">V560<\/a> A part of conditional expression is always true: x &lt; 40. editor.cpp 1137<\/li>\n<li><a href=\"https:\/\/www.viva64.com\/en\/w\/v560\/\">V560<\/a> A part of conditional expression is always true: y &lt; 30. editor.cpp 1137<\/li>\n<\/ul>\n<p>  <\/p>\n<pre><code class=\"cpp\">int editorclass::at( int x, int y ) {   if(x&lt;0) return at(0,y);   if(y&lt;0) return at(x,0);   if(x>=40) return at(39,y);   if(y>=30) return at(x,29);    if(x>=0 &amp;&amp; y>=0 &amp;&amp; x&lt;40 &amp;&amp; y&lt;30)   {       return contents[x+(levx*40)+vmult[y+(levy*30)]];   }   return 0; }<\/code><\/pre>\n<p>  The last <i>if<\/i> statement triggered all four warnings. The problem is that the statement performs four checks that always returns <i>true<\/i>. I would call this bug amusing rather than major. These checks are redundant, and you can remove them.<\/p>\n<p>  This error got here from the following article: <a href=\"https:\/\/www.viva64.com\/en\/b\/0707\/\">VVVVVV??? VVVVVV!!!<\/a><\/p>\n<h2>No 8. delete instead of delete[]<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v611\/\">V611<\/a> The memory was allocated using &#8216;new T[]&#8217; operator but was released using the &#8216;delete&#8217; operator. Consider inspecting this code. It&#8217;s probably better to use &#8216;delete [] poke_data;&#8217;. CCDDE.CPP 410<\/p>\n<pre><code class=\"cpp\">BOOL Send_Data_To_DDE_Server (char *data, int length, int packet_type) {   ....   char *poke_data = new char [length + 2*sizeof(int)]; \/\/ &lt;=   ....   if(DDE_Class->Poke_Server( .... ) == FALSE) {     CCDebugString(\"C&amp;C95 - POKE failed!\\n\");     DDE_Class->Close_Poke_Connection();     delete poke_data;                                  \/\/ &lt;=     return (FALSE);   }    DDE_Class->Close_Poke_Connection();    delete poke_data;                                    \/\/ &lt;=    return (TRUE); }<\/code><\/pre>\n<p>  The analyzer detected that memory is freed in a way that is incompatible with how memory was allocated. To free the memory allocated for the array, use the <i>delete[]<\/i> operator instead of <i>delete<\/i>.<\/p>\n<p>  For more information on this bug, check out the following article: &#171;<a href=\"https:\/\/www.viva64.com\/en\/b\/0748\/\">The Code of the Command &amp; Conquer Game: Bugs from the 90&#8217;s. Volume two<\/a>&#171;<\/p>\n<h2>No. 7. Buffer overflow<\/h2>\n<p>  Let&#8217;s take a look at the <i>net_hostname_get<\/i> function.<\/p>\n<pre><code class=\"cpp\">#if defined(CONFIG_NET_HOSTNAME_ENABLE) const char *net_hostname_get(void); #else static inline const char *net_hostname_get(void) {   return \"zephyr\"; } #endif<\/code><\/pre>\n<p>  The option from the <i>#else<\/i> branch is selected during preprocessing. The preprocessed file reflects this as follows:<\/p>\n<pre><code class=\"cpp\">static inline const char *net_hostname_get(void) {   return \"zephyr\"; }<\/code><\/pre>\n<p>  The function returns a pointer to a 7-byte array that contains the string and a null terminator.<\/p>\n<p>  Now let&#8217;s take a look at the code that produces the buffer overflow.<\/p>\n<pre><code class=\"cpp\">static int do_net_init(void) {   ....   (void)memcpy(hostname, net_hostname_get(), MAX_HOSTNAME_LEN);   .... }<\/code><\/pre>\n<p>  PVS-Studio warning: <a href=\"https:\/\/www.viva64.com\/en\/w\/v512\/\">V512<\/a> [CWE-119] A call of the &#8216;memcpy&#8217; function will lead to the &#8216;net_hostname_get()&#8217; buffer becoming out of range. log_backend_net.c 114<\/p>\n<p>  After preprocessing <i>MAX_HOSTNAME_LEN<\/i> expands as follows:<\/p>\n<pre><code class=\"cpp\">(void)memcpy(hostname, net_hostname_get(),     sizeof(\"xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx\"));<\/code><\/pre>\n<p>  When the data is copied, string literal overflow occurs. This causes undefined behavior. <\/p>\n<p>  For more information on this bug, see &#171;<a href=\"https:\/\/www.viva64.com\/en\/b\/0721\/\">Checking the Code of Zephyr Operating System<\/a>&#171;.<\/p>\n<h2>No. 6. Something super weird<\/h2>\n<p>  <\/p>\n<pre><code class=\"cpp\">static char *mntpt_prepare(char *mntpt) {   char *cpy_mntpt;    cpy_mntpt = k_malloc(strlen(mntpt) + 1);   if (cpy_mntpt) {     ((u8_t *)mntpt)[strlen(mntpt)] = '\\0';     memcpy(cpy_mntpt, mntpt, strlen(mntpt));   }   return cpy_mntpt; }<\/code><\/pre>\n<p>  PVS-Studio warning: <a href=\"https:\/\/www.viva64.com\/en\/w\/v575\/\">V575<\/a> [CWE-628] The &#8216;memcpy&#8217; function doesn&#8217;t copy the whole string. Use &#8216;strcpy \/ strcpy_s&#8217; function to preserve terminal null. shell.c 427<\/p>\n<p>  Here someone failed to emulate the <i>strdup<\/i> function.<\/p>\n<p>  Let&#8217;s start with the analyzer&#8217;s warning. The analyzer reports that the <i>memcpy<\/i> function copied the string but didn&#8217;t copy the null terminator. <\/p>\n<p>  The following line of code seems to copy the null terminator:<\/p>\n<pre><code class=\"cpp\">((u8_t *)mntpt)[strlen(mntpt)] = '\\0';<\/code><\/pre>\n<p>  However, it does not. There is a typo here, and the null terminator is assigned to itself. Note that the value is recorded to the <i>mntpt<\/i> array instead of <i>cpy_mntpt<\/i>. As a result, the <i>mntpt_prepare<\/i> function returns a string that lacks the null terminator.<\/p>\n<p>  We see that the programmer intended to write the statement below:<\/p>\n<pre><code class=\"cpp\">((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\\0';<\/code><\/pre>\n<p>  However, there is still no reason to make the line so complex. Let&#8217;s simplify the code:<\/p>\n<pre><code class=\"cpp\">static char *mntpt_prepare(char *mntpt) {   char *cpy_mntpt;    cpy_mntpt = k_malloc(strlen(mntpt) + 1);   if (cpy_mntpt) {     strcpy(cpy_mntpt, mntpt);   }   return cpy_mntpt; }<\/code><\/pre>\n<p>  See &#171;<a href=\"https:\/\/www.viva64.com\/en\/b\/0721\/\">Checking the Code of Zephyr Operating System<\/a>&#187; for more details.<\/p>\n<h2>No. 5. Meaningless overflow protection<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v547\/\">V547<\/a> [CWE-570] Expression &#8216;rel_wait &lt; 0&#8217; is always false. Unsigned type value is never &lt; 0. os_thread_windows.c 359<\/p>\n<pre><code class=\"cpp\">static DWORD get_rel_wait(const struct timespec *abstime) {   struct __timeb64 t;   _ftime64_s(&amp;t);   time_t now_ms = t.time * 1000 + t.millitm;   time_t ms = (time_t)(abstime->tv_sec * 1000 +     abstime->tv_nsec \/ 1000000);    DWORD rel_wait = (DWORD)(ms - now_ms);    return rel_wait &lt; 0 ? 0 : rel_wait; }<\/code><\/pre>\n<p>  In the code above, take a look at the <i>rel_wait<\/i> variable. It is of the unsigned <i>DWORD<\/i> type. This means that the <i>rel_wait &lt; 0<\/i> statement always returns TRUE and has no practical value.<\/p>\n<p>  The error itself is ordinary. However, its fix is more intriguing. The developers simplified the code but failed to fix the bug. You can read the entire case in my colleague&#8217;s article: &#171;<a href=\"https:\/\/www.viva64.com\/en\/b\/0776\/\">Why PVS-Studio Doesn&#8217;t Offer Automatic Fixes<\/a>&#171;.<\/p>\n<p>  For more details on this error, see the following article: &#171;<a href=\"https:\/\/www.viva64.com\/en\/b\/0756\/\">Static code analysis of the PMDK library collection by Intel and errors that are not actual errors<\/a>&#171;.<\/p>\n<h2>No. 4. Don&#8217;t expand std, bro<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v1061\/\">V1061<\/a> Extending the &#8216;std&#8217; namespace may result in undefined behavior. sized_iterator.hh 210<\/p>\n<pre><code class=\"cpp\">\/\/ Dirty hack because g++ 4.6 at least wants \/\/ to do a bunch of copy operations. namespace std { inline void iter_swap(util::SizedIterator first,                       util::SizedIterator second) {   util::swap(*first, *second); } } \/\/ namespace std<\/code><\/pre>\n<p>  You can read more on this example and why this is a poor practice in the following article: &#171;<a href=\"https:\/\/www.viva64.com\/en\/b\/0768\/\">Checking the Code of DeepSpeech, or Why You Shouldn&#8217;t Write in namespace std<\/a>&#171;.<\/p>\n<h2>No. 3. The little scrollbar that could not<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v501\/\">V501<\/a> There are identical sub-expressions to the left and to the right of the &#8216;-&#8216; operator: bufferHeight \u2014 bufferHeight TermControl.cpp 592<\/p>\n<pre><code class=\"cpp\">bool TermControl::_InitializeTerminal() {   ....   auto bottom = _terminal->GetViewport().BottomExclusive();   auto bufferHeight = bottom;    ScrollBar().Maximum(bufferHeight - bufferHeight);   ScrollBar().Minimum(0);   ScrollBar().Value(0);   ScrollBar().ViewportSize(bufferHeight);   .... }<\/code><\/pre>\n<p>  This is what&#8217;s called &#171;history-dependent activation&#187;. In this case, the Windows Terminal failed to show its scrollbar because of an error. My colleague researched the bug and figured out what happened. Curious? Here&#8217;s his article: &#171;<a href=\"https:\/\/www.viva64.com\/en\/b\/0718\/\">The Little Scrollbar That Could Not<\/a>&#171;.<\/p>\n<h2>No. 2. Radius and height mixed up<\/h2>\n<p>  And once again we&#8217;ll talk about the analyzer&#8217;s several warnings:<\/p>\n<ul>\n<li><a href=\"https:\/\/www.viva64.com\/en\/w\/v764\/\">V764<\/a> Possible incorrect order of arguments passed to &#8216;CreateWheel&#8217; function: &#8216;height&#8217; and &#8216;radius&#8217;. StandardJoints.cpp 791<\/li>\n<li><a href=\"https:\/\/www.viva64.com\/en\/w\/v764\/\">V764<\/a> Possible incorrect order of arguments passed to &#8216;CreateWheel&#8217; function: &#8216;height&#8217; and &#8216;radius&#8217;. StandardJoints.cpp 833<\/li>\n<li><a href=\"https:\/\/www.viva64.com\/en\/w\/v764\/\">V764<\/a> Possible incorrect order of arguments passed to &#8216;CreateWheel&#8217; function: &#8216;height&#8217; and &#8216;radius&#8217;. StandardJoints.cpp 884<\/li>\n<\/ul>\n<p>  This is how the function is called:<\/p>\n<pre><code class=\"cpp\">NewtonBody* const wheel = CreateWheel (scene, origin, height, radius);<\/code><\/pre>\n<p>  And this is its definition:<\/p>\n<pre><code class=\"cpp\">static NewtonBody* CreateWheel (DemoEntityManager* const scene,   const dVector&amp; location, dFloat radius, dFloat height)<\/code><\/pre>\n<p>  You can see that when the developer called the function, the arguments were mixed up.<\/p>\n<p>  Read more on this error in the following article: &#171;<a href=\"https:\/\/www.viva64.com\/en\/b\/0729\/\">A Second Check of Newton Game Dynamics with PVS-Studio<\/a>&#171;<\/p>\n<h2>No. 1. Overwriting the result<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v519\/\">V519<\/a> The &#8216;color_name&#8217; variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 621, 627. string.cpp 627<\/p>\n<pre><code class=\"cpp\">static bool parseNamedColorString(const std::string &amp;value,                                   video::SColor &amp;color) {   std::string color_name;   std::string alpha_string;    size_t alpha_pos = value.find('#');   if (alpha_pos != std::string::npos) {     color_name = value.substr(0, alpha_pos);     alpha_string = value.substr(alpha_pos + 1);   } else {     color_name = value;   }    color_name = lowercase(value); \/\/ &lt;=    std::map&lt;const std::string, unsigned>::const_iterator it;   it = named_colors.colors.find(color_name);   if (it == named_colors.colors.end())     return false;   .... }<\/code><\/pre>\n<p>  The function above analyzes the color name with its transparency parameter and returns the color&#8217;s hexadecimal code. If the string contains the transparency parameter, this parameter is split from the string and the color is recorded to the <i>color_name<\/i> variable. Otherwise, the <i>color_name<\/i> variable is assigned the original color string.<\/p>\n<p>  The problem arises when the lowercase() function is called. The programmer passed the wrong parameter into this function. If the <i>color_name<\/i> variable contains a substring of <i>value<\/i>, then this substring will always be rewritten. Thus, we won&#8217;t get what we expected from parseNamedColorString() function.<\/p>\n<p>  This is how we can fix this line:<\/p>\n<pre><code class=\"cpp\">color_name = lowercase(color_name);<\/code><\/pre>\n<p>  For more details on this error see: &#171;<a href=\"https:\/\/www.viva64.com\/en\/b\/0751\/\">PVS-Studio: analyzing pull requests in Azure DevOps using self-hosted agents<\/a>&#171;.<\/p>\n<h2>Conclusion<\/h2>\n<p>  Over the past year, we found many errors in open source projects. These were ordinary copy-paste bugs, incorrect constants, memory leaks, and many other problems. This year&#8217;s Top 10 bugs include several ones detected by our new algorithms and prove that our analyzer keeps evolving. <\/p>\n<p>  I hope you enjoyed reading my selection of memorable bugs as much as I enjoyed assembling this list. Of course, if you read our <a href=\"https:\/\/www.viva64.com\/en\/b\/\">blog<\/a> or looked through warning lists PVS-Studio produced after scanning open source projects, you may have your own Top-10.<\/p>\n<p>  Here are the Top 10 bugs we found in C++ projects over the previous years: <a href=\"https:\/\/www.viva64.com\/en\/b\/0483\/\">2016<\/a>, <a href=\"https:\/\/www.viva64.com\/en\/b\/0565\/\">2017<\/a>, <a href=\"https:\/\/www.viva64.com\/en\/b\/0619\/\">2018<\/a>, <a href=\"https:\/\/www.viva64.com\/en\/b\/0700\/\">2019<\/a>.<\/div>\n<\/div>\n<\/div>\n<p><!----><!----><\/div>\n<p><!----><!----><br \/> \u0441\u0441\u044b\u043b\u043a\u0430 \u043d\u0430 \u043e\u0440\u0438\u0433\u0438\u043d\u0430\u043b \u0441\u0442\u0430\u0442\u044c\u0438 <a href=\"https:\/\/habr.com\/ru\/articles\/533694\/\"> https:\/\/habr.com\/ru\/articles\/533694\/<\/a><\/p>\n","protected":false},"excerpt":{"rendered":"<div><!--[--><!--]--><\/div>\n<div id=\"post-content-body\">\n<div>\n<div class=\"article-formatted-body article-formatted-body article-formatted-body_version-1\">\n<div xmlns=\"http:\/\/www.w3.org\/1999\/xhtml\">\n<div style=\"text-align:center;\"><img decoding=\"async\" src=\"https:\/\/habrastorage.org\/r\/w1560\/webt\/ed\/qz\/kt\/edqzktmzcrowmhowiz0-csjx1hy.png\" alt=\"image1.png\" data-src=\"https:\/\/habrastorage.org\/webt\/ed\/qz\/kt\/edqzktmzcrowmhowiz0-csjx1hy.png\"\/><\/div>\n<p>  It&#8217;s winter outside, the year is coming to an end, which means it&#8217;s time to review the most notable errors the PVS-Studio analyzer detected in 2020.  <\/p>\n","protected":false},"author":1,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[],"tags":[],"class_list":["post-388239","post","type-post","status-publish","format-standard","hentry"],"_links":{"self":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/388239","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/users\/1"}],"replies":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcomments&post=388239"}],"version-history":[{"count":0,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/388239\/revisions"}],"wp:attachment":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fmedia&parent=388239"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcategories&post=388239"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Ftags&post=388239"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}