Александр Чибисов

30 Май 2016

Теги:

#Cpp

Опечатки в Miranda IM

30 Май 2016

Автор: Александр Чибисов

Введение
Некорректное освобождение памяти
Несоблюдение приоритета операций
"Потерянное выражение"
Лишнее присваивание
Присваивание в условии
Повтор в условии
Нужны ли скобки?
Излишние проверки
Условные блоки, выполняющие одинаковый код
Заключение

Статья посвящена часто встречающимся ошибкам, возникающим из-за опечаток на примере проекта Miranda IM. Многие подобные ошибки могут привести к некорректному поведению программы, а некоторые из них не наносят явного вреда, но приводят к ухудшению понятности кода.

Введение

Miranda IM известная программа обмена мгновенными сообщениями. Исходный код программы был взят мною из репозитория SourceForge, где доступны все версии исходников программы. Для проверки использовалась версия Miranda IM 0.10.50 и анализатор PVS-Studio версии 6.03. Проект уже анализировался ранее, и о результатах можно почитать в заметке "Как уменьшить вероятность ошибки на этапе написания кода". В исходном коде Miranda IM анализатор указал на многие проблемные места. Среди сообщений анализатора попадались и такие, которые невозможно точно идентифицировать как ошибочные, возможно это просто слишком хитрый код. Для статьи такие фрагменты не подходят, и поэтому были выбраны только наиболее интересные из ошибок.

Некорректное освобождение памяти

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

Анализатор оповещает об ошибке работы с памятью при уничтожении объекта. В случае, когда в списке нет элементов функция завершится преждевременно, при этом память, выделенная под массив recentEntries будет очищена некорректно. В то же время, если функция будет исполнена до конца, то объект будет уничтожен правильно, именно поэтому данную ошибку можно отнести к категории опечаток. При создании массива с помощью new[] для корректного уничтожения и очистки памяти необходимо использовать команду delete[]. Использование функции free совместно с оператором new недопустимо. При очистке памяти функция free не вызывает деструкторы объектов, что может привести к неопределённому поведению программы. Да и само по себе такое освобождение памяти, уже является неопределённым поведением. Для исправления ошибки необходимо привести код к единому виду и заменить функцию free на оператор delete[].

Несоблюдение приоритета операций

Приоритет операций очень важен. Часто из-за несоблюдения приоритета происходят ошибки в расчётах или конструкция ведёт себя совсем не так, как ожидает программист.

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

Фрагмент кода показывает, как неверно поставленная закрывающаяся скобка, привела к ошибке в работе тернарного оператора. Так как оператор побитового ИЛИ имеет больший приоритет чем тернарный оператор сначала вычисляется выражение MF_BYCOMMAND | (nen_options.bTraySupport) и только после этого полученное значение сравнивается внутри тернарной конструкции. Для исправления ошибки код следует изменить следующим образом.

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

Самое забавное, что на самом деле это самая настоящая ошибка никак не влияет на корректность работы программы. Дело в том, что константа MF_BYCOMMAND представляет собой ни что иное, как 0x00000000L. Подробнее именно такой случай рассматривает Андрей Карпов в своей небольшой электронной книге "Главный вопрос программирования, рефакторинга и всего такого" (см. главу N39: Почему некорректный код иногда работает).

Ещё один пример связанный с неверным приоритетом операций:

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

Здесь при проверке второго выражения вместо применения оператора логического отрицания к выражению tmp->state == GG_STATE_WAITING_FOR_ACCEPT проверка применяется к переменой tmp->state и только после этого сравнивается с константой GG_STATE_WAITING_FOR_ACCEPT. Для исправления ошибки требуется взять второе выражение в скобки и условие изменится следующим образом:

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

Хотя, конечно, проще использовать оператор != и упростить код:

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

"Потерянное выражение"

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

По приведённому фрагменту видно, что функция создана для удаления маски по ID. Если количество масок больше единицы, то требуется уменьшать счётчик масок mmTemplateList->dwMaskCnt. В данном случае код был просто скопирован из нижней части функции и в результате лишняя строчка с декрементом счётчика была исправлена некорректно. Вероятнее всего выражение требуется изменить на:

mmTemplateList->dwMaskCnt=0;

Данный пример ярко показывает, что необходимо быть очень внимательным при копировании кода. В данном случае это может быть просто опечатка, но в результате теряется значение счётчика.

Похожая ошибка с потерянным значением цвета фона была обнаружена в другом участке кода. Но упомяну о ней только в виде диагностического сообщения.

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

