Geekly Articles each Day

PVS-Studioを使用してVisual C ++ 2017ライブラリを改善する方法

この記事のタイトルは、Visual Studio開発者がPVS-Studio静的コードアナライザーを使用することでメリットが得られることを示唆しています。 この記事では、Visual C ++ 2017の最近リリースされたバージョンの一部であるライブラリの分析結果を示し、エラーを改善および排除するための推奨事項を示します。 Visual C ++ライブラリの開発者がどのように脚を撃ったかを学ぶために読者を招待します。それは興味深く有益です。

少しの背景


これは、Visual C ++に含まれるライブラリをチェックする最初の実験ではありません。 ここで以前のチェックについて読むことができます：


これらの出版物の後、長い休憩がありました、そして、私はVS2015に関する記事を書きませんでした。 検証を待っている多くの興味深いプロジェクトがありました。 正直なところ、私はこのトピックに関する記事を書くのを忘れていました。 幸いなことに、Visual C ++開発者の1人（ @MalwareMinigun ）からのツイートは、 VS2017を思い出させてくれました。

標準のライブラリヘッダーであなたが見つけたものをいつも叫んでくれる人がいないことに驚いています。

写真1


確かに、私はVisual Studio 2017ライブラリのバグについて世界に語ったことはありませんでした！ さて、電話は受け入れられました！

ご覧のとおり、ツイート（3月31日）から1か月が経過しています。 回答を遅らせたことは認めますが、今は修正します。

確認された内容と方法


検証のために、入手可能な最新バージョンのPVS-Studioアナライザー（6.15）を使用しました。

Visual Studio 2017の最近リリースされたバージョンの一部であるC ++ライブラリを確認しました。2017年4月12日、コンピューター上にあったライブラリのバージョンを確認しました。 ただし、このテキストはバグレポートではなく、静的分析の一般的な方法論、特にPVS-Studioアナライザーを一般的に紹介する記事であるため、バージョンはそれほど重要ではありません。

私は難しいので、完全なライブラリチェックを気にしませんでした。

まず、すべてのライブラリを別のフォルダーにコピーする必要がありました。そうしないと、それらのライブラリーに関する警告を取得できませんでした。 アナライザは、システムライブラリにメッセージを発行しません。 ファイルを別のフォルダーにコピーすると、PVS-Studioをだまして、必要な警告が表示されます。

ちなみに、上記のツイートに対する回答はこちらです。 このため、Visual C ++ユーザーは、ライブラリに関するPVS-Studioの警告については書きません。 彼らは人々をそらすだけなので、彼らを配るのは意味がありません。 同時に、インライン関数の本体を完全に解析および分析する必要がないため、分析の速度が少し向上します。

第二に、私は正直にプロジェクトを集めようとしませんでした。 新しいソリューションを作成し、そこにあるライブラリからファイルを追加しました。 その結果、いくつかのファイルを確認できませんでした。 しかし、記事を書くという観点からは、これは必須ではありません。 全文の資料であるため、かなり十分であることが判明しました。 Visual C ++開発者自身によって、より徹底的で正しいチェックが既に行われている必要があります。これを支援する準備ができています。

誤検知


今回は、残念ながら、具体的な数値と割合を示すことはできません。

私はそれを言うことができます：


ただし、これらの数値はいかなる方法でも解釈できず、それらから結論を引き出すことはできません！

ファイルの一部のみをチェックし、奇妙な方法でもチェックしたことを思い出させてください。 さらに、ライブラリには異常な瞬間があります。 アナライザーは、完全に正しいメッセージを多数生成しますが、それらは同時に完全に偽です。 このパラドックスの理由を説明します。

システムのデータ型を独立して宣言することは有害で危険です。 悪いアイデアの例：

typedef unsigned long DWORD;

PVS-Studioアナライザーは次の警告を生成します。標準の「DWORD」タイプのV677カスタム宣言。 システムヘッダーファイル#include <WinDef.h>を使用する必要があります。

アナライザーは完全に正しいです。 型を「手動で」宣言するのではなく、対応するヘッダーファイルを含める必要があります。

ご理解のとおり、このような診断はVisual C ++ライブラリには適用されません。 結局のところ、そのような型が宣言されるのはまさにそこです。 そして、アナライザーは250以上のそのようなメッセージを発行しました。

または、もう1つの興味深い例です。 PVS-Studioアナライザーは、 thisポインターがNULLであるかどうかをチェックするコードを批判するとき、絶対に正しいです。 最新のC ++言語標準によれば、 thisポインターをNULLにすることはできません。

Visual C ++にはこれに関する大きな問題があります。 どうやら、この点で、それは決して標準を満たしていないか、まもなく発生します。 実際には、ライブラリ（たとえば、MFC）はアーキテクチャ的に構築されているため、 これはNULLに等しいことが一般的な状況です。

ライブラリコードには、 thisポインターをチェックする多数の関数が含まれています。 いくつかの機能を次に示します。

 _AFXWIN_INLINE CDC::operator HDC() const { return this == NULL ? NULL : m_hDC; } _AFXWIN_INLINE HDC CDC::GetSafeHdc() const { return this == NULL ? NULL : m_hDC; }

PVS-Studioアナライザーは、当然それらに対して警告を発行します。


このようなメッセージは40以上ありますが、もちろん意味はありません。 今後数年間で、それらは誤検知とみなされる可能性があります。

