Проект Miranda NG отримує приз «дикі покажчики» (частина друга)

Miranda NG
Продовжимо розглядати помилки, які вдалося виявити в проекті 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 "С" __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),©):0)
#define mir_strrchr(s,c) (((s)!=0)?strrchr((s),©):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;
віртуальний 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 greater than not 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).

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio і CppCat, версія 2014. Будь ласка, ознайомтеся зі списком.


Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.