http://habrahabr.ru/company/pvs-studio/blog/244415/


Продолжим рассматривать ошибки, которые удалось обнаружить в проекте Miranda NG с помощью статического анализатора кода PVS-Studio. В прошлый раз мы говорили об указателях и работе с памятью. Теперь поговорим об ошибках общего плана, которые, в основном, связаны с неаккуратностью и опечатками.

Продолжение проверки

Предыдущая часть обзора кода Miranda NG доступна здесь.

Опечатки

Начну вот с такой красивой опечатки. Рядом с клавишей '=' находится клавиша '-'. Вот что из-за этого получилось:

CBaseTreeItem* CMsgTree::GetNextItem(....)
{
  ....
  int Order = TreeCtrl->hItemToOrder(TreeView_GetNextItem(....));
  if (Order =- -1)
    return NULL;
  ....
}

Предупреждение PVS-Studio: V559 Suspicious assignment inside the condition expression of 'if' operator: Order = — - 1. NewAwaySys msgtree.cpp 677

Естественно, здесь должно быть написано: if (Order == -1).

А вот тут забыли звёздочку '*':

HWND WINAPI CreateRecentComboBoxEx(....)
{
  ....
  if (dbv.ptszVal != NULL && dbv.ptszVal != '\0') {
  ....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: dbv.ptszVal != 0 && dbv.ptszVal != '\0' SimpleStatusMsg msgbox.cpp 247

Хотели проверить что указатель ненулевой и что строка не пустая. Но забыли разыменовать указатель. Получилось, что два раза проверили указатель на равенство нулю.

Правильный вариант:

if (dbv.ptszVal != NULL && *dbv.ptszVal != '\0') {

Это ошибка обнаруживается и с помощью другой диагностики: V528 It is odd that pointer to 'wchar_t' type is compared with the L'\0' value. Probably meant: *dbv.ptszVal != L'\0'. SimpleStatusMsg msgbox.cpp 247

Это нередкая ситуация, когда на одну ошибку указывает 2, а то и 3 диагностики. Получается так, что ошибка делает код подозрительным сразу с нескольких точек зрения.

Есть ещё несколько V528 и я предлагаю проверить соответствующий код:

• options.cpp 759
• exportimport.cpp 425
• exportimport.cpp 433
• exportimport.cpp 441

Некий массив заголовков копируется сам в себя. Скорее всего, здесь какая-то опечатка:

int InternetDownloadFile (char *szUrl) 
{
  ....
  CopyMemory(nlhr.headers, nlhr.headers,
             sizeof(NETLIBHTTPHEADER)*nlhr.headersCount);
  ....
}

Предупреждение PVS-Studio: V549 The first argument of 'memcpy' function is equal to the second argument. NimContact http.cpp 46

Вот ещё одна схожая ситуация:

TCHAR* get_response(TCHAR* dst, unsigned int dstlen, int num)
{
  ....
  TCHAR *tmp, *src = NULL;
  ....
  src = (TCHAR*)malloc(MAX_BUFFER_LENGTH * sizeof(TCHAR));
  ....
  _tcscpy(src, src);
  ....
}

Предупреждение PVS-Studio: V549 The first argument of 'wcscpy' function is equal to the second argument. Spamotron utils.cpp 218

Строка копируется сама в себя. Я подозреваю, что в качестве одного из аргументов должен был использоваться указатель 'dst'.

#define TTBBF_ISLBUTTON      0x0040

INT_PTR TTBAddButton(WPARAM wParam, LPARAM lParam)
{
  ....
  if (!(but->dwFlags && TTBBF_ISLBUTTON) &&
      nameexists(but->name))
    return -1;
  ....
}

Предупреждение PVS-Studio: V560 A part of conditional expression is always true: 0x0040. TopToolBar toolbar.cpp 307

Скорее всего, рука дернулась и вместо '&' получилось '&&'.

И последний случай, где вместо сравнения происходит присваивание:

#define MAX_REPLACES 15
INT_PTR CALLBACK DlgProcCopy(....)
{
  ....
  if (string == newString[k])
    k--;
  if (k = MAX_REPLACES) break;
  string = oldString[++k];
  i+=2;
  ....
}

Предупреждение PVS-Studio: V559 Suspicious assignment inside the condition expression of 'if' operator: k = 15. NimContact contactinfo.cpp 339

Недописанный код

INT_PTR SVC_OTRSendMessage(WPARAM wParam,LPARAM lParam){
  ....
  CCSDATA *ccs = (CCSDATA *) lParam;
  ....
  if (otr_context_get_trust(context) >= TRUST_UNVERIFIED)
    ccs->wParam;
  ....
}

Предупреждение PVS-Studio: V607 Ownerless expression 'ccs->wParam'. MirOTR svcs_proto.cpp 103

Если условие выполнилось, то ничего не произойдёт. Возможно хотели присвоить переменной «ccs->wParam» какое-то значение. Аналогичное предупреждение выдаётся также здесь: bandctrlimpl.cpp 226.

А вот недописанный цикл:

extern "C" __declspec(dllexport) int  Load(void)
{
  ....
  for (i = MAX_PATH; 5; i--){
  ....
}

Предупреждение PVS-Studio: V654 The condition '5' of loop is always true. Xfire main.cpp 1110

С циклом что-то не так. Мне кажется забыли сравнить 'i' с числом '5'. Плюс этот цикл скопирован ещё в одно место программы: variables.cpp 194.

Невнимательность

int findLine(...., int *positionInOldString)
{
  ....
    *positionInOldString ++; 
     return (linesInFile - 1);
  }
  ....
}

V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. NimContact namereplacing.cpp 92

Есть большое подозрение, что хотели изменить переменную, на которую ссылается указатель 'positionInOldString'. Но получилось, что изменили сам указатель.

Скорее всего, код следует изменить следующим образом:

(*positionInOldString)++; 

«Перезапись» значения

INT_PTR TTBSetState(WPARAM wParam, LPARAM lParam)
{
  mir_cslock lck(csButtonsHook);

  TopButtonInt *b = idtopos(wParam);
  if (b == NULL)
    return -1;

  b->bPushed = (lParam & TTBST_PUSHED) ? TRUE : FALSE;
  b->bPushed = (lParam & TTBST_RELEASED) ? FALSE : TRUE;
  b->SetBitmap();
  return 0;
}

Предупреждение PVS-Studio: V519 The 'b->bPushed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 358, 359. TopToolBar toolbar.cpp 359

Странно, что в начале в переменную было помещено какое-то значение, а затем неожиданно оно перетирается другим значением.

Ещё один пример:

static INT_PTR CALLBACK sttOptionsDlgProc(....)
{
  ....
  rc.left += 10;
  rc.left = prefix + width * 0;
  ....
}

V519 The 'rc.left' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 583, 585. Miranda hotkey_opts.cpp 585

Конечно, записывать в одну переменную подряд 2 разных значения не всегда является ошибкой. Иногда переменную на всякий случай инициализируют нулём, а потом используют. Есть и другие корректные ситуации. Однако я выписал 14 предупреждений, которые указывают, по моему мнению, на подозрительный код: MirandaNG-519.txt.

Иногда предупреждение V519 косвенно выявляет ситуацию, когда забыт оператор 'break':

void OnApply()
{
  ....
  case ACC_FBOOK:
    m_proto->m_options.IgnoreRosterGroups = TRUE;
      
  case ACC_OK:
    m_proto->m_options.IgnoreRosterGroups = TRUE;
    m_proto->m_options.UseSSL = FALSE;
    m_proto->m_options.UseTLS = TRUE;

  case ACC_TLS:
  case ACC_LJTALK:
  case ACC_SMS:
    m_proto->m_options.UseSSL = FALSE;
    m_proto->m_options.UseTLS = TRUE;
    break;
  ....
}

Предупреждение PVS-Studio: V519 The 'm_proto->m_options.IgnoreRosterGroups' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1770, 1773. Jabber jabber_opt.cpp 1773

Одинаковые фрагменты кода

Встречаются места, где, независимо от условия, будут выполнены одни и те же действия.

static void Build_RTF_Header()
{
  ....
  if (dat->dwFlags & MWF_LOG_RTL)
    AppendToBuffer(buffer, bufferEnd, bufferAlloced,
                   "{\\rtf1\\ansi\\deff0{\\fonttbl");
  else
    AppendToBuffer(buffer, bufferEnd, bufferAlloced,
                   "{\\rtf1\\ansi\\deff0{\\fonttbl");
  ....
}

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. TabSRMM msglog.cpp 439

Возможно, код писался с помощью Copy-Paste. При этом одну из строк забыли поправить.

Есть ещё 9 таких подозрительных мест: MirandaNG-523.txt.

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

(прошло время)

Итак, продолжим. Copy-Paste может проявлять себя ещё вот так:

static int RoomWndResize(...., UTILRESIZECONTROL *urc)
{
  ....
  urc->rcItem.top = (bToolbar && !bBottomToolbar) ?
                      urc->dlgNewSize.cy - si->iSplitterY :
                      urc->dlgNewSize.cy - si->iSplitterY;
  ....
}

Предупреждение PVS-Studio: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: urc->dlgNewSize.cy — si->iSplitterY. TabSRMM window.cpp 473

Зачем нужен оператор "?:", если вычисляется одно и тоже выражение?

Ещё 11 бессмысленных тернарных операторов: MirandaNG-583.txt.

Подозрительные операции деления

void CSkin::setupAeroSkins()
{
  ....
  BYTE alphafactor = 255 - ((m_dwmColor & 0xff000000) >> 24);
  ....
  fr *= (alphafactor / 100 * 2.2);
  ....
}

Предупреждения PVS-Studio: V636 The 'alphafactor / 100' expression was implicitly casted from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. TabSRMM themes.cpp 1753

У меня есть подозрение, что программист хотел, чтобы деление «alphafactor / 100» было не целочисленным. Сейчас получается, что, поделив переменную типа BYTE на 100, можно получить только три значения: 0, 1 и 2.

Вероятно, нужно делить так:

fr *= (alphafactor / 100.0 * 2.2);

В этом же файле можно найти ещё 2 подозрительных операции деления в строке 1758 и 1763.

WTF?

static INT_PTR CALLBACK DlgProc_EMail(....)
{
  case WM_COMMAND:
    switch (LOWORD(wParam)) {
      if (HIWORD(wParam) == BN_CLICKED) {
        case IDOK:
  ....
}

Предупреждение PVS-Studio: V622 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. UInfoEx ctrl_contact.cpp 188

Что-это за строчка «if (HIWORD(wParam) == BN_CLICKED) {» перед «case IDOK»? Она никогда не получит управление. Что вообще хотел этим сказать программист?

Ещё одно такое место есть чуть ниже (в строке 290).

Странно отформатированный код

С кодом, приведённом ниже, что-то не в порядке. Но непонятно, что. То ли он неудачно отформатирован, то ли не дописан.

int ExtractURI(....)
{
  ....
  while ( msg[i] && !_istspace(msg[i])) 
  {
    if ( IsDBCSLeadByte(msg[i] ) && msg[i+1]) i++;
    else                                               <<<---
  
    if ( _istalnum(msg[i]) || msg[i]==_T('/')) 
    {
      cpLastAlphaNum = charCount; 
      iLastAlphaNum = i;
    }
    charCount++;
    i++;
  }
  ....
}

Предупреждение PVS-Studio: V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. LinkList linklist_fct.cpp 92

Обратите внимание на странный 'else'.

Встретилось вот такое:

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

Предупреждение PVS-Studio: 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. TabSRMM infopanel.cpp 370

Очень вероятно, что программист забыл написать здесь фигурные скобки. Из 'rc.bottom' всегда вычитается двойка.

На этом страшные истории не заканчиваются. Нужно посмотреть ещё вот сюда:

• msn_p2p.cpp 385
• crypt_lists.cpp 13
• crypt_lists.cpp 44
• common.c 273
• common.c 307

Остановка цикла на самом интересном месте

bool PopupSkin::load(LPCTSTR dir)
{
  ....
  while (hFind != INVALID_HANDLE_VALUE) {
    loadSkin(ffd.cFileName);
    break;
    if (!FindNextFile(hFind, &ffd))
      break;
  }
  ....
}

Предупреждение PVS-Studio: V612 An unconditional 'break' within a loop. Popup skin.cpp 807

Зачем нужен 'break' в середине цикла? Последствие неудачного рефакторинга? И к сожалению, не единственное:

• icq_servlist.cpp 226
• rawping.cpp 159
• main.cpp 304
• gfileutils.c 266

Всегда истинные или ложные условия

Чаще всего эта ошибка связана с проверками вида (UNSIGNED < 0) или (UNSIGNED >=0). Но бывают и более экзотические варианты. Указатель сравнивается со строкой:

static void yahoo_process_search_connection(....)
{
  ....
  if (cp != "\005")
  ....
}

Предупреждение PVS_Studio: V547 Expression 'cp != "\005"' is always true. To compare strings you should use strcmp() function. Yahoo libyahoo2.cpp 4486

Но вернёмся к классике жанра. Приведу только один пример, а остальные предупреждения будут, как обычно, списком.

ULONG_PTR itemData;

LONG_PTR CALLBACK HotkeyHandlerDlgProc(....)
{
  ....
  if (dis->itemData >= 0) {
  ....
}

Предупреждение PVS-Studio: V547 Expression 'dis->itemData >= 0' is always true. Unsigned type value is always >= 0. TabSRMM hotkeyhandler.cpp 213

Обещанный список: MirandaNG-547.txt.

Кто-то не знает, как работают функции strchr() и strrchr()

#define mir_strchr(s,c) (((s)!=0)?strchr((s),(c)):0)
#define mir_strrchr(s,c) (((s)!=0)?strrchr((s),(c)):0)
BYTE CExImContactBase::fromIni(LPSTR& row)
{
  ....
  if (cchBuf > 10 && (p1 = mir_strrchr(pszBuf, '*{')) &&
      (p2 = mir_strchr(p1, '}*')) && p1 + 2 < p2) {
  ....
}

Предупреждения PVS-Studio:

• V575 The 'strrchr' function processes value '10875'. Inspect the second argument. UInfoEx classeximcontactbase.cpp 177
• V575 The 'strchr' function processes value '32042'. Inspect the second argument. UInfoEx classeximcontactbase.cpp 177

Видимо кто-то хотел найти фрагмент текста, обрамлённого символами "*{" и "}*". Но получилась какая-то глупость.

Во-первых, функции strchr() и strrchr() ищут один символ, а не подстроку.

Во-вторых, '*{' интерпретируется как число 10875. Функции в качестве второго аргумента ожидают значения типа 'int', но это ничего не значит. Они используют от этого аргумента только младший байт.

И к сожалению, это не случайная, а систематическая ошибка.

Есть 10 таких-же некорректных вызовов: MirandaNG-575.txt.

Неопределённое поведение

void FacebookProto::UpdateLoop(void *)
{
  ....
  for (int i = -1; !isOffline(); i = ++i % 50)
  ....
}

Предупреждение PVS-Studio: V567 Undefined behavior. The 'i' variable is modified while being used twice between sequence points. Facebook connection.cpp 191

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

Правильней и понятней будет написать так: i = (i + 1) % 50.

Ещё одно опасное место здесь: dlg_handlers.cpp 883.

Теперь рассмотрим более интересный пример:

void importSettings(MCONTACT hContact, char *importstring )
{
  ....
  char module[256] = "", setting[256] = "", *end;
  ....
  if (end = strpbrk(&importstring[i+1], "]")) {
    if ((end+1) != '\0') *end = '\0';
    strcpy(module, &importstring[i+1]);
  }
  ....
}

Предупреждение PVS_Studio: V694 The condition ((end + 1) != '\0') is only false if there is pointer overflow which is undefined behaviour anyway. DbEditorPP exportimport.cpp 425

Вообще, здесь банальная опечатка. Хотели проверить, что указатель 'end' указывает на символ, стоящий перед терминальным нулём. Ошибка в том, что забыли разменивать указатель. Должно быть написано:

if (*(end+1) != '\0')

А причём здесь неопределенное поведение? Сейчас мы это обсудим.

Вообще, эта ошибка диагностируется и другой диагностикой (V528). Но мне интересно поговорить именно о неопределённом поведении. Я хочу показать, что даже когда анализатор говорит что-то невнятное, не стоит спешить, а следует подумать, что его смущает в коде.

Итак, прибавляя к указателю 1, мы всегда получаем значение отличное от NULL. Кроме одного единственного случая: если произойдет переполнение, мы получим NULL. Но стандарт языка говорит, что это неопределённое поведение.

Таким образом анализатор нашел условие, которое или всегда истинно, или приводит к неопределённому поведению. А это значит, что с кодом что-то не в порядке.

Другие неправильные проверки:

• exportimport.cpp 433
• exportimport.cpp 441
• openfolder.cpp 35
• skype.cpp 473

И последнее на тему неопределённого поведения. Поговорим о сдвигах:

METHODDEF(boolean)
decode_mcu_AC_refine (....)
{
  ....
  m1 = (-1) << cinfo->Al;
  ....
}

Предупреждение PVS-Studio: V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1)' is negative. AdvaImg jdarith.c 460

Плюс:

• jdhuff.c 930
• cipher.c 1529

Нет виртуального деструктора

Есть базовый класс CNetClient:

class CNetClient
{
public:
  CNetClient(): Stopped(FALSE) {}
  virtual void Connect(const char* servername,const int port)=0;
  virtual void Send(const char *query)=0;
  virtual char* Recv(char *buf=NULL,int buflen=65536)=0;
  virtual void Disconnect()=0;
  virtual BOOL Connected()=0;
  virtual void SSLify()=0;
  ....
};

Как видите, в нём есть виртуальные функции, но нет виртуального деструктора. От него наследуются какие-то другие классы:

class CNLClient: public CNetClient { .... };

И последний штрих. Есть, например, вот такой класс:

class CPop3Client
{
  ....
 
  class CNetClient *NetClient;
  
  ~CPop3Client() {
    if (NetClient != NULL) delete NetClient;
  }

  ....
};

Предупреждения PVS-Studio: V599 The virtual destructor is not present, although the 'CNetClient' class contains virtual functions. YAMN pop3.h 23

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

Аналогично, плохо дело обстоит со следующими классами:

• CUpdProgress
• FactoryBase
• ContactCompareBase

Неправильное форматирование строк

static const char* 
ConvertAnyTag(FITAG *tag) {
  ....
  UINT64 *pvalue = (UINT64 *)FreeImage_GetTagValue(tag);
  sprintf(format, "%ld", pvalue[0]);
  ....
}

Предупреждение PVS-Studio: V576 Incorrect format. Consider checking the third actual argument of the 'sprintf' function. The argument is expected to be not greater than 32-bit. AdvaImg tagconversion.cpp 202

О том, как делать правильно, написано здесь: "Как правильно распечатать значение типа __int64, size_t и ptrdiff_t".

Дополнительно нужно поправить эти места в коде: MirandaNG-576.txt.

Разное

Странное сравнение:

#define CPN_COLOURCHANGED     1
#define CBN_SELCHANGE       1
INT_PTR CALLBACK DlgPopupOpts(....)
{
  ....
  if (wNotifyCode == CPN_COLOURCHANGED) {
    ....
  }
  else if (wNotifyCode == CBN_SELCHANGE) {
    ....
  }
  ....
}

Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 243, 256. PluginUpdater options.cpp 243

Неправильно используется функция ZeroMemory():

static int ScanFolder(....)
{
  ....
  __except (EXCEPTION_EXECUTE_HANDLER)
  {
    ZeroMemory(szMyHash, 0);
    // smth went wrong, reload a file from scratch
  }
  ....
}

Предупреждение PVS-Studio: V575 The 'memset' function processes '0' elements. Inspect the third argument. PluginUpdater dlgupdate.cpp 652

Функция ничего не обнуляет, так как второй аргумент равен нулю. Ещё один такой неправильный вызов здесь: shlipc.cpp 68.

Двойная проверка:

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

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'job->hContact' to the left and to the right of the '&&' operator. TabSRMM hotkeyhandler.cpp 523

Мне кажется, вторая проверка ' job->hContact' просто лишняя и её можно удалить. Тем не менее, предлагаю проверить это место и вот эти:

• ekhtml_mktables.c 67
• affixmgr.cxx 1784
• affixmgr.cxx 1879
• ac.c 889

Двойное освобождение ресурса:

static INT_PTR ServiceCreateMergedFlagIcon(....)
{
  HRGN hrgn;
  ....
  if (hrgn!=NULL) {
    SelectClipRgn(hdc,hrgn);
    DeleteObject(hrgn);
    ....
    DeleteObject(hrgn);
  }
  ....
}

Предупреждение PVS-Studio: V586 The 'DeleteObject' function is called twice for deallocation of the same resource. Check lines: 264, 273. UInfoEx svc_flagsicons.cpp 273

Что не вошло в статью

У меня уже больше нет сил. Многое, несущественное я поленился описывать. Ну, например, подобное:

#define MF_BYCOMMAND 0x00000000L
void CMenuBar::updateState(const HMENU hMenu) const
{
  ....
  ::CheckMenuItem(hMenu, ID_VIEW_SHOWAVATAR,
    MF_BYCOMMAND | dat->bShowAvatar ? MF_CHECKED : MF_UNCHECKED);
  ....
}

Этот код работает не так, как предполагает программист. Но, тем не менее, он работает правильно.

Условием тернарного оператора является не (dat->bShowAvatar), а выражение (MF_BYCOMMAND | dat->bShowAvatar). Благодаря везению, константа MF_BYCOMMAND равна нулю и никак не влияет на результат.

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

Поэтому не стоит рассматривать эту статью как руководство к действию. Она хорошо рекламирует мощь анализатора PVS-Studio, но весьма поверхностна, чтобы поправить описанные в ней ошибки и на этом успокоиться. Предлагаю разработчикам самим запустить PVS-Studio и внимательно изучить все предупреждения.

Заключение

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

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

Приглашаю незамедлительно скачать PVS-Studio и попробовать его на своём проекте.

Эта статья на английском

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Miranda NG Project to Get the «Wild Pointers» Award (Part 2).