メッセージV677およびV704の例でわかるように、すべての診断がVisual C ++ライブラリに適用できるわけではありません。 もちろん、問題はありません。これらの警告やその他の警告をオフにするだけで、メッセージの数をすぐに300個減らすことができます。

アナライザーを事前に構成していない場合、誤検知の割合を議論することはしばしば無意味であることを再度実証するために、このすべてを書いています。

一般的には、今回は興味がなく、おIび申し上げます。 私の主観的な意見では、誤検知はほとんどありません。

アナライザーは興味深いものを見つけました


私は無害から恐ろしいことに行くことにしました。 初めに、マイナーな改善のための推奨事項を検討します。 それから私は大胆不敵な過ちに進み、私の意見では「ホラー」と呼ぶことができるもので物語を終えます。 一般的に、次第に学位を上げていきます。 バグからプログラムの世界を救いなさい！

写真5



ミクロ最適化


アナライザーは、多くのマイクロ最適化を提供します。 つまり、この章で説明することはすべてエラーではありませんが、コードを少し改善することができます。

警告V808から始めましょう。これは、オブジェクトは作成されているが使用されていないことを示しています。 例として2つの関数を使用して、この状況を考慮してください。

 void CMFCToolBarComboBoxButton::AdjustRect() { .... if (m_pWndEdit != NULL) { CRect rectEdit = m_rect; const int iBorderOffset = 3; m_pWndEdit->SetWindowPos( NULL, m_rect.left + nHorzMargin + iBorderOffset, m_rect.top + iBorderOffset, m_rect.Width() - 2 * nHorzMargin - m_rectButton.Width() - iBorderOffset - 3, m_rectCombo.Height() - 2 * iBorderOffset, SWP_NOZORDER | SWP_NOACTIVATE); } .... }

PVS-Studio警告：「CRect」タイプのV808 「rectEdit」オブジェクトが作成されましたが、使用されませんでした。 afxtoolbarcomboboxbutton.cpp 607

作成および初期化後のrectEditオブジェクトはどこでも使用されません。 これは単に不要であり、安全に削除できます。 コードは少し短くなります。

別のケース：

 BOOL CALLBACK AFX_EXPORT CMFCToolBarFontComboBox::EnumFamPrinterCallBackEx(....) { .... CString strName = pelf->elfLogFont.lfFaceName; pCombo->AddFont((ENUMLOGFONT*)pelf, FontType, CString(pelf->elfScript)); return 1; }

「CStringT」タイプのV808 「strName」オブジェクトが作成されましたが、使用されませんでした。 afxtoolbarfontcombobox.cpp 138

CString型のオブジェクトが作成および初期化されますが、どこでも使用されません。 コンパイラが、文字列を作成してコピーするために余分なコードを捨てることを推測するかどうかはわかりません。 CStirngは複雑なクラスであるため、 不可能な場合があります。 ただし、これは重要ではありません。いずれにしても、 strNameオブジェクトを削除して、コードを簡単にする必要があります。

そして、そのような余分なオブジェクトがたくさんあります。 すでに検討されているものに加えて、アナライザーはさらに50のメッセージを発行しました。 記事のテキストを煩雑にしないために、これらのメッセージを別のファイルvs2017_V808.txtに書きました 。

今度は、追加のチェックの時間です。

 TaskStack::~TaskStack() { if (m_pStack) delete [] m_pStack; }

PVS-Studio警告： V809ポインター値がNULLでないことを確認する必要はありません。 「if（m_pStack）」チェックは削除できます。 taskcollection.cpp 29

delete演算子は、 nullptrを入力として完全に安全です。 したがって、チェックは不要であり、コードを簡素化できます。

 TaskStack::~TaskStack() { delete [] m_pStack; }

このような追加のチェックがたくさんあります。 vs2017_V809.txtファイルに68のメッセージを書き込みました 。

変更できる次の小さなことは、イテレータの接尾辞の増分を接頭辞の1に置き換えることです。 例：

 size_type count(const key_type& _Keyval) const { size_type _Count = 0; const_iterator _It = _Find(_Keyval); for (;_It != end() && !this->_M_comparator(....); _It++) { _Count++; } return _Count; }

警告PVS-Studio： V803パフォーマンスの低下。 '_It'がイテレータの場合、プレフィックス形式のインクリメントを使用する方が効果的です。 イテレータ++を++イテレータに置き換えます。 internal_concurrent_hash.h 509

次のように書くと、少し良くなります。

 for (;_It != end() && !this->_M_comparator(....); ++_It)

このようなリファクタリングに利点があるかどうかの質問については、記事で次のように検討しました。 回答：ありますが、大きなものではありません。

ライブラリの作成者が修正を行う価値があると判断した場合、さらに26個の同様の警告： vs2017_V803.txtがあります。

次の最適化。 _T（ ""）値を割り当てるよりも、 str.Empty（）関数を呼び出して文字列をクリアすることをお勧めします。 クラスは、回線に割り当てるメモリ量を事前に知りません。 したがって、文字列の長さを無駄に数え始めます。 一般的に、不必要なアクション。

 CString m_strRegSection; CFullScreenImpl::CFullScreenImpl(CFrameImpl* pFrameImpl) { m_pImpl = pFrameImpl; m_pwndFullScreenBar = NULL; m_bFullScreen = FALSE; m_bShowMenu = TRUE; m_bTabsArea = TRUE; m_uiFullScreenID = (UINT)-1; m_strRegSection = _T(""); }

