Alexander Chibisov

May 30 2016

Tags:

#Cpp

Typos in Miranda IM

May 30 2016

Author: Alexander Chibisov

Introduction
Incorrect memory freeing
Incorrect operation precedence
"Lost expression"
Extra assignment
Assignment in the condition
Repetition in the condition
Do you need curly brackets?
Unnecessary checks
Conditional blocks, performing the same code
Conclusion

This article is about common errors which happen due to typos, using Miranda IM as a case in point. Many of these errors can lead to incorrect program behavior; some of them don't do much harm, but lead to worsened readability of code.

Introduction

Miranda IM is a well-known instant messaging program. The source code of the program was taken from the Sourceforge repository, where you can find all available versions of the program source code. To do the analysis we used Miranda IM 0.10.50 and PVS-Studio 6.03. The project has already been checked previously, and the results can be found in the post "How to make fewer errors at the stage of code writing". In Miranda IM code the analyzer detected quite a number of issues. Also, there were analyzer warnings that are hard to define as "errors", perhaps the code looked too tricky for the analyzer. Such "errors" aren't really suitable for an article, therefore we have chosen the most interesting bugs.

Incorrect memory freeing

void TSAPI LoadFavoritesAndRecent()
{
  RCENTRY    *recentEntries, rceTemp;
  ....
  recentEntries = new RCENTRY[nen_options.wMaxRecent + 1];
  ....
  if (iIndex == 0) {
    free(recentEntries); // <=
    return;
  }
  ....
  delete[] recentEntries;
}

V611 The memory was allocated using 'new' operator but was released using the 'free' function. Consider inspecting operation logics behind the 'recentEntries' variable. trayicon.cpp 355

The analyzer warns about incorrect memory handling during the destruction of the object. In the case of there being no entries in the list, the function will prematurely exit, and the memory allocated for the recentEntries will be cleaned incorrectly. At the same time, if the function is executed until the end, then the object will be destroyed correctly, and this error can be called a typo. To destroy the object correctly and clean the memory it's necessary to use a delete[] command when creating an array with the help of a new[] operator. It's incorrect to use the free function and the new operator. During memory cleaning, the free function doesn't call the destructors of the objects, which may cause undefined behavior. Actually, this memory freeing itself is undefined behavior. To fix this issue, we should unify the code style, and replace the free function with delete[] operator.

Incorrect operation precedence

The operation precedence is very important. Quite often, because of non-compliance with operation precedence, the construction performs in an unexpected way, or we may have evaluation errors.

LONG_PTR CALLBACK HotkeyHandlerDlgProc(....)
{
  ....
  EnableMenuItem(
    submenu, 
    ID_TRAYCONTEXT_HIDEALLMESSAGECONTAINERS,
    MF_BYCOMMAND | 
    (nen_options.bTraySupport) ? MF_ENABLED : MF_GRAYED);
  ....
}

V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator. hotkeyhandler.cpp 310

This code fragment shows that a misplaced closing parenthesis caused a ternary operator to function incorrectly. As the operator of bitwise OR has a higher precedence than the ternary operator, the program first evaluates MF_BYCOMMAND | (nen_options.bTraySupport), and only after that the recieved value is compared inside the ternary construction. The code should be edited in the following way:

EnableMenuItem(submenu, ID_TRAYCONTEXT_HIDEALLMESSAGECONTAINERS, 
MF_BYCOMMAND | (nen_options.bTraySupport ? MF_ENABLED : MF_GRAYED));

A funny part is that this is a real error that does not affect the correctness of the program. The thing is that MF_BYCOMMAND is nothing but 0x00000000L. More details on this topic can be found in a small e-book, written by Andrey Karpov "The Ultimate Question of Programming, Refactoring, and Everything" (see chapter N39): Why incorrect code works.

One more example related to incorrect operation precedence:

static struct gg_dcc7 *gg_dcc7_session_find(....)
{
  ....
  if (tmp->peer_uin == uin && 
      !tmp->state == GG_STATE_WAITING_FOR_ACCEPT)
        return tmp;
  ....
}

V562 It's odd to compare 0 or 1 with a value of 39. dcc7.c 151

When checking the second expression, instead of using the logical negation operator for the tmp->state == GG_STATE_WAITING_FOR_ACCEPT, the check is used for tmp->state variable, and only then it is compared with the GG_STATE_WAITING_FOR_ACCEPT constant. To fix this bug we should enclose the second expression in parentheses, and the condition will be changed in the following way:

if (tmp->peer_uin == uin &&  
    !(tmp->state == GG_STATE_WAITING_FOR_ACCEPT))
      return tmp;

Although it would be easier to use the != operator, and simplify the code:

if (tmp->peer_uin == uin && 
    tmp->state != GG_STATE_WAITING_FOR_ACCEPT)
      return tmp;

"Lost expression"

