PVS-Studioを使用したMozilla Thunderbirdの静的コード分析

この記事では、PVS-Studio静的アナライザーを使用したMozilla Thunderbirdプロジェクトのチェックについて説明します。 Thunderbirdを使用すると、フリーズや奇妙なプログラムの動作に遭遇することがありました。 おそらく、ソースコードでこの理由の少なくともいくつかを見つけることができます。 このような人気のあるプロジェクトでどのようなエラーが隠されるのかをご覧ください。



Mozilla Thunderbirdメールクライアント



Mozilla Thunderbirdは、Mozilla Foundationが開発した無料のクロスプラットフォームのフリーウェアメールおよびニュースグループプログラムです。 Thunderbirdを使用する主な利点は、プログラムのシンプルさと柔軟性です。 ユーザーは、ボタンを変更、追加、または削除して、インターフェイスをカスタマイズできます。 これに加えて、拡張機能と新しいテーマのインストールがサポートされています。 プログラムは、デジタル署名、メッセージの暗号化、証明書の検証を使用できます。



PVS-Studioアナライザーについて



PVS-Studioは、CおよびC ++プログラム用の静的コードアナライザーです。 PVS-StudioはVisual Studio開発システムへのプラグインとして提供されていますが、スタンドアロンユーティリティを介して使用することもできます。 このユーティリティには、コンパイラー呼び出しを追跡し、必要なファイルをアナライザーに転送する監視機能があります。 したがって、PVS-Studioはプロジェクトのビルドシステムから独立しています。



このツールは使いやすいので、多くの言葉を使うよりも、自分のコードでデモ版をダウンロードして試してみる方が良いでしょう。



Thunderbirdのビルドとテスト



Mozillaには独自のビルドシステムがあります。 プロジェクトを構築するための基本手順を説明する手順はここにあります 。 アセンブリ自体は、ユーザーにとって可能な限り便利になっています。 Mozillaは、7zip、msys、mercurialなど、Windowsでのインストールに必要なすべてのユーティリティのバイナリインストーラーを提供します。



このチェックは、PVS-Studioに含まれているスタンドアロンユーティリティにある上記のコンパイラの呼び出し監視システムを使用して実行されました。



アナライザーの警告



Thunderbirdは大規模なプロジェクトであり、多くのサードパーティライブラリを使用しています。 警告のほとんどは、正確にそのコードから来ました。 この記事では、これらの警告を除外し、メールプログラムのソースコードに対して発行された警告のみを残そうとしました。



さらに、Mozillaプロジェクトのバグを説明するために、 キーワードの説明を含むページがあります。 それらの中には、コベリティ、klocwork、valgrind、clang-anazylerなどがあります。 これらのコード分析ツールはすでにMozillaで使用されているようです。 したがって、これらのアナライザーが気付かなかったものを見るのは興味深いでしょう。



疑わしい条件



PVS-Studio警告: V501 「||」の左側と右側に同じサブ式「aStatus == NS_ERROR_OFFLINE」があります 演算子。 nsdocshell.cpp 7606

nsresult nsDocShell::EndPageLoad(nsresult aStatus, ....) { if(....) { .... } else if (aStatus == NS_ERROR_NET_TIMEOUT || .... aStatus == NS_ERROR_OFFLINE || aStatus == NS_ERROR_MALWARE_URI || aStatus == NS_ERROR_PHISHING_URI || aStatus == NS_ERROR_UNWANTED_URI || aStatus == NS_ERROR_UNSAFE_CONTENT_TYPE || aStatus == NS_ERROR_REMOTE_XUL || aStatus == NS_ERROR_OFFLINE || ....) }
      
      





このコードには、追加のチェック「NS_ERROR_OFFLINE」が含まれています。 変数 'aStatus'をチェックする値のリストは大きいため、簡単に間違いを犯して誤ってチェックを複製してしまう可能性があります。 2番目のオプションは、プログラマーがコピー後に同じ部分を上書きしないように同じ行を挿入し、定数「NS_ERROR_OFFLINE」の名前を変更するのを忘れることです。 この場合、必要なチェックがコードにありません。



