ミランダNGプロジェクトがワイルドポインター賞を受賞（パート2）

PVS-Studio静的コードアナライザーを使用して、Miranda NGプロジェクトで見つかったエラーを引き続き検討します。 前回、ポインターとメモリの操作について話しました。 次に、主にずさんさとタイプミスに関連する一般的なエラーについて説明します。

検証を続ける

Miranda NGコードレビューの前の部分は、 ここから入手できます 。

タイプミス

このような美しいタイプミスから始めます。 「=」キーの隣には「-」キーがあります。 これにより、次のことが起こりました。

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

PVS-Studio警告： V559 'if'演算子の条件式内の疑わしい割り当て：順序=--1. NewAwaySys msgtree.cpp 677

当然、if（Order == -1）のように記述する必要があります。

そして、ここで彼らはアスタリスク「*」を忘れました：

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

PVS-Studioの警告 ： V501 '&&'演算子の左右に同じ副次式があります：dbv.ptszVal！= 0 && dbv.ptszVal！= '\ 0' SimpleStatusMsg msgbox.cpp 247

彼らは、ポインタがゼロ以外であり、文字列が空でないことを確認したかったのです。 しかし、ポインターの逆参照を忘れていました。 ゼロへの等価性へのポインターをダブルチェックしたことが判明しました。

正しいオプション：

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

このエラーは、別の診断を使用して検出されます： V528 「wchar_t」タイプへのポインターがL「\ 0」値と比較されるのは奇妙です。 おそらく次の意味：* dbv.ptszVal！= L '\ 0'。 SimpleStatusMsg msgbox.cpp 247

これは、2つまたは3つの診断で1つのエラーが示される一般的な状況です。 このエラーにより、いくつかの観点からコードが一度に疑わしいことがわかります。

さらにいくつかの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 'memcpy'関数の最初の引数は2番目の引数と同じです。 NimContact http.cpp 46

同様の状況がもう1つあります。

 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 'wcscpy'関数の最初の引数は2番目の引数と同じです。 Spamotron utils.cpp 218

文字列はそれ自体にコピーされます。 「dst」ポインターを引数の1つとして使用する必要があったと思われます。

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

警告PVS-Studio： V560条件式の一部は常に真です：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 'if'演算子の条件式内の疑わしい割り当て：k =15。NimContactcontactinfo.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所有者なしの表現 '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ループの条件「5」は常に真です。 Xfire main.cpp 1110

ループに何か問題があります。 「i」と「5」を比較するのを忘れていたと思います。 さらに、このサイクルはプログラムのもう1つの場所、variables.cpp 194にコピーされます。

不注意

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

V532「*ポインター++」パターンのステートメントの検査を検討してください。 おそらく意味：「（*ポインター）++」。 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 「b-> bPushed」変数には、連続して2回値が割り当てられます。 おそらくこれは間違いです。 行を確認してください：358、359。TopToolBar toolbar.cpp 359

最初は変数に値が設定されていたのに、突然別の値に変更されるのは奇妙です。

別の例：

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

V519「rc.left」変数には値が連続して2回割り当てられます。 おそらくこれは間違いです。 行を確認してください：583、585。Miranda hotkey_opts.cpp 585

もちろん、2つの異なる値を連続して1つの変数に書き込むことは、必ずしもエラーではありません。 念のため、変数がゼロに初期化されてから使用される場合があります。 他にも正しい状況があります。 しかし、私の意見では、不審なコードMirandaNG-519.txtを示す14個の警告を書きました。

警告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「m_proto-> m_options.IgnoreRosterGroups」変数には、連続して2回値が割り当てられます。 おそらくこれは間違いです。 行を確認してください：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 「then」ステートメントは「else」ステートメントと同等です。 TabSRMM msglog.cpp 439

おそらく、コードはCopy-Pasteを使用して作成されました。 同時に、行の1つを修正するのを忘れていました。

このような不審な場所はさらに9つあります ： MirandaNG-523.txt 。

この場所で何かが疲れた。 説明する必要のある豊富なエラーに疲れました。 私はすでに2番目の記事を書いていますが、警告には終わりと端が見えません。 コーヒーを飲みます。

（時間が経過しました）

それでは、続けましょう。 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 「？：」演算子は、その条件式に関係なく、常に1つの同じ値を返します：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 「alphafactor / 100」式は、暗黙的に「int」型から「float」型にキャストされました。 分数部分の損失を避けるために、明示的な型キャストの使用を検討してください。 例：double A =（double）（X）/ Y;。 TabSRMM themes.cpp 1753

私は、プログラマーが「alphafactor / 100」の除算が整数ではないことを望んでいたのではないかと疑っています。 ここで、BYTE型の変数を100で除算すると、0、1、2の3つの値しか取得できないことがわかります。

おそらく次のように分割する必要があります。

 fr *= (alphafactor / 100.0 * 2.2); 

同じファイルで、1758行と1763行にさらに2つの不審な除算操作があります。

WTF？

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

PVS-Studio警告： V622 「switch」ステートメントの検査を検討してください。 最初の「ケース」演算子が欠落している可能性があります。 UInfoEx ctrl_contact.cpp 188