int DeleteMaskByItID(....)
{
  ....
  if (mmTemplateList->dwMaskCnt==1)
  {
    ....
    mmTemplateList->pl_Masks=NULL;   
    mmTemplateList->dwMaskCnt;    // <=
  }
  else
  {
    ....
    mmTemplateList->pl_Masks=newAlocation;
    mmTemplateList->dwMaskCnt--;
  }
  ....
}

V607 Ownerless expression 'mmTemplateList->dwMaskCnt'. modern_skinselector.cpp 246

In the code fragment, we can see that the function was created to remove the mask by ID. If the number of masks is greater than one, then we should decrease the mask counter mmTemplateList->dwMaskCnt. In this case, the code was just copied from the bottom part of the function, and thus, the extra string with the counter decrement was fixed incorrectly. Most likely, the expression should be replaced with:

mmTemplateList->dwMaskCnt=0;

This example shows quite vividly that you must be very careful when you copy your code. In this case, it might just be a typo, but as a result, we lose the value of the counter.

A similar bug, related to the loss of the background colour, was detected in another code fragment. But I will mention it only as a diagnostic message.

V607 Ownerless expression 'Frames[nFramescount].TitleBar.BackColour'. cluiframes.c 1717

Extra assignment

static INT_PTR CALLBACK DlgProcClistListOpts(....)
{
  ....
  tvi.iImage=tvi.iSelectedImage=tvi.iImage=!tvi.iImage;
  ....
}

V570 The same value is assigned twice to the 'tvi.iImage' variable. modern_clcopts.cpp 563

Assigning values to multiple variables at once is allowed in C++. This is quite convenient when using short variables in small functions. However, in large fragments it worsens the readability, and leads to additional errors. We can clearly see an error that occurred because of code copying, as in this project there is another version of a plugin written in C with the following string:

tvi.iImage = tvi.iSelectedImage = tvi.iImage == 1 ?  2 : 1;

In addition to this, the work with int type in the new plugin is done in the same way as with bool type.

The code can be fixed in the following way:

tvi.iImage=tvi.iSelectedImage=!tvi.iImage;

Or, to make it more readable we should split it into two strings:

tvi.iImage=!tvi.iImage;
tvi.iSelectedImage=tvi.iImage;

Similar errors can be seen in several places within the project.

V570 The same value is assigned twice to the 'mi.hIcon' variable. modern_clistmenus.cpp 157
V570 The same value is assigned twice to the 'button.pszTooltipUp' variable. jabber_menu.cpp 980
V570 The same value is assigned twice to the 'button.pszTooltipUp' variable. jabber_menu.cpp 986
V570 The same value is assigned twice to the 'button.pszTooltipUp' variable. jabber_menu.cpp 993

Assignment in the condition

Assignment in the condition is not always a mistake, but it can cause great difficulties when the programmer starts editing and checking the code. This error often haunted me, after switching to C++ from a different language. It's hard to notice this bug during a simple code check, and Visual C++ reports errors of this kind only if you assign the result of the function execution. But the analyzer is more attentive, and can detect all cases of similar errors.

int FindItem(....)
{
  ....
  int ret;
  ret=FindItem(hwnd,dat,hItem,
                (struct ClcContact ** )&z,
                (struct  ClcGroup** )&isv,NULL);
  if (ret=0) {return (0);}
  ....
}

V559 Suspicious assignment inside the condition expression of 'if' operator: ret = 0. clcidents.c 179

The given fragment shows a situation where the assignment inside a condition leads to premature function exit. This function is intended to search for elements, and there is a reference to it inside a variable, but inside the condition the variable value gets rewritten. As a result the function will always generate the same result regardless of whether the item was found or not.

There was a similar fragment in another place.

V559 Suspicious assignment inside the condition expression of 'if' operator: Drawing->type = 1. clcpaint.c 548

Repetition in the condition

Similar errors are fairly common. It's quite simple to detect repetitions in small code fragments, but in conditions with a high number of checks they somehow get lost. Static code analysis is perfect in such cases.

Here are several examples of such an error.

LONG_PTR CALLBACK HotkeyHandlerDlgProc(....)
{
  ....
  if (job->hOwner && 
      job->iAcksNeeded && 
      job->hOwner && 
      job->iStatus == SendQueue::SQ_INPROGRESS) 
      {
        if (IsWindow(job->hwndOwner))
           ....
      }
      ....
}

V501 There are identical sub-expressions 'job->hOwner' to the left and to the right of the '&&' operator. hotkeyhandler.cpp 564

We can see in the code that the job->hOwner variable is checked twice. Most likely, the variable should be changed to job->hwndOwner, as the main workload is related to this variable handling.

In another example, found by the V501 diagnostic, we can clearly see the repetition in the condition.

USERINFO* UM_AddUser(....)
{
  ....
  if (!pStatusList || !ppUserList || !ppUserList) // <=
       return NULL;
  ....   
}

