PVS-Studio - это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#.

PVS-Studio выполняет статический анализ кода и генерирует отчёт, помогающий программисту находить и устранять ошибки. PVS-Studio выполняет широкий спектр проверок кода, но наиболее силён в поисках опечаток и последствий неудачного Copy-Paste. Показательные примеры таких ошибок: V501, V517, V522, V523, V3001.

Анализатор ориентирован на разработчиков, использующих среду Visual Studio, и может в фоновом режиме выполнять анализ измененных файлов после их компиляции. В идеале ошибки будут обнаружены и исправлены ещё до попадания в репозиторий. Однако ничто не мешает использовать анализатор для проверки всего решения целиком или для встраивания в системы непрерывной интеграции. Эти и иные способы использования анализатора описаны в документации.

Опечатки в Miranda IM

Запись от el_programmer размещена 30.05.2016 в 16:24

Показов 2377 Комментарии 0

Метки bugs, c++, cpp, miranda, open source, programming

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

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

Нажмите на изображение для увеличения
Название: image1.png
Просмотров: 583
Размер: 52.5 Кб
ID: 3869

Введение

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

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

C++
1
2
3
4
5
6
7
8
9
10
11
12
13
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[].

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

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

C++
1
2
3
4
5
6
7
8
9
10
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)и только после этого полученное значение сравнивается внутри тернарной конструкции. Для исправления ошибки код следует изменить следующим образом.

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

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

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

C++
1
2
3
4
5
6
7
8
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. Для исправления ошибки требуется взять второе выражение в скобки и условие изменится следующим образом:

C++
1
2
3
if (tmp->peer_uin == uin && 
    !(tmp->state == GG_STATE_WAITING_FOR_ACCEPT))
      return tmp;

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

C++
1
2
3
if (tmp->peer_uin == uin && 
    tmp->state != GG_STATE_WAITING_FOR_ACCEPT)
      return tmp;

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

C++
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
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. В данном случае код был просто скопирован из нижней части функции и в результате лишняя строчка с декрементом счётчика была исправлена некорректно. Вероятнее всего выражение требуется изменить на:

C++
1
mmTemplateList->dwMaskCnt=0;

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

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

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

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

C++
1
2
3
4
5
6
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 и там используется следующая строка кода:

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

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

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

C++
1
tvi.iImage=tvi.iSelectedImage=!tvi.iImage;

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

C++
1
2
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++ сообщает о подобных ошибках только если присваивается результат выполнения функций. Но анализатор работает более внимательно и может выявить все случаи подобных ошибок.

C++
1
2
3
4
5
6
7
8
9
10
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

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

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

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

C++
1
2
3
4
5
6
7
8
9
10
11
12
13
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, явно видно место повтора в условии.

C++
1
2
3
4
5
6
7
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из условия.

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

C++
1
2
3
4
5
6
7
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

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

C++
1
2
3
4
5
if(m_height >= DEGRADE_THRESHOLD)
{
  rc.top -= 2; 
  rc.bottom -= 2;
}

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

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

C++
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
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

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

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

C++
1
2
3
4
5
6
7
8
9
10
11
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 и проверить свой проект.

Метки bugs, c++, cpp, miranda, open source, programming

Размещено в Без категории

« Microsoft открыла исходники Xamarin.Forms. Мы не могли упустить шанс проверить их с помощью PVS-Studio Главная страница Проверяем исходный код 7-Zip с помощью PVS-Studio »

Надоела реклама? Зарегистрируйтесь и она исчезнет полностью.

Всего комментариев 0

Комментарии

Новые блоги и статьи Все статьи Все блоги /
20. Мат мед. Абсентеизм как отдельный тип простоя anaschu 29.05.2026 Апдейт модели: исправленные баги, абсентеизм и новые механизмы Продолжаю развивать ранее описанную модель рабочего коллектива на AnyLogic. За последние несколько дней был проведён серьёзный. . .	19. здоровье, усталость и психотип работника влияют на производительность предприятия, и наоборот, производительность на здоровье, усталось и психотип anaschu 28.05.2026 Дискретно-событийная модель рабочего коллектива на AnyLogic: здоровье, выгорание, психотипы и микростимуляция Привет, коллеги. Хочу поделиться итогами нескольких недель работы над симуляционной. . .	"Прокси" для последовательного порта Eddy_Em 28.05.2026 Эту штуку написал я достаточно давно. Но сейчас вот понадобилось настроить датчик грозы, но при этом не отключать его от "метеодемона". Соответственно, надо запустить этот "прокси": метеодемон будет. . .	Рефакторинг программы уравнивания. Massaraksh7 26.05.2026 Пример по предыдущей записи в блоге. Но, надо заметить, что, во-первых, там оптимизация не только математики, но и работы с базой данных, и с графами, а во-вторых, это ещё не всё.
Использование TThread в Lazarus для математических вычислений. Massaraksh7 25.05.2026 Производя рефакторинг своих программ на предмет ускорения их работы, обратил внимание на такой аспект, как сокращение времени матвычислений. Дело в том, что приходится работать с большими матрицами. . .	Модель здравосохранения 18. Чем здоровее работник, тем быстрее выгорает anaschu 24.05.2026 Имитационная модель корпоративного здравоохранения: что показывает математика Сегодня в модели рабочего коллектива на AnyLogic появились три новые механики — выгорание через накопленную усталость,. . .	Модель здравосохранения 17. Планы на выгорание anaschu 23.05.2026 Вот конкретная схема реализации: В классе Работник добавить: накопленнаяУсталость — растёт каждый час работы, снижается в перерывы и болезни коэффициентПрезентеизма — снижает продуктивность. . .	Изменение цветов в палитре gif файла aka фавикона russiannick 23.05.2026 Изменение цветов в палитре gif файла, юзаемого как фавиконка в составе html-файла, помещенная в base64, средствами нативного Java Script, навеянное сном в майский день. Для работы необходим браузер,. . .

Сообщение форума

Отменить изменения