警告PVS-Studio： V815パフォーマンスの低下。 式「m_strRegSection = L ""」を「m_strRegSection.Empty（）」に置き換えることを検討してください。 afxfullscreenimpl.cpp 52

ラインを交換する方が良い

 m_strRegSection = _T("");

に

 m_strRegSection.Empty();

些細なことですが、完璧主義者であれば、このようなコードの改善に満足するでしょう。

ご注意 実際、このコードはコンストラクター内にあり、行はすでに空なので、この行は完全に削除できます。

このトピックに関するさらに27の警告： vs2017_V815.txtです。

そして最後に、コードは次のとおりです。

 HRESULT GetPropertyInfo(....) { .... for(ul=0; ul<m_cPropSetDex; ul++) { .... for(ULONG ulProp=0; ....) { .... pDescBuffer += (wcslen(L"UNKNOWN") + 1); .... }

警告PVS-Studio： V814パフォーマンスの低下。 'wcslen'関数は、ループの本体内で複数回呼び出されました。 atldb.h 2374

wcslen関数はネストされたループ内にあるため、複数回呼び出されることに注意してください。 事前に計算し、文字列L "UNKNOWN"の長さを覚えておくことは、より論理的です。

別のメッセージ： V814パフォーマンスの低下。 'wcslen'関数は、ループの本体内で複数回呼び出されました。 atldb.h 2438

推奨事項がすべて完了しました。 さらに興味深いものに移りましょう。

中小口径エラー


コンパイラの警告は、ヘッダーファイルで誤って抑制されます。 アラートを一時的に無効にする間違った方法を検討してください。

 #ifdef _MSC_VER #pragma warning(disable:4200) #endif typedef struct adpcmwaveformat_tag { WAVEFORMATEX wfx; WORD wSamplesPerBlock; WORD wNumCoef; #if defined( _MSC_VER ) ADPCMCOEFSET aCoef[]; #else ADPCMCOEFSET aCoef[1]; #endif } ADPCMWAVEFORMAT; typedef ADPCMWAVEFORMAT *PADPCMWAVEFORMAT; typedef ADPCMWAVEFORMAT NEAR *NPADPCMWAVEFORMAT; typedef ADPCMWAVEFORMAT FAR *LPADPCMWAVEFORMAT; #ifdef _MSC_VER #pragma warning(default:4200) #endif

PVS-Studio警告： V665このコンテキストでは、「＃pragma warning（default：X）」の使用が間違っている可能性があります。 代わりに「#pragma warning（push / pop）」を使用する必要があります。 チェックライン：2610、2628。mmreg.h 2628

エラー自体が何であるかを見るのは簡単ではないことを理解しているので、本質を強調します。

 #pragma warning(disable:4200) .... #pragma warning(default:4200)

まず、コンパイラの警告がオフになります。 次に、アラートステータス4200がデフォルト値に設定されます。 これはできません。 ユーザーがファイルの1つに対して4200診断を完全に無効にしたとします。 残念ながら、彼はファイルに書いた：

 #include <mmreg.h>

その結果、警告が再びオンになり、ユーザーコードに対して発行されます。

正しい解決策は、状態を保存してから前の状態を返すことです。

 #pragma warning(push) #pragma warning(disable:4200) .... #pragma warning(pop)

ヘッダーファイルでプラグマ警告が誤って使用される場所を次に示します。


* .cppファイルにもこのようなエラーがありますが、Visual C ++ユーザーコードに危険を及ぼさないため、書きませんでした。 もちろん、それらを修正することも役立ちます。

次に、 新しい演算子について説明します。

 inline HRESULT CreatePhraseFromWordArray(....) { .... SPPHRASEELEMENT *pPhraseElement = new SPPHRASEELEMENT[cWords]; if(pPhraseElement == NULL) { ::CoTaskMemFree(pStringPtrArray); return E_OUTOFMEMORY; } memset(pPhraseElement, 0, sizeof(SPPHRASEELEMENT) * cWords); .... }

PVS-Studio警告： V668メモリは「new」演算子を使用して割り当てられたため、「pPhraseElement」ポインターをnullに対してテストする意味はありません。 メモリ割り当てエラーの場合、例外が生成されます。 sphelper.h 2973

正式には、このコードは正しくありません。 メモリ割り当てエラーが発生した場合、 newオペレーターは例外をスローする必要があります。 その結果、 ifステートメントの本体の内部には入らず、 CoTaskMemFree関数は呼び出されません。 そして、一般的に、プログラムはプログラマーの期待どおりに動作し始めません。

これが本当の間違いかどうかはわかりません。 プロジェクトはnothrownew.objにリンクされている可能性があります。 この場合、 new演算子は例外をスローしません。 このような機会は、たとえば、ドライバー開発者によって使用されます。 詳細： newおよびdelete演算子 。 したがって、これらが誤検知である場合、警告V668をオフにすることができます。

しかし、別のシナリオが可能です。 このコードは、エラーが発生したときにnew演算子がNULL値を返した古代から残っています。 その後、アナライザーは112個のそのような警告を発行したため、すべてが悪いです： vs2017_V668.txt 。

続けましょう。 アナライザーはV730に多くの警告を出します。これは、すべてのメンバーがコンストラクターで初期化されるわけではないことを示しています。 警告の本質を2つの例で説明します。

まず、CMFCScanlinerクラスを検討します。 次のメンバーが宣言されています：

 class CMFCScanliner { .... private: LPBYTE m_line; LPBYTE m_line_begin; LPBYTE m_line_end; size_t m_pitch; DWORD m_start_row; DWORD m_start_col; DWORD m_rows; DWORD m_cols; long m_offset; BYTE m_channels; size_t m_height; };

コンストラクターを見てみましょう：

 CMFCScanliner() { empty(); }

実際、コンストラクターには注意する必要はありません。 次に、 空の関数を調べます。

 void empty() { m_line = NULL; m_pitch = 0; m_start_row = 0; m_start_col = 0; m_rows = 0; m_cols = 0; m_offset = 0; m_height = 0; m_line_begin = NULL; m_line_end = NULL; }

PVS-Studio警告： V730クラスのすべてのメンバーがコンストラクター内で初期化されるとは限りません。 検査を検討してください：m_channels。 afxtoolbarimages.cpp 510

m_channelsを除くすべてのメンバーを初期化しました 。 同意します、これは奇妙なことです。なぜなら、このメンバーは他のメンバーより際立っていないからです。 これは間違いに非常に似ていますが、問題のクラスの原則に精通していないため、最後まで確信が持てません。

次に、構造AFX_EVENTを検討します。

 struct AFX_EVENT { enum { event, propRequest, propChanged, propDSCNotify }; AFX_EVENT(int eventKind); AFX_EVENT(int eventKind, DISPID dispid, ....); int m_eventKind; DISPID m_dispid; DISPPARAMS* m_pDispParams; EXCEPINFO* m_pExcepInfo; UINT* m_puArgError; BOOL m_bPropChanged; HRESULT m_hResult; DSCSTATE m_nDSCState; DSCREASON m_nDSCReason; }; AFX_INLINE AFX_EVENT::AFX_EVENT(int eventKind) { m_eventKind = eventKind; m_dispid = DISPID_UNKNOWN; m_pDispParams = NULL; m_pExcepInfo = NULL; m_puArgError = NULL; m_hResult = NOERROR; m_nDSCState = dscNoState; m_nDSCReason = dscNoReason; }

PVS-Studio警告： V730クラスのすべてのメンバーがコンストラクター内で初期化されるわけではありません。 検査を検討してください：m_bPropChanged。 afxpriv2.h 104

今回は、変数m_bPropChangedは初期化されていません。

どちらの場合も、これらの変数を初期化する必要があるかどうかを判断することは想定していません。 そのため、PVS-Studioアナライザーが注意を引くこれらのケースやその他の疑わしいケースを研究することを開発者に勧めます。 vs2017_V730.txtファイルに別の183アナライザー警告を書きました。 確かにこれらのメッセージの中には、真に重大なエラーを示すものがいくつかあります。 メンバーを確実に初期化する必要があると確信した場合、これらの警告はすべてこの記事の次の章に帰することにします。 初期化されていない変数は、エラーがまれに不規則に発生する可能性があるため、非常に潜行的です。

次のアナライザー警告は、無意味なチェック用です。 このようなチェックは削除するか、他のチェックに置き換える必要があります。

 HRESULT SetDpiCompensatedEffectInput(....) { .... hr = deviceContext->CreateEffect(CLSID_D2D1DpiCompensation, &dpiCompensationEffect); if (SUCCEEDED(hr)) { if (SUCCEEDED(hr)) { .... }

PVS-Studio警告： V571繰り返しチェック。 'if（（（HRESULT）（hr））> = 0）'条件は、881行目で既に検証されています。d2d1_1helper.h 883

変数hrの値は、連続して2回チェックされます。 余分なコードを扱っているか、何らかのタイプミスであり、2番目の条件を変更する必要があります。

 void Append(_In_reads_(nLength) PCXSTR pszSrc, _In_ int nLength) { // See comment in SetString() about why we do this UINT_PTR nOffset = pszSrc-GetString(); UINT nOldLength = GetLength(); if (nOldLength < 0) { // protects from underflow nOldLength = 0; } .... }

PVS-Studio 警告 ： V547式 'nOldLength <0'は常にfalseです。 符号なしの型の値が<0になることはありませんatlsimpstr.h 392

変数nOldLengthは符号なしの型UINTであり、ゼロより小さくすることはできません。

次に、 FreeLibrary関数について説明します 。

 extern "C" BOOL WINAPI DllMain(HINSTANCE hInstance, DWORD dwReason, LPVOID) { .... ::FreeLibrary(pState->m_appLangDLL); .... }

PVS-Studio警告： V718 「DllMain」関数から「FreeLibrary」関数を呼び出さないでください。 dllinit.cpp 639

FreeLibrary関数についてMSDNが言っていることは次のとおりです。DllMainからFreeLibraryを呼び出すのは安全ではありません。 詳細については、 DllMainの 「解説」セクションを参照してください。

運のおかげで、コードは機能します。 ただし、コードはまだ悪いので注意が必要です。

この章の終わりに、このテンプレート関数に注意を払いたいと思います。

 template<class _FwdIt> string_type transform_primary(_FwdIt _First, _FwdIt _Last) const { // apply locale-specific case-insensitive transformation string_type _Res; if (_First != _Last) { // non-empty string, transform it vector<_Elem> _Temp(_First, _Last); _Getctype()->tolower(&*_Temp.begin(), &*_Temp.begin() + _Temp.size()); _Res = _Getcoll()->transform(&*_Temp.begin(), &*_Temp.begin() + _Temp.size()); } return (_Res); }

PVS-Studio警告： V530関数「tolower」の戻り値を使用する必要があります。 正規表現319

このコードを見たのはこれが初めてで、ナビゲートするのが難しいです。 したがって、 tolower関数呼び出しを指すときにアナライザーが正しいかどうかはわかりません。 通常、 tolower関数の結果を使用する必要がありますが、どの特定のtolower関数がここで呼び出されるのかわかりません。 したがって、私は単にこのコードにライブラリ開発者の注意を引き、確認するようにお願いします。

ハードコア


ここで、私の意見では、最も興味深い部分が始まります。

写真7
 _AFXCMN_INLINE int CToolBarCtrl::GetString( _In_ int nString, _Out_writes_to_(cchMaxLen, return + 1) LPTSTR lpstrString, _In_ size_t cchMaxLen) const { ASSERT(::IsWindow(m_hWnd)); return (int) ::SendMessage(m_hWnd, ...., (LPARAM)lpstrString); lpstrString[cchMaxLen]=_T('\0'); }

PVS-Studio警告： V779到達不能コードが検出されました。 エラーが存在する可能性があります。 afxcmn2.inl 111

明示的なエラー：関数の最後の行は実行されません。

次のコードフラグメントには、ループ本体内のreturnステートメントへの非常に疑わしい呼び出しが含まれています。

 HRESULT GetIndexOfPropertyInSet( _In_ const GUID* pPropSet, _In_ DBPROPID dwPropertyId, _Out_ ULONG* piCurPropId, _Out_ ULONG* piCurSet) { HRESULT hr = GetIndexofPropSet(pPropSet, piCurSet); if (hr == S_FALSE) return hr; UPROPINFO* pUPropInfo = m_pUPropSet[*piCurSet].pUPropInfo; for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++) { if( dwPropertyId == pUPropInfo[ul].dwPropId ) *piCurPropId = ul; return S_OK; } return S_FALSE; }

PVS-Studio警告： V612ループ内の無条件の「戻り」。 atldb.h 4837

ループを作成する必要がある理由は明らかではありませんが、すべて同じ場合、このループでは複数回の反復を実行できません。 コードを簡素化できます。 しかし、ここではコードを単純化するのではなく、エラーを修正する必要があるように思えます。 ここで中括弧が忘れられているのではないかと疑っています。実際、関数は次のようになります。

 HRESULT GetIndexOfPropertyInSet( _In_ const GUID* pPropSet, _In_ DBPROPID dwPropertyId, _Out_ ULONG* piCurPropId, _Out_ ULONG* piCurSet) { HRESULT hr = GetIndexofPropSet(pPropSet, piCurSet); if (hr == S_FALSE) return hr; UPROPINFO* pUPropInfo = m_pUPropSet[*piCurSet].pUPropInfo; for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++) { if( dwPropertyId == pUPropInfo[ul].dwPropId ) { *piCurPropId = ul; return S_OK; } } return S_FALSE; }

検討中のループに加えて、常にサイクルを中断するいくつかのbreakステートメントに注意する価値があります。


次に、コピーペーストについて説明します。 大規模なプロジェクトを作成したり、テキストブロックのコピーや編集に関連する間違いを犯したりすることはできません。

ところで、冒頭で、この記事をさらに読むことなく、自分でエラーを見つけようとすることを提案します。

 void CPaneContainerManager::RemoveAllPanesAndPaneDividers() { ASSERT_VALID(this); POSITION pos = NULL; for (pos = m_lstControlBars.GetHeadPosition(); pos != NULL;) { POSITION posSave = pos; CBasePane* pWnd = DYNAMIC_DOWNCAST( CBasePane, m_lstControlBars.GetNext(pos)); ASSERT_VALID(pWnd); if (pWnd->IsPaneVisible()) { m_lstControlBars.RemoveAt(posSave); } } for (pos = m_lstSliders.GetHeadPosition(); pos != NULL;) { POSITION posSave = pos; CBasePane* pWnd = DYNAMIC_DOWNCAST( CBasePane, m_lstControlBars.GetNext(pos)); ASSERT_VALID(pWnd); if (pWnd->IsPaneVisible()) { m_lstSliders.RemoveAt(posSave); } } }

写真3


何が間違っているのか見てください。

多くの人はあきらめて、この記事をさらに読むことにしたと思います。 これは、静的アナライザーが重要で必要な理由の良い例です。それらは怠notではなく、疲れません。

PVS-Studio警告： V778同様の2つのコードフラグメントが見つかりました。 おそらく、これはタイプミスであり、「m_lstControlBars」の代わりに「m_lstSliders」変数を使用する必要があります。 afxpanecontainermanager.cpp 1645

ただし、アナライザーの警告を読んだ後でも、コード内のエラーを見つけるのは簡単ではありません。 したがって、私はそれを減らし、重要なものを残します：

 for (... m_lstControlBars ...) { CBasePane* pWnd = ... m_lstControlBars ... m_lstControlBars.RemoveAt(); } for (... m_lstSliders ...) { CBasePane* pWnd = ... m_lstControlBars ... m_lstSliders.RemoveAt(); }

最初のループではm_lstControlBarsコンテナーが処理され、2番目のループではm_lstSlidersコンテナーが処理されます。

99％の確率で、2番目のサイクルはCopy-Pasteテクノロジーを使用して記述されました。最初のサイクルが取得、コピーされ、 m_lstControlBarsという名前がm_lstSlidersに置き換えられました 。 しかし、ある場所では名前を置き換えるのを忘れていました！

エラーは次のとおりです。CBasePane* pWnd = ... m_lstControlBars ...

それは良い間違いでしたが、次は美しさで彼女に劣っていません。 CMFCScanlinerクラスでのインクリメント/デクリメント演算子の実装方法を検討してください。

 class CMFCScanliner { .... inline const CMFCScanliner& operator ++ () { m_line += m_offset; return *this; } inline const CMFCScanliner& operator ++ (int) { m_line += m_offset; return *this; } inline const CMFCScanliner& operator -- () { m_line -= m_offset; return *this; } inline const CMFCScanliner& operator -- (int) { m_line += m_offset; return *this; } .... };

PVS-Studio警告： V524 「-」関数の本体が「++」関数の本体と完全に同等であることは奇妙です。 afxtoolbarimages.cpp 656

最新のステートメントの実装に注意してください。 + =から-=への変更を忘れました。 これは古典的です！ これが実際の「 最後の行効果 」です！

リークが発生する可能性のある場所は3つありました。 それらの1つを次に示します。

 CSpinButtonCtrl* CMFCPropertyGridProperty::CreateSpinControl( CRect rectSpin) { ASSERT_VALID(this); ASSERT_VALID(m_pWndList); CSpinButtonCtrl* pWndSpin = new CMFCSpinButtonCtrl; if (!pWndSpin->Create(WS_CHILD | WS_VISIBLE | UDS_ARROWKEYS | UDS_SETBUDDYINT | UDS_NOTHOUSANDS, rectSpin, m_pWndList, AFX_PROPLIST_ID_INPLACE)) { return NULL; } .... }

PVS-Studio警告： V773 「pWndSpin」ポインターを解放せずに関数が終了しました。 メモリリークが発生する可能性があります。 afxpropertygridctrl.cpp 1490

Create関数がエラーで実行された場合、 pWndSpin変数にポインターが保存されているオブジェクトは削除されません。

同様に：


C ++言語標準によれば、void *型のポインターで削除演算子を呼び出すと、未定義の動作が発生します。そして、読者がすでに推測したように、これはVisual C ++ライブラリで起こります。

 typedef void *PVOID; typedef PVOID PSECURITY_DESCRIPTOR; class CSecurityDescriptor { .... PSECURITY_DESCRIPTOR m_pSD; .... }; inline CSecurityDescriptor::~CSecurityDescriptor() { delete m_pSD; // <=  m_pSD   void * free(m_pOwner); free(m_pGroup); free(m_pDACL); free(m_pSACL); }

PVS-Studio警告：V772は、無効なポインターに対して「削除」演算子を呼び出すと、未定義の動作を引き起こします。atlcom.h 1039

同様の問題はここにあります：


機能CMFCReBar :: CalcFixedLayoutは、パラメータの取りbStretchを、それを使用していません。むしろ、最初の使用の前に、bStretchは明示的に1を記述します。

 CSize CMFCReBar::CalcFixedLayout(BOOL bStretch, BOOL bHorz) { ASSERT_VALID(this); ENSURE(::IsWindow(m_hWnd)); // the union of the band rectangles is the total bounding rect int nCount = (int) DefWindowProc(RB_GETBANDCOUNT, 0, 0); REBARBANDINFO rbBand; rbBand.cbSize = m_nReBarBandInfoSize; int nTemp; // sync up hidden state of the bands for (nTemp = nCount; nTemp--; ) { rbBand.fMask = RBBIM_CHILD|RBBIM_STYLE; VERIFY(DefWindowProc(RB_GETBANDINFO, nTemp, (LPARAM)&rbBand)); CPane* pBar = DYNAMIC_DOWNCAST( CPane, CWnd::FromHandlePermanent(rbBand.hwndChild)); BOOL bWindowVisible; if (pBar != NULL) bWindowVisible = pBar->IsVisible(); else bWindowVisible = (::GetWindowLong( rbBand.hwndChild, GWL_STYLE) & WS_VISIBLE) != 0; BOOL bBandVisible = (rbBand.fStyle & RBBS_HIDDEN) == 0; if (bWindowVisible != bBandVisible) VERIFY(DefWindowProc(RB_SHOWBAND, nTemp, bWindowVisible)); } // determine bounding rect of all visible bands CRect rectBound; rectBound.SetRectEmpty(); for (nTemp = nCount; nTemp--; ) { rbBand.fMask = RBBIM_STYLE; VERIFY(DefWindowProc(RB_GETBANDINFO, nTemp, (LPARAM)&rbBand)); if ((rbBand.fStyle & RBBS_HIDDEN) == 0) { CRect rect; VERIFY(DefWindowProc(RB_GETRECT, nTemp, (LPARAM)&rect)); rectBound |= rect; } } // add borders as part of bounding rect if (!rectBound.IsRectEmpty()) { CRect rect; rect.SetRectEmpty(); CalcInsideRect(rect, bHorz); rectBound.right -= rect.Width(); rectBound.bottom -= rect.Height(); } bStretch = 1; return CSize((bHorz && bStretch) ? 32767 : rectBound.Width(), (!bHorz && bStretch) ? 32767 : rectBound.Height()); }

PVS-Studio警告：V763パラメーター 'bStretch'は、使用される前に常に関数本体で書き換えられます。 afxrebar.cpp 209

「bStretch = 1;」という行は、誰かがデバッグ用に作成して、削除するのを忘れたように見えます。おそらくこれがまさに起こったことです。開発者にこの奇妙なコードをチェックして対処するように依頼します。AdjustDockingLayout

関数がCBasePaneおよびCDockSiteクラスでどのように宣言されているかを検討してください。

 class CBasePane : public CWnd { .... virtual void AdjustDockingLayout(HDWP hdwp = NULL); .... }; class CDockSite : public CBasePane { .... virtual void AdjustDockingLayout(); .... };

PVS-Studio警告：V762仮想機能が誤ってオーバーライドされた可能性があります。派生クラス「CDockSite」および基本クラス「CBasePane」の関数「AdjustDockingLayout」の最初の引数を参照してください。afxdocksite.h 94

基本クラスのある時点で、hdwp引数が関数宣言に追加され、子孫クラスで忘れられたようです。その結果、これらは2つの異なる機能になりました。

同様の状況：


クラス内の関数について説明しているので、仮想デストラクタについて、またはそれらの不在について説明しましょう。まず、CAccessTokenクラスを見てください。

 class CAccessToken { .... mutable CRevert *m_pRevert; }; inline bool CAccessToken::ImpersonateLoggedOnUser() const throw(...) { .... delete m_pRevert; m_pRevert = _ATL_NEW CRevertToSelf; .... }

アナライザーは警告を生成します。V599「CRevert」クラスには仮想関数が含まれていますが、仮想デストラクターは存在しません。atlsecurity.h 5252

彼がそれをする理由を見てみましょう。我々は興味がメンバーであるm_pRevert型のオブジェクトへのポインタである、CRevert。クラスは多態的に使用されます。次のコード行を見ると、この結論に到達できます。

 m_pRevert = _ATL_NEW CRevertToSelf;

クラスCRevertToSelfはから継承CRevert。これらのクラスを今すぐ知りましょう：

 class CRevert { public: virtual bool Revert() throw() = 0; }; class CRevertToSelf : public CRevert { public: bool Revert() throw() { return 0 != ::RevertToSelf(); } };

このCRevertクラスには何が欠けていますか？仮想デストラクタがありません。

PVS-Studioアナライザーに新しい診断を追加するだけでなく、既存の診断を改善します。たとえば、最近V611診断は、メモリの割り当てと解放のプロセスが異なる機能にある場合にメモリを解放する問題を探すことを学びました。次に、これが実際にどのように機能するかを示します。

 template <class TAccessor> class CBulkRowset : public CRowset<TAccessor> { .... void SetRows(_In_ DBROWCOUNT nRows) throw() { if (nRows == 0) nRows = 10; if (nRows != m_nRows) { delete m_phRow; m_phRow = NULL; m_nRows = nRows; } } HRESULT BindFinished() throw() { m_nCurrentRows = 0; m_nCurrentRow = 0; m_hr = S_OK; if (m_phRow == NULL) { m_phRow = _ATL_NEW HROW[m_nRows]; if (m_phRow == NULL) return E_OUTOFMEMORY; } return S_OK; } .... HROW* m_phRow; .... }

PVS-Studio 警告：V611メモリは「new T []」演算子を使用して割り当てられましたが、「delete」演算子を使用して解放されました。このコードを調べることを検討してください。「delete [] m_phRow;」を使用することをお勧めします。atldbcli.h 5689 new []演算子を使用

して、BindFinished関数でメモリが割り当てられます。

 m_phRow = _ATL_NEW HROW[m_nRows];

delete演算子を使用してSetRows関数でメモリを解放します：

 delete m_phRow;

結果：未定義の動作。memset

関数の非常に疑わしい呼び出しについて説明しましょう。ただし、誤ったコードを見る前に、すべてが問題のないコードを見てください。正しいコードは次のとおりです。



 void CToolTipCtrl::FillInToolInfo(TOOLINFO& ti, ....) const { memset(&ti, 0, sizeof(AFX_OLDTOOLINFO)); ti.cbSize = sizeof(AFX_OLDTOOLINFO); .... }

これは典型的な状況です。memset関数を呼び出すと、構造体のすべてのメンバーがクリアされます（ゼロで埋められます）。次に、そのサイズが構造に書き込まれます。これはWinAPIの標準的な方法です。そのため、多くの関数は、それらが機能する構造のバージョン（形式）を見つけます。

上記のコードでは、すべてが論理的です。AFX_OLDTOOLINFO構造体のサイズが計算されます。次に、構造のこのサイズを使用してmemset関数を呼び出し、同じサイズが構造内に書き込まれます。

異常なコードを考えてみましょう：

 BOOL CControlBar::PreTranslateMessage(MSG* pMsg) { .... TOOLINFO ti; memset(&ti, 0, sizeof(AFX_OLDTOOLINFO)); ti.cbSize = sizeof(TOOLINFO); .... }

PVS-Studio警告：V512「memset」関数を呼び出すと、バッファー「＆ti」がアンダーフローします。barcore.cpp 384

構造体はTOOLINFO型です。書き込まれるTOOLINFO構造体のサイズです。ti.cbSize = sizeof（TOOLINFO）; 。

ただし、式sizeof（AFX_OLDTOOLINFO）を使用してリセット可能なバイト数が計算されるため、構造は完全にはリセットされず、部分的にのみリセットされます。

その結果、構造フィールドの一部は初期化されないままになります。memsetが構造全体を満たさない

別のケースがあります。

 GUID m_Id; void zInternalStart() { .... // Zero the activity id in case we end up logging the stop. ZeroMemory(&m_Id, sizeof(&m_Id)); .... }

PVS-Studio警告：V512「memset」関数を呼び出すと、バッファー「＆m_Id」のアンダーフローが発生します。 traceloggingactivity.h 656

構造のサイズではなく、ポインターのサイズが計算される場合の古典的なエラー。その結果、32ビットアプリケーションと64ビットアプリケーションのどちらがコンパイルされているかに応じて、最初の4バイトまたは8バイトのみがリセットされます。 GUIDの構造は16バイト（128ビット）です。

正しいオプション：

 ZeroMemory(&m_Id, sizeof(m_Id));

警告V595がないわけではありません。この診断はCおよびC ++プログラムで最も一般的なエラーの1つを検出するため、これは驚くことではありません。ただし、C＃ではすべてが完璧ではありません。

エラーの本質は、チェックされる前にポインターの逆参照が発生するという事実にあります。

この診断を使用して特定されたコードを検討してください。

 HRESULT CBasePane::get_accHelp(VARIANT varChild, BSTR *pszHelp) { if ((varChild.vt == VT_I4) && (varChild.lVal == CHILDID_SELF)) { *pszHelp = SysAllocString(L"ControlPane"); return S_OK; } if (((varChild.vt != VT_I4) && (varChild.lVal != CHILDID_SELF)) || (NULL == pszHelp)) { return E_INVALIDARG; } .... }

PVS-Studio警告：V595 nullpsrに対して検証される前に、「pszHelp」ポインターが使用されました。行をチェックしてください：1324、1328。afxbasepane.cpp 1324

関数が次のように呼び出された場合：

 VARIANT foo; foo.vt = VT_I4; foo.lVal = CHILDID_SELF; get_accHelp(foo, NULL);

この関数はE_INVALIDARGのステータスを返す必要がありますが、代わりにnullポインターが逆参照されます。

アナライザーは次のように推論しました。ポインターは逆参照されます。ただし、以下では、このNULLポインターが等しいかどうかのチェックがあります。そのようなチェックがあるので、どうやらポインターはヌルである可能性があります。ゼロの場合、トラブルが発生します。ええ、警告する必要があります！

私が言ったように、これは非常によくある間違いです。 Visual C ++ライブラリも例外ではありません。リファクタリングを待っているさらに17の場所があります：vs2017_V595.txt。

そして、FALSE定数とS_FALSE定数の混同に関連する最終エラーを考慮してください。

 BOOL CMFCRibbonPanel::OnSetAccData (long lVal) { .... if (nIndex < 0 || nIndex >= arElements.GetSize()) { return FALSE; } if (GetParentWnd()->GetSafeHwnd() == NULL) { return S_FALSE; } ASSERT_VALID(arElements[nIndex]); return arElements[nIndex]->SetACCData(GetParentWnd(), m_AccData); }

PVS-Studio 警告：V716 returnステートメントでの疑わしい型変換：HRESULTを返しましたが、関数は実際にはBOOLを返します。afxribbonpanel.cpp4107。

この関数はBOOL型を返します。親ウィンドウからHWNDを取得できない場合、プログラマは関数からFALSE値を返したいと考えていました。しかし、彼は封印し、すべてを根本的に変えるS_FALSEを書きました。

これは、S_FALSE定数の宣言方法です。

 #define S_FALSE ((HRESULT)1L)

何が起こっているのかはすでに理解していると思いますが、念のために説明します。

「return S_FALSE;」と書くことは、「return TRUE;」と書くことと同じです。壮大な失敗。

エラーは一人ではなく、彼女には友人がいます：



記事の冒頭で述べたように、すべてのファイルをチェックしませんでした。さらに、アナライザーによって発行された警告の中から何かを見落としているかもしれません。したがって、このドキュメントの開発者をいくつかのエラーを修正するためのガイドと見なさないようにしてください。開発者自身がライブラリを完全にチェックし、アナライザーの警告を注意深く調べると、はるかに優れています。

おわりに


写真4



もう一度、静的コード分析ツールの利点を読者に示すことができました。

一つの間違いに対して警告したい。時々、リリースに備えて一部のプログラマーが静的アナライザーを起動すると聞きます。誰かがこれを行い、これが正常であると主張する場合、彼は深く誤解されています。これは、静的アナライザーを使用する間違った方法です。 これは、リリース前にコンパイラの警告をオンにして、残りの時間はプロジェクトで作業し、それらを完全に無効にすることと同じです。

PVS-Studioアナライザーのデモ版をインストールし、プロジェクトを確認してみてください。

PVS-Studio製品ページ：https : //www.viva64.com/en/pvs-studio/

サポートされている言語とコンパイラ：


読んでくれてありがとう、Twitter @Code_Analysisでフォローしてください。



この記事を英語圏の聴衆と共有したい場合は、翻訳へのリンクを使用してください：Andrey Karpov。 PVS-Studioを使用してVisual C ++ 2017ライブラリを改善する方法

記事を読んで質問がありますか？
多くの場合、記事には同じ質問が寄せられます。 ここで回答を収集しました： PVS-Studioバージョン2015に関する記事の読者からの質問への回答 。 リストをご覧ください。

Source: https://habr.com/ru/post/J327784/

More articles:


All Articles