Лишнее присваивание

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

Присваивание значения нескольким переменным сразу допустимы в языке С++. Это достаточно удобно при использовании коротких переменных в небольших функциях. Но в больших фрагментах кода такая запись ухудшает читаемость кода и ведёт к появлению дополнительных ошибок. Здесь явно видна ошибка, произошедшая в результате копирования кода, так как в этом же проекте лежит другая версия плагина на языке C и там используется следующая строка кода:

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

К тому же в новом плагине работа с типом int, осуществляется исключительно как с типом bool.

Исправить код возможно следующим образом:

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

Или для улучшения читаемости разделить его на две строки:

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

Похожие ошибки можно увидеть ещё в нескольких местах проекта.

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

Присваивание в условии

Присваивание в условии не всегда является ошибкой, но может привести к большим сложностям при изменениях и проверках кода. Данная ошибка часто преследовала меня после перехода на C++ с другого языка программирования. Её достаточно сложно заметить при обычной проверке кода, а компилятор Visual C++ сообщает о подобных ошибках только если присваивается результат выполнения функций. Но анализатор работает более внимательно и может выявить все случаи подобных ошибок.

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

Приведённый фрагмент показывает ситуацию, когда присваивание внутри условия приводит к преждевременному выходу из функции. Данная функция предназначена для поиска элемента и внутри переменной хранится ссылка на него, но из-за ошибки внутри условия значение переменой перезаписывается. В результате функция всегда будет выдавать одинаковый результат независимо от того, был найден элемент или нет.

Похожий фрагмент присутствует ещё в одном месте.

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

Повтор в условии

Похожие ошибки в программах встречаются достаточно часто. В небольших фрагментах обнаружить повторы достаточно просто, а вот в условиях с большим количеством проверок они теряются из виду. Именно в таких случаях для поиска ошибки поможет статический анализ кода.

Приведу несколько примеров данной ошибки.

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

Из приведённого кода видно, что переменная job->hOwner проверяется дважды. Скорее всего во втором случае переменную следует изменить на job->hwndOwner, так как именно с ней продолжается работа внутри условного блока.

В другом примере, найденном диагностикой V501, явно видно место повтора в условии.

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

В данном случае ошибка не является критичной, так как до проверки третьего аргумента в условии программа никогда не дойдёт. Но это не значит, что данный фрагмент кода не нуждается в исправлении: необходимо убрать лишнее выражение !ppUserList из условия.

Нужны ли скобки?

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

По данному коду сложно установить какая именно ошибка перед нами. Скорее всего обе команды должны выполняться только при выполнении условия. В этом случае код работает некорректно и для его исправления необходимо добавить фигурные скобки вокруг блока операторов. Дополнительно, для улучшения читаемости кода стоит разделить операторы на несколько строк.

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

Но может быть и так, что код работает как задумано и второй оператор должен выполняться всегда, независимо от условия. Тогда налицо ошибка форматирования, которая сильно мешает пониманию кода, и для её исправления необходимо команду rc.bottom -= 2; перенести на новую строку.

Излишние проверки

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

Часто подобные ошибки свидетельствуют об опечатках в именах переменных или логических ошибках, но в данном случае это просто избыточный код. По приведённому фрагменту видно, что внутри вложенного условия проверяются те же выражения, что уже были проверены во внешнем блоке. Подобная проверка не имеет смысла, так как вложенное условие будет всегда истинно.

Анализатор обнаружил ещё несколько избыточных условий.

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

Условные блоки, выполняющие одинаковый код

Такой код чаще всего освидетельствует о наличии логической ошибки. Но бывают и другие ситуации, которые не всегда можно трактовать как ошибку.

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

В данном случае условный блок скорее всего был сделан для поддержания стиля кода. Или данные блоки должны были служить для обработки ошибок, но так и не были дописаны. Именно поэтому блоки кода выглядят подозрительно, и на них стоит обратить внимание.

В проекте Miranda IM встречается достаточно много таких блоков, и их лучше показать просто списком.

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

Заключение

Miranda IM развивается уже не так быстро, как раньше, но в проекте ещё много неисправленных ошибок разного уровня опасности. Это показывает, что статический анализ кода важен на любом этапе развития проекта. Анализатор PVS-Studio поможет найти весьма заковыристые и подлые ошибки. Если вы разрабатываете проект на C, C++ или C#, предлагаю не откладывая скачать PVS-Studio и проверить свой проект: http://www.viva64.com/ru/pvs-studio/download/

#Cpp