「case IDOK」の前の「if（HIWORD（wParam）== BN_CLICKED）{」という行は何ですか？ 彼女はコントロールを得ることができません。 それによってプログラマは何を意味しましたか？

別のそのような場所は少し下です（行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 「else」ブロックが忘れられているかコメントアウトされている可能性があり、そのためプログラムの操作ロジックが変更されています。 LinkList linklist_fct.cpp 92

奇妙な「その他」に注意してください。

これは次のようなものです。

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

PVS-Studio警告：V640コードの操作ロジックはそのフォーマットに対応していません。 2番目のステートメントは常に実行されます。 中括弧が欠落している可能性があります。 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ループ内の無条件の「ブレーク」。 ポップアップskin.cpp 807

ループの途中で「ブレーク」が必要なのはなぜですか？ リファクタリングの失敗の結果は？ 残念ながら、唯一のものではありません：

• 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式 'cp！= "\ 005"'は常にtrueです。 文字列を比較するには、strcmp（）関数を使用する必要があります。 Yahoo libyahoo2.cpp 4486

しかし、ジャンルの古典に戻って。 例は1つだけで、残りの警告は通常どおりリストになります。

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

PVS-Studio警告：V547式 'dis-> itemData> = 0'は常にtrueです。 符号なしの型の値は常に> = 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「strrchr」関数は値「10875」を処理します。 2番目の引数を調べます。 UInfoEx classeximcontactbase.cpp 177
• V575「strchr」関数は値「32042」を処理します。 2番目の引数を調べます。 UInfoEx classeximcontactbase.cpp 177

どうやら、誰かが文字「* {」と「} *」で囲まれたテキストを見つけたいと思っていたようです。 しかし、それはある種の愚かさを明らかにしました。

まず、strchr（）およびstrrchr（）関数は、サブストリングではなく単一の文字を探します。

次に、「* {」は番号10875として解釈されます。関数は、タイプ「int」の値を2番目の引数として期待しますが、これは何も意味しません。 これらは、この引数の下位バイトのみを使用します。

残念ながら、これは偶然ではなく、体系的なエラーです。

同じ誤った呼び出しが10個あります： MirandaNG-575.txt 。

未定義の動作

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

警告PVS-Studio： V567未定義の動作。 'i'変数は、シーケンスポイント間で2回使用されている間に変更されます。 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条件（（end + 1）！= '\ 0'）は、とにかく未定義の動作であるポインターオーバーフローがある場合にのみfalseです。 DbEditorPP exportimport.cpp 425

一般的に、ここにありふれたタイプミスがあります。 「終了」ポインタが終端ゼロの前の文字を指していることを確認したかったのです。 エラーは、ポインターの逆参照を忘れたことです。 それは書かれるべきです：

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

そして、不定の動作はどこにありますか？ これについて説明します。

一般に、このエラーは他の診断でも診断されます（ V528 ）。 しかし、私はあいまいな行動について話すことに興味があります。 アナライザーが何か不明瞭なことを言ったとしても、急いではいけませんが、コードで彼を混乱させると考える必要があります。

したがって、ポインターに1を追加すると、常にNULL以外の値が取得されます。 1つの場合を除き、オーバーフローが発生すると、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未定義の動作。 シフト演算子 '<<を確認してください。 左のオペランド '（-1）'は負です。 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 「CNetClient」クラスには仮想関数が含まれていますが、仮想デストラクタは存在しません。 YAMN pop3.h 23

結果は明らかだと思います。 仮想デストラクタに関する質問は、2回目のインタビューのたびに聞かれます。

同様に、次のクラスでは物事は良くありません。

• Cupdprogress
• FactoryBase
• ContactCompareBase

誤った文字列フォーマット

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

警告PVS-Studio： V576の形式が正しくありません 。 'sprintf'関数の3番目の実引数を確認することを検討してください。 引数は32ビット以下であると予想されます。 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 「if（A）{...} else if（A）{...}」パターンの使用が検出されました。 論理エラーが存在する可能性があります。 行を確認してください：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 「memset」関数は「0」要素を処理します。 3番目の引数を調べます。 PluginUpdater dlgupdate.cpp 652

2番目の引数がゼロであるため、関数は何もリセットしません。 別のこのような間違った呼び出し：shlipc.cpp 68。

再確認：

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

PVS-Studio警告： V501 「&&」演算子の左側と右側に同一の副次式「job-> hContact」があります。 TabSRMM hotkeyhandler.cpp 523

'job-> hContact'の2番目のチェックは単に不要であり、削除できるように思えます。 それでも、この場所とこれらを確認することをお勧めします。

• 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同じリソースの割り当てを解除するために、 'DeleteObject'関数が2回呼び出されます。 行を確認してください：264、273。UInfoEx svc_flagsicons.cpp 273

記事に含まれていないもの

私にはもう力がありません。 私はあまりにも怠describeだったので、あまり重要ではありません。 さて、例えば、このように：

 #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を自分で起動し、すべての警告を注意深く検討することをお勧めします。

おわりに

静的コード分析がどれほど役立つかをもう一度示すことができたと思います。 1回限りのチェックでも多くのエラーが明らかになりますが、これは静的アナライザーを使用する場合の誤ったシナリオです。

分析は定期的に実行する必要があり、その後、多くのエラーが最も早い段階で発見されます。 これにより、検索と削除にかかる時間が大幅に短縮されます。

PVS-Studioをすぐにダウンロードして、プロジェクトで試してみてください。

この記事は英語です。

この記事を英語圏の聴衆と共有したい場合は、翻訳へのリンクを使用してください：Andrey Karpov。 「ワイルドポインター」賞を獲得するミランダNGプロジェクト（パート2）