PVS-Studio 警告V590 'type!=(1)&& type ==(2)'式の検査を検討してください。 表現が過剰であるか、誤植が含まれています。 nswindowsregkey.cpp 313

 #define REG_SZ ( 1 ) #define REG_EXPAND_SZ ( 2 ) #define REG_MULTI_SZ ( 7 ) NS_IMETHODIMP nsWindowsRegKey::ReadStringValue(const nsAString& aName, nsAString& aResult) { .... if (type != REG_SZ && type == REG_EXPAND_SZ && type == REG_MULTI_SZ) { return NS_ERROR_FAILURE; } .... }
      
      





1つの変数が同時に2つの値を持つことはできないため、条件 "type == REG_EXPAND_SZ && type == REG_MULTI_SZ"は常にfalseです。 結果として、関数はエラーステータスNS_ERROR_FAILUREを返すことはありません。



PVS-Studio警告: V616 値が0の「eBorderStyle_none」という名前の定数がビット演算で使用されます。 nswindow.cpp 2318

 enum nsBorderStyle { eBorderStyle_none = 0, .... } NS_IMETHODIMP nsWindow::SetNonClientMargins(....) { if (!mIsTopWidgetWindow || mBorderStyle & eBorderStyle_none) return NS_ERROR_INVALID_ARG; .... }
      
      





条件をチェックするときに、値が0の定数が使用されます。これは、変数を使用したビット演算「AND」に参加します。 もちろん、操作の結果もゼロになります。 したがって、条件は変数「mBorderStyle」に依存しません。



同様の警告:





PVS-Studio警告: V646 アプリケーションのロジックの検査を検討してください。 「else」キーワードが欠落している可能性があります。 nsnativethemewin.cpp 924



 nsresult nsNativeThemeWin::GetThemePartAndState(nsIFrame* aFrame, uint8_t aWidgetType, int32_t& aPart, int32_t& aState) { .... { .... if (!aFrame) { aState = TS_NORMAL; } else { if (GetCheckedOrSelected(aFrame, !isCheckbox)) { inputState = CHECKED; } if (isCheckbox && GetIndeterminate(aFrame)) { inputState = INDETERMINATE; } .... } .... }
      
      





最後の「if」の前に、elseという単語が欠落している可能性があります。 現在のフォームのコードは、条件を満たせば、変数「inputState」の「CHECKED」の値が「INDETERMINATE」に変更されることを意味します。 このコードで1つの条件または別の条件のいずれかが満たされた場合、外部構造のように「if-else」を使用する方が論理的です。



別の同様の設計はここにあります:

PVS-Studio警告: V713 ポインターmHTMLEditorは、同じ論理式のnullptrに対して検証される前に、論理式で使用されました。 nshtmleditrules.cpp 6593



 nsHTMLEditor* mHTMLEditor; nsresult nsHTMLEditRules::SplitParagraph(...) { if (mHTMLEditor->IsTextNode(child) || !mHTMLEditor || mHTMLEditor->IsContainer(child)) .... }
      
      





チェックのSplitParagraph関数には、誤った引数の順序が含まれています。 このコードでmHTMLEditorポインターがnullの場合、これを検出する前に既に間接参照されているため、未定義の動作が発生します。 コードを修正するには、「!MHTMLEditor」と「mHTMLEditor-> IsTextNode(child)」を交換する必要があります。



2つの同様のエラーがここにあります。





PVS-Studio警告: V522 nullポインター 'aStyleValues'の 参照が行われる場合があります。 sdnaccessible.cpp 252



 STDMETHODIMP sdnAccessible::get_computedStyle( BSTR __RPC_FAR* aStyleProperties, BSTR __RPC_FAR* aStyleValues, unsigned short __RPC_FAR* aNumStyleProperties) { if (!aStyleProperties || aStyleValues || !aNumStyleProperties) return E_INVALIDARG; .... aStyleValues[realIndex] = ::SysAllocString(value.get()); .... }
      
      





sayingにもあるように、いたずらに注意してください。







アナライザーは、ヌルポインターの逆参照を検出しました。 チェックするとき、プログラマーは「!」 「aStyleValues」の前。 それ以降のコードは、このポインターがゼロの場合にのみ制御を取得し、その逆参照につながります。



PVS-Studio警告: V547 式は常にfalseです。 おそらく '||' ここで演算子を使用する必要があります。 nsmsgdbview.cpp 3014



 class NS_NO_VTABLE nsMsgViewCommandType { enum { .... junk = 27, unjunk = 28, .... }; }; nsresult nsMsgDBView:: ApplyCommandToIndices(nsMsgViewCommandTypeValue command, ....) { .... if ((command == nsMsgViewCommandType::junk) && (command == nsMsgViewCommandType::unjunk)) .... }
      
      





コマンド変数が同時に2つの値を持つことはできないため、ifブロックに対応するコードは実行されません。 ここでは、「OR」演算子-「||」を使用する方が論理的です。



ポインターの問題



PVS-Studio警告: V579 HashBytes関数は、ポインターとそのサイズを引数として受け取ります。 間違いかもしれません。 2番目の引数を調べます。 nsdisplaylist.h 929



 struct AnimatedGeometryRootLookup { .... PLDHashNumber Hash() const { return mozilla::HashBytes(this, sizeof(this)); } .... }
      
      





アナライザーは、ポインターが最初の引数としてHashBytes関数に渡され、ポインターのサイズが2番目の引数として渡されると疑っています。 ソースファイルで関数の名前を調べると、hashfunctions.hファイルで次のコメントを見つけることができます。



 /* Utilities for hashing. */ /* * This file exports functions for hashing data down * to a 32-bit value, including: .... * - HashBytes Hash a byte array of known length. .... */
      
      





コメントは、2番目の引数がポインターにあるオブジェクトのサイズであることを示唆しています。 ほとんどの場合、正しいコードは次のようになります。



 return mozilla::HashBytes(this, sizeof(*this));
      
      





次の警告に移りましょう。



PVS-Studio警告: V611 メモリは「new」演算子を使用して割り当てられましたが、「free」機能を使用して解放されました。 「instanceData」変数の背後にある操作ロジックを調べることを検討してください。 nptest.cpp 971



 NPError NPP_New(....) { .... InstanceData* instanceData = new InstanceData; .... free(instanceData); .... }
      
      





エラーは、メモリが「new」演算子を使用して割り当てられ、「free」を使用して解放されることです。 「free」関数は、このポインターにあるオブジェクトのデストラクターを呼び出しません。 つまり、オブジェクトにメモリが割り当てられたポインタがまだ含まれている場合、オブジェクトは解放されず、リークが発生します。



実際、これはできません。 このようなコードは、未定義のプログラムの動作につながります。



PVS-Studio警告: V614 潜在的に初期化されていないポインター 'hOldFont'が使用されました。 progressui_win.cpp 168



 static void InitDialog(....) { .... HFONT hInfoFont, hOldFont; hInfoFont = (HFONT)SendMessage(hWndInfo, WM_GETFONT, 0, 0); if (hInfoFont) hOldFont = (HFONT)SelectObject(hDCInfo, hInfoFont); .... if (hOldFont) SelectObject(hDCInfo, hOldFont); .... }
      
      





SendMessage関数がゼロを返す場合、次のチェックの結果はfalseになります。つまり、hOldFont変数は初期化されません。 変数にはランダムな値があり、ゼロ以外の値になる場合があります。 0でない場合、このランダムな値はSelectObject関数に渡されます。



別の同様の状況がここで発生する可能性があります。





コピーアンドペーストエラー



PVS-Studio 警告V517 「if(A){...} else if(A){...}」パターンの使用が検出されました。 論理エラーが存在する可能性があります。 行を確認してください:1060、1062。nsstylestruct.cpp 1060



 nsStyleClipPath::nsStyleClipPath(const nsStyleClipPath& aSource) { if (aSource.mType == NS_STYLE_CLIP_PATH_URL) { SetURL(aSource.mURL); } else if (aSource.mType == NS_STYLE_CLIP_PATH_SHAPE) { SetBasicShape(aSource.mBasicShape, aSource.mSizingBox); } else if (aSource.mType == NS_STYLE_CLIP_PATH_SHAPE) { SetSizingBox(aSource.mSizingBox); } }
      
      





if-else ifブロックには、copy-pasteメソッドによって呼び出される等価チェックが繰り返されます。 つまり、「NS_STYLE_CLIP_PATH_SHAPE」の2番目のチェックに対応するコードの最後の部分は実行されません。







PVS-Studio警告: V523 「then」ステートメントは「else」ステートメントと同等です。

mozspelli18nmanager.cpp 34



 NS_IMETHODIMP mozSpellI18NManager::GetUtil(mozISpellI18NUtil **_retval, ....) { .... nsAutoString lang; .... if(lang.EqualsLiteral("en")) { *_retval = new mozEnglishWordUtils; } else { *_retval = new mozEnglishWordUtils; } NS_IF_ADDREF(*_retval); return NS_OK; }
      
      





アナライザーは、ifブロックとelseブロックが同じアクションに対応していることに気付きました。 これは、コピー時の間違い、余分な条件、または単に不完全なコードである可能性があります。 何らかの形で、この形式では、条件は意味をなしません。



さらにいくつかの同様のエラー:





未定義の動作



PVS-Studio警告: V595 nullptrに対して検証される前に、「aParent」ポインターが使用されました。 行を確認してください:511、518。nsgenericdomdatanode.cpp 511



 #define NS_ADDREF(_ptr) \ (_ptr)->AddRef() nsresult nsGenericDOMDataNode::BindToTree(nsIContent* aParent, ....) { .... ShadowRoot* parentContainingShadow = aParent->GetContainingShadow(); .... if (aParent) { if (!GetParent()) { NS_ADDREF(aParent); } mParent = aParent; } .... }
      
      





aParentポインターをチェックすると、nullになる可能性があることが示されます。 これは、チェックの前に最初に参照解除されるときに、未定義の動作が発生するリスクが生じることを意味します。



警告V595は検証済みプロジェクトの中で最も一般的なものの1つであり、Thunderbirdも例外ではありません。 合計で、アナライザーはThunderbirdコードに直接関連する95個の警告を生成しました。



警告PVS-Studio: V610 未定義の動作。 シフト演算子「<<」を確認してください。 左のオペランド '〜0L'は負です。 nsprotocolproxyservice.cpp 336



 static void proxy_MaskIPv6Addr(PRIPv6Addr &addr, uint16_t mask_len) { .... addr.pr_s6_addr32[3] = PR_htonl( PR_ntohl(addr.pr_s6_addr32[3]) & (~0L << (128 - mask_len))); .... }
      
      





左オフセットパラメータの1つが負の数である場合、動作は未定義です。 標準についての説明は次のとおりです。



シフト演算子<<および>>は、左から右にグループ化します。 シフト式<<加算式、シフト式>>加算式。



オペランドは整数またはスコープなしの列挙型である必要があり、整数のプロモーションが実行されます。 1.結果の型は、昇格した左オペランドの型です。 右のオペランドが負の場合、または昇格した左のオペランドのビット長以上の場合、 動作は未定義です。 2. ... E1に符号なしの型がある場合、結果の値はE1 * 2 ^ E2で、結果の型で表現可能な最大値よりも1を法として減じられます。 それ以外の場合、E1に符号付きの型と負でない値があり、E1 * 2 ^ E2が結果の型で表現できる場合、それが結果の値です。 それ以外の場合、 動作はundefinedです。 ...



未定義の動作のさらに3つのケース:





機能を備えたアラート



PVS-Studio警告: V597 コンパイラーは、 'ctx'オブジェクトのフラッシュに使用される 'memset'関数呼び出しを削除できました。 RtlSecureZeroMemory()関数を使用して、プライベートデータを消去する必要があります。 gmploader.cpp 166



 bool GMPLoaderImpl::Load(....) { SHA256Context ctx; .... // Overwrite all data involved in calculation as it could //potentially identify the user, so there's no chance a GMP //can read it and use it for identity tracking. memset(&ctx, 0, sizeof(ctx)); .... }
      
      





ここで、アナライザーは、「memset」関数の呼び出しを削除できることに気付きました。 変数 'c​​tx'は将来使用されないため、コンパイラは最適化中にmemset呼び出しを削除するすべての権利を持っています。 Windowsでは、「RtlSecureZeroMemory」機能を使用できます。



PVS-Studio警告: V530 関数 'getenv'の戻り値を使用する必要があります。

nswindowswmain.cpp 134



 int wmain(int argc, WCHAR **argv) { .... // Force creation of the multibyte _environ variable. getenv("PATH"); int result = main(argc, argvConverted, _environ); .... }
      
      





ここではgetenv関数の呼び出しを扱っていますが、その結果は使用されず、変数にも書き込まれません。 この機能がcplusplus.comでどのように説明されているかを次に示します。



引数として名前が指定されている環境変数の値を含むC文字列を取得します。 要求された変数が環境リストの一部ではない場合、関数はNULLポインターを返します。



この形式で「getenv」を使用することは無意味であり、コードを読み取るときに混乱するだけです。



UPDhabrahabr.ru/company/pvs-studio/blog/267663/#comment_8589575



その他の警告







警告PVS-Studio: V609 ゼロ除算。 分母範囲[0..8]。 ionbuilder.cpp 10922



 static inline size_t UnboxedTypeSize(JSValueType type) { switch (type) { .... default: return 0; } } MInstruction*IonBuilder::loadUnboxedProperty(size_t offset, JSValueType unboxedType, ....) { size_t index = offset / UnboxedTypeSize(unboxedType); .... }
      
      





UnboxedTypeSize関数はゼロを返すことができるため、ゼロによる除算を処理する可能性があります。 新しいタイプがUnboxedTypeSize関数に渡されると、デフォルト値のゼロが返され、例外が発生します。 安全にプレイして、分割前に検証を追加することをお勧めします。



ゼロによる別の潜在的な除算:





警告PVS-Studio: V621 「for」演算子の検査を検討してください。 ループが誤って実行されるか、まったく実行されない可能性があります。 nsmsgdbfolder.cpp 4501



 NS_IMETHODIMP nsMsgDBFolder::GetDisplayRecipients(bool *displayRecipients) { .... // There's one FCC folder for sent mail, and one for sent news nsIMsgFolder *fccFolders[2]; int numFccFolders = 0; for (int i = 0; i < numFccFolders; i++) { .... } .... }
      
      





アナライザーは、ループが1回繰り返されない疑わしい場所を見つけました。 この理由は、値がゼロの「numFccFolders」変数です。 おそらく、この割り当ては何らかの目的で行われますが、タイプミスである可能性もあります。 上記のポインターのコメントと宣言は、変数の値が2であることを示唆しています。



PVS-Studio警告: V678 オブジェクトは、独自のメソッドの引数として使用されます。 'Assign'関数の最初の実引数を確認することを検討してください。 nsgenerichtmlelement.h 411



 class nsGenericHTMLElement : public nsGenericHTMLElementBase, public nsIDOMHTMLElement { .... NS_IMETHOD GetItemId(nsAString& aId) final override { nsString id; GetItemId(id); aId.Assign(aId); return NS_OK; } .... }
      
      





「aId」オブジェクトを独自のメソッドの引数として使用すること自体はエラーではありません。 ただし、関数は類似した名前「id」を持つ変数を使用するため、このコードは疑わしいです。 これは、タイプミスが含まれており、引数「aId.Assign」が変数「id」であることを示唆しています。



PVS-Studio警告: V670 「mWorkerStatements」メンバーを初期化するために、初期化されていないクラスメンバー「mWorkerConnection」が使用されます。 メンバーは、クラス内の宣言の順序で初期化されることに注意してください。 domstoragedbthread.cpp 50



 DOMStorageDBThread::DOMStorageDBThread() : mWorkerStatements(mWorkerConnection) , .... {} class DOMStorageDBThread final : public DOMStorageDBBridge { private: .... StatementCache mWorkerStatements; //<=line 304 .... nsCOMPtr<mozIStorageConnection> mWorkerConnection; //<=line 309 .... }
      
      





初期化リストを使用する場合、変数はクラスで宣言された順序で初期化されることに注意してください。初期化リストの順序は重要ではありません。 このコードでは、mWorkerStatements変数は別のクラスのmWorkerConnectionオブジェクトに初期化されます。 ただし、初期化の時点では、このオブジェクトはmWorkerStatements変数よりも後でクラスで宣言されているため、このオブジェクトのコンストラクターはまだ呼び出されていません。 これを修正するには、クラス内のこれら2つのオブジェクトの宣言を入れ替えるだけです。



このクラスでは、別の同じエラーが隠れています:





おわりに



要約すると、PVS-StudioがMozilla Thunderbirdプロジェクトで多くの疑わしい場所を見つけたことに注意したいと思います。 それらのほとんどはサードパーティのライブラリに属します。 それでも、Thunderbird自体には興味深いエラーがありました。



最も経験豊富で注意深いプログラマーでさえ、大規模なプロジェクトをエラーなしで書く余裕はありません。 そのような目的のために、静的コードアナライザーがあります。 それらを使用すると、古いエラーを検索する時間を節約し、新しいエラーを防ぐことができます。 プロジェクトでPVS-Studioを試すことをお勧めします: http : //www.viva64.com/en/pvs-studio-download/







英語を話す聴衆とこの記事を共有したい場合は、翻訳へのリンクを使用してください:Igor Shtukarev。 PVS-StudioによるMozilla Thunderbirdのコードの静的分析



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




All Articles