V501 There are identical sub-expressions to the left and to the right of the '||' operator: !pStatusList ||!ppUserList ||!ppUserList manager.cpp 1267

In this case, the error is not critical, because the program won't get to the check of the third argument. But that does not mean that this code needs no editing: the unnecessary expression, !ppUserList ,has to be removed from the condition.

Do you need curly brackets?

void CInfoPanel::renderContent(const HDC hdc)
{
  ....
  if(m_height >= DEGRADE_THRESHOLD)
      rc.top -= 2; rc.bottom -= 2;
  ....
}

V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. infopanel.cpp 360

It's not easy to say which error is here, looking at this code fragment. Perhaps both commands should be executed only if the condition is true. In this case the code works incorrectly, and we should add curly brackets to the block of the operators. Additionally, we should separate the operators to improve the readability of the code.

if(m_height >= DEGRADE_THRESHOLD)
{
  rc.top -= 2; 
  rc.bottom -= 2;
}

Although, there is still a possibility that the code works in the way it was meant to, and the second operator should be always executed despite the condition. Then we have a formatting error that strongly hinders understanding of the code and we should move the rc.bottom -= 2; command to a different string.

Unnecessary checks

int GetDropTargetInformation(....)
{
  ....
  if (bottomItem==-1 && 
      contact->type!=CLCIT_GROUP && 
      contact->groupId==0)
      {
         if (contact->type!=CLCIT_GROUP && 
             contact->groupId==0)
             {
               ....
             }
      }
  ....
}

V571 Recurring check. The 'contact->type != 0' condition was already verified in line 406. modern_clcutils.cpp 408

Usually these errors indicate logic errors or typos in the variable names, but in this case it's just redundant code. In the fragment above, we can see that inside the nested expression the same conditions get checked as the ones that were already checked in the external block. This check does not make sense because the nested condition is always true.

The analyzer detected several more redundant conditions.

V571 Recurring check. The '!bFound' condition was already verified in line 1611. window.c 1612
V571 Recurring check. The 'hIcon == 0' condition was already verified in line 571. modern_statusbar.cpp 573
V571 Recurring check. The 'dat->windowData.hwndLog != ((void *) 0)' condition was already verified in line 1241. msgdialog.c 1242
V571 Recurring check. The 'windowOpen' condition was already verified in line 946. eventpopups.cpp 947
V571 Recurring check. The '!isShift' condition was already verified in line 787. msgdialog.cpp 788

Conditional blocks, performing the same code

This code is usually a sign of a logical error. But there are other cases that may not always be interpreted as errors.

HRESULT CLUI::CreateCLC()
{
  ....
  if (bOldUseGroups !=(BYTE)-1)  
    CallService( MS_CLIST_SETUSEGROUPS ,
                 (WPARAM)bOldUseGroups, 0);
  else
    CallService( MS_CLIST_SETUSEGROUPS ,
                 (WPARAM)bOldUseGroups, 0);
  ....
};

V523 The 'then' statement is equivalent to the 'else' statement. modern_clui.cpp 445

In this case the conditional block was probably written to keep the coding style. Or, these blocks were meant for handling these errors but were never written. This is' why the code blocks look suspicious, and they should be catered for.

In Miranda IM, there were quite a number of these blocks, so we'll just list them here:

V523 The 'then' statement is equivalent to the 'else' statement. modern_clcpaint.cpp 1038
V523 The 'then' statement is equivalent to the 'else' statement. modern_clistsettings.cpp 308
V523 The 'then' statement is equivalent to the 'else' statement. modern_popup.cpp 95
V523 The 'then' statement is equivalent to the 'else' statement. pluginbmp.cpp 602
V523 The 'then' statement is equivalent to the 'else' statement. pluginbmp.cpp 810
V523 The 'then' statement is equivalent to the 'else' statement. pluginbmp.cpp 956
V523 The 'then' statement is equivalent to the 'else' statement. bsplinerotate.cpp 675
V523 The 'then' statement is equivalent to the 'else' statement. msglog.c 424
V523 The 'then' statement is equivalent to the 'else' statement. msglog.c 677
V523 The 'then' statement is equivalent to the 'else' statement. container.cpp 804
V523 The 'then' statement is equivalent to the 'else' statement. msglog.cpp 447
V523 The 'then' statement is equivalent to the 'else' statement. msgs.c 135
V523 The 'then' statement is equivalent to the 'else' statement. irclib.cpp 365
V523 The 'then' statement is equivalent to the 'else' statement. coolscroll.cpp 1427

Conclusion

Miranda IM is developing slower than it used to, but the project still contains a lot of errors, of varying severity levels. This shows that static analysis is important at every stage of development. PVS-Studio analyzer helps you to find very tricky and nasty errors. If you develop a project in C, C++, or C#, I suggest downloading PVS-Studio and checking your project http://www.viva64.com/en/pvs-studio/download/.

#Cpp