How to reduce the likelihood of errors at the stage of writing code. Note N1

Published on March 09, 2011

How to reduce the likelihood of errors at the stage of writing code. Note N1

    Check Miranda IM
    I got to the code of the well-known Miranda IM instant messenger client . Together with various plugins, this is a fairly large project, the size of which is about 950 thousand lines of code in C and C ++. And, as in any solid project with a history of development, it has a considerable number of errors and typos.

    Examining defects in various applications, I noticed some patterns. And now, with the example of defects found in Miranda IM, I will try to formulate some recommendations that will help to avoid many errors and typos even at the stage of writing code.



    For the analysis of Miranda IM, I used (yes, you guessed it) the PVS-Studio 4.14 analyzer . The code of the Miranda IM project is very high quality, which is confirmed by the popularity of this program. I myself am a user of this client and have no complaints about it in terms of quality. The project is assembled in Visual Studio with Warning Level 3 (/ W3), and the number of comments is 20% of the entire program text.

    1. Avoid the functions memset, memcpy, ZeroMemory and the like


    We will start with the errors that occur when using low-level memory functions such as memset, memcpy, ZeroMemory and the like.

    I recommend that you avoid these functions in every way. It is clear that you should not literally follow my advice and replace all these functions with loops. But I have seen such a huge number of errors related to the use of these functions that I highly recommend being careful with them and using them only when necessary. In my opinion, there are only two cases where the use of these functions is justified:

    1) Processing of large arrays. That is, where an optimized function algorithm will give a gain compared to processing elements in a simple cycle.

    2) Processing a large number of small arrays. It also makes sense in terms of improving performance.

    In all other cases, it is better to try to do without them. For example, I find these features unnecessary in a program like Miranda. There are no resource-intensive algorithms or huge arrays in it. Therefore, the use of memset / memcpy functions stems only from the convenience of writing short code. However, this simplicity is very deceiving and, saving a few seconds on writing code, you can catch a fuzzy manifest memory corruption error for weeks. Let's look at some code examples taken from the Miranda IM project.

    V512 A call of the 'memcpy' function will lead to a buffer overflow or underflow. tabsrmm utils.cpp 1080
    typedef struct _textrangew
    {
      CHARRANGE chrg;
      LPWSTR lpstrText;
    } TEXTRANGEW;
    const wchar_t * Utils :: extractURLFromRichEdit (...)
    {
      ...
      :: CopyMemory (tr.lpstrText, L "mailto:", 7);
      ...
    }

    Copy only a piece of the string. A mistake is banal to disgrace, but this does not cease to be a mistake. Most likely, a string consisting of 'char' was used here before. Then they switched to Unicode strings, and forgot to change the constant.

    If you initially use functions that are intended for this to copy strings, then such an error simply cannot occur. Imagine what was written like this:
    strncpy (tr.lpstrText, "mailto:", 7);

    Then, when switching to Unicode strings, the number 7 does not need to be changed:
    wcsncpy (tr.lpstrText, L "mailto:", 7);

    I am not saying that this is perfect code. But it is already much better than using CopyMemory. Consider another example.

    V568 It's odd that the argument of sizeof () operator is the '& ImgIndex' expression. clist_modern modern_extraimage.cpp 302
    void ExtraImage_SetAllExtraIcons (HWND hwndList, HANDLE hContact)
    {
      ...
      char * (ImgIndex [64]);
      ...
      memset (& ImgIndex, 0, sizeof (& ImgIndex));
      ...
    }

    Here I wanted to nullify an array of 64 pointers. But instead, we only reset the first element. The same error, by the way, is present again in another file. Say thanks. Copy-Paste:

    V568 It's odd that the argument of sizeof () operator is the '& ImgIndex' expression. clist_mw extraimage.c 295 The

    correct version of the code should look like this:
    memset (& ImgIndex, 0, sizeof (ImgIndex));

    By the way, taking an address from an array can only confuse someone who reads the code. Here, taking an address has no effect. And the code can be written like this:
    memset (ImgIndex, 0, sizeof (ImgIndex));

    The following example.

    V568 It's odd that the argument of sizeof () operator is the '& rowOptTA' expression. clist_modern modern_rowtemplateopt.cpp 258
    static ROWCELL * rowOptTA [100];
    void rowOptAddContainer (HWND htree, HTREEITEM hti)
    {
      ...
      ZeroMemory (rowOptTA, sizeof (& rowOptTA));
      ...
    }

    Again, instead of calculating the size of the array, we calculate the size of the pointer. The correct expression is "sizeof (rowOptTA)". To clean the array, I propose the following version of such code:
    const size_t ArraySize = 100;
    static ROWCELL * rowOptTA [ArraySize];
    ...
    std :: fill (rowOptTA, rowOptTA + ArraySize, nullptr);

    I’m already used to the fact that such lines like to reproduce in the program by copying:

    V568 It's odd that the argument of sizeof () operator is the '& rowOptTA' expression. clist_modern modern_rowtemplateopt.cpp 308

    V568 It's odd that the argument of sizeof () operator is the '& rowOptTA' expression. clist_modern modern_rowtemplateopt.cpp 438

    Do you think that this is all about low-level work with arrays? No, not all. Look further, be afraid and shake hands of fans to write memset.

    V512 A call of the 'memset' function will lead to a buffer overflow or underflow. clist_modern modern_image_array.cpp 59
    static BOOL ImageArray_Alloc (LP_IMAGE_ARRAY_DATA iad, int size)
    {
      ...
      memset (& iad-> nodes [iad-> nodes_allocated_size], 
        (size_grow - iad-> nodes_allocated_size) *
           sizeof (IMAGE_ARRAY_DATA_NODE),
        0);
      ...
    }

    This time the size of the copied data is calculated correctly. The second and third arguments are just confused. As a result, we fill in 0 elements. The correct option:
    memset (& iad-> nodes [iad-> nodes_allocated_size], 0,
      (size_grow - iad-> nodes_allocated_size) *
         sizeof (IMAGE_ARRAY_DATA_NODE));

    How to rewrite this piece of code more elegantly, I do not know. Rather, it cannot be rewritten beautifully without affecting other parts of the program and data structures.

    The question is, how can I do without memset when working with structures such as OPENFILENAME:
    OPENFILENAME x;
    memset (& x, 0, sizeof (x));

    Very simple. You can create a nullified structure like this:
    OPENFILENAME x = {0};


    2. Closely monitor whether you work with a signed or unsigned type


    The problem of confusion between signed and unsigned types may seem far-fetched at first glance. But this question is underestimated by programmers in vain.

    In most cases, people do not like to consider compiler warnings related to the fact that a variable of type int is compared with a variable of type unsigned. And indeed, most often this code is completely correct. Programmers turn off such warnings or do not pay attention to them. Or there is a third option, they enter an explicit type conversion to drown out the compiler warning, without going into details.

    I suggest that you don’t do this again and analyze the situation every time when a signed type is encountered with an unsigned one. And, in general, be attentive to what type the expression has or what the function returns. Now a few examples on this topic.

    V547 Expression 'wParam> = 0' is always true. Unsigned type value is always> = 0. clist_mw cluiframes.c 3140

    The program code has an id2pos function that returns the value '-1' in case of an error. There is nothing wrong with this feature. Elsewhere, the result of the id2pos function is used as follows:
    typedef UINT_PTR WPARAM; 
    static int id2pos (int id);
    static int nFramescount = 0;
    INT_PTR CLUIFrameSetFloat (WPARAM wParam, LPARAM lParam)
    {
      ...
      wParam = id2pos (wParam);
      if (wParam> = 0 && (int) wParam <nFramescount)
        if (Frames [wParam] .floating)
      ...
    }

    The problem is that the wParam variable has an unsigned type. Therefore, the condition 'wParam> = 0' is always true. If the id2pos function returns '-1', then the condition for checking invalid values ​​will not work, and we will start using a negative index.

    I am pretty sure that the following code was written at first:

    if (wParam> = 0 && wParam <nFramescount)

    The Visual C ++ compiler issued the warning “warning C4018: '<': signed / unsigned mismatch”. This warning is just included on Warning Level 3, with which Miranda IM is going. And at that moment insufficient attention was paid to this place. The warning was suppressed by explicit type conversion. But the error did not disappear, but only hid. The correct code should have been the following code:

    if ((int) wParam>

    So attention and once again attention to such places. In Miranda IM, I counted 33 conditions that, due to confusion with signed / unsigned, are always true or always false.

    Let's continue. I especially like the following example. And the comment is just wonderful.

    V547 Expression 'nOldLength <0' is always false. Unsigned type value is never <0. IRC mstring.h 229
    void Append (PCXSTR pszSrc, int nLength)
    {
      ...
      UINT nOldLength = GetLength ();
      if (nOldLength <0)
      {
        // protects from underflow
        nOldLength = 0;
      }
      ...
    }

    I think no explanation for this code is required.

    Of course, only programmers are not always to blame for errors. Sometimes the library developers (in this case, WinAPI) also add a pig.
    #define SRMSGSET_LIMITNAMESLEN_MIN 0
    static INT_PTR CALLBACK DlgProcTabsOptions (...)
    {
      ...
      limitLength =
        GetDlgItemInt (hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE)> =
        SRMSGSET_LIMITNAMESLEN_MIN?
        GetDlgItemInt (hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE):
        SRMSGSET_LIMITNAMESLEN_MIN;
      ...
    }

    If you do not pay attention to an overly complex expression, then the code looks correct. By the way, it was generally one line. For convenience, I broke it into several. However, we will not touch on the formatting of the code now.

    The problem is that the GetDlgItemInt () function does not return 'int' at all, as the programmer expected. This function returns a UINT. Here is her prototype from the WinUser.h file:
    WINUSERAPI
    UINT
    WINAPI
    GetDlgItemInt (
        __in HWND hDlg,
        __in int nIDDlgItem,
        __out_opt BOOL * lpTranslated,
        __in BOOL bSigned);

    As a result, PVS-Studio displays the message:

    V547 Expression is always true. Unsigned type value is always> = 0. scriver msgoptions.c 458

    And it really is. The expression "GetDlgItemInt (hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE)> = SRMSGSET_LIMITNAMESLEN_MIN" is always true.

    Specifically, in this case, perhaps, there will be no mistake. But the meaning of my warning, I hope, is clear. Watch carefully what functions return to you.

    3. Avoid too many calculations on one line.


    Every programmer knows and responsibly declares that it is necessary to write simple and understandable code. But in practice, sometimes it seems that he is participating in a secret competition, who will write a line more cunningly, using an interesting language construction or demonstrate the ability to juggle with pointers.

    Most often, errors occur where a programmer, in order to create compact code, collects several actions in one line at once. With a slight gain in elegance, it significantly increases the risk of a typo or that does not take into account side effects. Consider an example:

    V567 Undefined behavior. The 's' variable is modified while being used twice between sequence points. msn ezxml.c 371
    short ezxml_internal_dtd (ezxml_root_t root, char * s, size_t len)
    {
      ...
      while (* (n = ++ s + strspn (s, EZXML_WS)) && * n! = '>') {
      ...
    }

    This is where undefined behavior arises. This code may function correctly over the long life of the program. But there is no guarantee that it will also behave after changing the compiler version or optimization keys. The compiler has the right to first calculate '++ s', and then call the function 'strspn (s, EZXML_WS)'. Or, on the contrary, at first it can call a function, and only then increase the value of the variable 's'.

    I will give another example, why you should not try to collect everything in one line. In Miranda IM, some branches of the program execution are disabled / enabled using inserts of the form '&& 0'. Examples:
    if ((1 || altDraw) && ...
    if (g_CluiData.bCurrentAlpha == GoalAlpha && 0)
    if (checkboxWidth && (subindex == - 1 || 1)) {

    With the above comparisons, everything is clear and they are clearly visible. Now imagine that you meet the fragment shown below. I formatted the code. This is ONE line in the program.

    V560 A part of conditional expression is always false: 0. clist_modern modern_clui.cpp 2979
    LRESULT CLUI :: OnDrawItem (UINT msg, WPARAM wParam, LPARAM lParam)
    {
      ...
      DrawState (dis-> hDC, NULL, NULL, (LPARAM) hIcon, 0,
        dis-> rcItem.right + dis-> rcItem.left-
        GetSystemMetrics (SM_CXSMICON)) / 2 + dx,
        (dis-> rcItem.bottom + dis-> rcItem.top-
        GetSystemMetrics (SM_CYSMICON)) / 2 + dx,
        0,0
        DST_ICON |
        (dis-> itemState & ODS_INACTIVE && FALSE? DSS_DISABLED: DSS_NORMAL));
       ...
    }

    If there is no mistake, it is still not easy to remember and find the word FALSE in this line. Did you find him? Agree - not an easy task. And if this is a mistake? Then there is no chance at all to find it just by looking at the code. Such expressions are very useful to make separately. Example:
    UINT uFlags = DST_ICON;
    uFlags | = dis-> itemState & ODS_INACTIVE && FALSE?
                DSS_DISABLED: DSS_NORMAL;

    And I myself, perhaps, would write it in a long, but more understandable way:
    UINT uFlags;
    if (dis-> itemState & ODS_INACTIVE && (((FALSE))))
      uFlags = DST_ICON | DSS_DISABLED;
    else 
      uFlags = DST_ICON | DSS_NORMAL;

    Yes, it is longer, but it is easy to read, and the word FALSE is better noticeable.

    4. Align everything possible in the code


    Code alignment reduces the chance of typos or errors in Copy-Paste. If an error is still made, then finding it during the code review process will be several times easier. Consider a code example.

    V537 Consider reviewing the correctness of 'maxX' item's usage. clist_modern modern_skinengine.cpp 2898
    static BOOL ske_DrawTextEffect (...)
    {
      ...
      minX = max (0, minX + mcLeftStart-2);
      minY = max (0, minY + mcTopStart-2);
      maxX = min ((int) width, maxX + mcRightEnd-1);
      maxY = min ((int) height, maxX + mcBottomEnd-1);
      ...
    }

    A monolithic piece of code that is completely uninteresting to read. Format it:
    minX = max (0, minX + mcLeftStart - 2);
    minY = max (0, minY + mcTopStart - 2);
    maxX = min ((int) width, maxX + mcRightEnd - 1);
    maxY = min ((int) height, maxX + mcBottomEnd - 1);

    This is not a good example, but you must admit, now it has become much easier to notice that the variable maxX is used twice.

    My alignment recommendation should not be taken literally and everywhere to build code columns. Firstly, it takes extra time when writing and editing code. Secondly, it can lead to other errors. In the following example, just the desire to make a beautiful column led to an error in the Miranda IM code.

    V536 Be advised that the utilized constant value is represented by an octal form. Oct: 037, Dec: 31. msn msn_mime.cpp 192
    static const struct _tag_cpltbl
    {
      unsigned cp;
      const char * mimecp;
    } cptbl [] =
    {
      {037, "IBM037"}, // IBM EBCDIC US-Canada 
      {437, "IBM437"}, // OEM United States 
      {500, "IBM500"}, // IBM EBCDIC International 
      {708, "ASMO-708"}, // Arabic (ASMO 708) 
      ...
    }

    Wanting to make a nice column of numbers, it's easy to get carried away and enter '0' at the beginning, making the octal constant.

    Clarify the recommendation. Align everything possible in the code. But don't align the numbers by adding zeros.

    5. Do not propagate a string more than once


    Copying strings during programming is inevitable. But you can insure yourself by not pasting a line from the clipboard several times at once. In most cases, it is better to copy the line, then edit. Copy and edit again. And so on. So it’s harder to forget to change something in a line or change it incorrectly. Consider a code example:

    V525 The code containing the collection of similar blocks. Check items '1316', '1319', '1318', '1323', '1323', '1317', '1321' in lines 954, 955, 956, 957, 958, 959, 960. clist_modern modern_clcopts.cpp 954
    static INT_PTR CALLBACK DlgProcTrayOpts (...)
    {
      ...
      EnableWindow (GetDlgItem (hwndDlg, IDC_PRIMARYSTATUS), TRUE);
      EnableWindow (GetDlgItem (hwndDlg, IDC_CYCLETIMESPIN), FALSE);
      EnableWindow (GetDlgItem (hwndDlg, IDC_CYCLETIME), FALSE);    
      EnableWindow (GetDlgItem (hwndDlg, IDC_ALWAYSPRIMARY), FALSE);
      EnableWindow (GetDlgItem (hwndDlg, IDC_ALWAYSPRIMARY), FALSE);
      EnableWindow (GetDlgItem (hwndDlg, IDC_CYCLE), FALSE);
      EnableWindow (GetDlgItem (hwndDlg, IDC_MULTITRAY), FALSE);
      ...
    }

    Most likely, there is no real mistake here. We just work twice with the IDC_ALWAYSPRIMARY element. Nevertheless, it is very easy to make mistakes in such blocks from the copied lines.

    6. Set a high level of warnings at the compiler and use static analyzers


    For many errors, it is impossible to give recommendations on how to reduce the likelihood of their occurrence in the code. Most often, they are typos, which allows both a novice and the most highly skilled programmer.

    Nevertheless, many of these errors can be detected at the stage of writing code. First of all, these are compiler warnings. And the second - reports from the nightly launch of static code analyzers.

    Now someone will say that this is a poorly hidden advertisement. But in fact, this is just another recommendation that reduces the number of errors. If I found errors using static analysis, and I can’t say how to avoid their appearance in the code, then the recommendation is to use code analyzers.

    Let's

    look at some examples of errors that can be quickly detected by source code analyzers: V560 A part of conditional expression is always true: 0x01000. tabsrmm tools.cpp 1023
    #define GC_UNICODE 0x01000
    DWORD dwFlags;
    UINT CreateGCMenu (...)
    {
      ...
      if (iIndex == 1 && si-> iType! = GCW_SERVER &&
          ! (si-> dwFlags && GC_UNICODE)) {
      ...
    }

    Typo allowed. Instead of the operator &, the operator & && is used. How to be safe here when writing code, I do not know. The correct version of the condition:
     (si-> dwFlags & GC_UNICODE)

    The following example.

    V528 It is odd that pointer to 'char' type is compared with the '\ 0' value. Probably meant: * str! = '\ 0'. clist_modern modern_skinbutton.cpp 282

    V528 It is odd that pointer to 'char' type is compared with the '\ 0' value. Probably meant: * endstr! = '\ 0'. clist_modern modern_skinbutton.cpp 283
    static char * _skipblank (char * str)
    {
      char * endstr = str + strlen (str);
      while ((* str == '' || * str == '\ t') && str! = '\ 0') str ++;
      while ((* endstr == '' || * endstr == '\ t') &&
             endstr! = '\ 0' && endstr <str)
        endstr--;
      ...
    }

    In this code, just two asterisks '*' are just forgotten to dereference pointers. The result can be fatal. This code is prone to access violation. The correct version of the code:
    while ((* str == '' || * str == '\ t') && * str! = '\ 0') str ++;
    while ((* endstr == '' || * endstr == '\ t') &&
           * endstr! = '\ 0' && endstr <str)
      endstr--;

    Here again, it’s hard to give advice other than using special tools to check the code.

    The following example.

    V514 Dividing sizeof a pointer 'sizeof (text)' by another value. There is a probability of logical error presence. clist_modern modern_cachefuncs.cpp 567
    #define SIZEOF (X) (sizeof (X) / sizeof (X [0]))
    int Cache_GetLineText (..., LPTSTR text, int text_size, ...)
    {
      ...
      tmi.printDateTime (pdnce-> hTimeZone, _T ("t"), text, SIZEOF (text), 0);
      ...
    }

    At first glance, everything is fine. The text and its length calculated using the SIZEOF macro are passed to the function. In fact, the macro should be called COUNT_OF, but that’s not the point. The trouble is that we are trying to count the number of characters in the index. Here it computes “sizeof (LPTSTR) / sizeof (TCHAR)”. A person notices such suspicious places poorly, but the compiler and static analyzer are good. Corrected version of the code:
    tmi.printDateTime (pdnce-> hTimeZone, _T ("t"), text, text_size, 0);

    The following example

    V560 A part of conditional expression is always true: 0x29. icqoscar8 fam_03buddy.cpp 632
    void CIcqProto :: handleUserOffline (BYTE * buf, WORD wLen)
    {
      ...
      else if (wTLVType = 0x29 && wTLVLen == sizeof (DWORD))
      ...
    }

    The recommendation here is to write the constant in the condition in the first place. This kind of code just does not compile:
    if (0x29 = wTLVType && sizeof (DWORD) == wTLVLen)

    But many programmers, including me, do not like this style. For example, it prevents me from reading the code, since at first I want to find out which variable is compared, and only then with what exactly.

    If the programmer refuses this style of comparison, then he can either rely on the compiler / analyzer or take risks.

    By the way, despite the fact that everyone knows about this error, it is not the rarest. Three more examples from Miranda IM, where the PVS-Studio analyzer issued a warning V559 :
    else if (ft-> ft_magic = FT_MAGIC_OSCAR)
    if (ret = 0) {return (0);}
    if (Drawing-> type = CLCIT_CONTACT)

    Analysis of the code also allows you to identify if not errors, then very suspicious places in the code. For example, in Miranda IM pointers are used not only as pointers. If in some places in the code such games look normal, then in others they scare. Example code that really

    worries me: V542 Consider inspecting an odd type cast: 'char *' to 'char'. clist_modern modern_toolbar.cpp 586
    static void
    sttRegisterToolBarButton (..., char * pszButtonName, ...)
    {
      ...
      if ((BYTE) pszButtonName)
        tbb.tbbFlags = TBBF_FLEXSIZESEPARATOR;
      else
        tbb.tbbFlags = TBBF_ISSEPARATOR;
      ...
    }

    In fact, we check that the address of the line is not a multiple of 256. I don’t understand what we really wanted to write in the condition. Perhaps this is even correct, but highly doubtful.

    By analyzing the code, you can find many incorrect conditions. Example:

    V501 There are identical sub-expressions 'user-> statusMessage' to the left and to the right of the '&&' operator. jabber jabber_chat.cpp 214
    void CJabberProto :: GcLogShowInformation (...)
    {
      ...
      if (user-> statusMessage && user-> statusMessage)
      ...
    }

    And so on and so forth. I can give other examples for a long time, but this does not make sense. The important thing is that a large number of errors can be detected using static analysis at the earliest stages.

    When the static analyzer finds few errors in your program, its use does not seem interesting. But this is the wrong way in reasoning. After all, many errors that the analyzer could find in the early stages, you corrected later with blood, debugging for hours.

    Static analysis is of more interest in the program development process, and not as a one-time check. Many errors and typos are in the process of testing and creating unit tests. But if some of these errors can be found even at the stage of writing the code, then this will be a tremendous gain in time and effort. It's a shame to debug the program for two hours, then to notice an extra semicolon ';' after the 'for' statement. Such an error can often be neutralized by spending 10 minutes on a static analysis of files changed during the operation.

    Conclusion


    In this article, I shared only a few thoughts on how to make fewer mistakes when programming in C ++. Other thoughts are ripening, which I will try to write about in the following articles and notes.

    PS


    It has already become a tradition that after a similar article, someone asks if you informed the program / library developers about the errors found. I will answer in advance the question of whether we sent a bug report on the Miranda IM project.

    No, not sent. This is too demanding a task. The article shows only a small part of what was found. There are about a hundred fragments in the project where it is worth fixing the code, and more than a hundred where I just don’t know if this is a mistake or not. However, the translation of this article will be sent to the authors of Miranda IM and they will be offered a free version of the PVS-Studio analyzer. If they show interest, they can verify the source code themselves and fix whatever they see fit.

    It’s also worth explaining why I often don’t presume to judge whether this or that piece of code is error or not. Ambiguous code example:

    V523The 'then' statement is equivalent to the 'else' statement. scriver msglog.c 695
    if (streamData-> isFirst) {
      if (event-> dwFlags & IEEDF_RTL) {
        AppendToBuffer (& buffer, & bufferEnd, & bufferAlloced, "\\ rtlpar");
      } else {
        AppendToBuffer (& buffer, & bufferEnd, & bufferAlloced, "\\ ltrpar");
      }
    } else {
      if (event-> dwFlags & IEEDF_RTL) {
        AppendToBuffer (& buffer, & bufferEnd, & bufferAlloced, "\\ rtlpar");
      } else {
        AppendToBuffer (& buffer, & bufferEnd, & bufferAlloced, "\\ ltrpar");
      }
    }

    Here are two identical pieces of code. Perhaps this is a mistake. And perhaps now in any branch you need the same set of actions. And the code is written specifically so that later it was easy to modify. Here you need to know the program in order to figure out whether this place is wrong or not.