Geekly Articles each Day

LibreOfficeプロジェクトの検証

読者に、有名なオープンソースプロジェクトのテストに関する別の記事を提供します。 今回は、オフィススイートであるLibreOfficeプロジェクトを確認しました。 480人以上のプログラマーが開発に参加しています。 コードは、非常に高品質で、定期的にチェックされる静的Coverityアナライザーであることが判明しました。 しかし、他の大規模プロジェクトと同様に、新しいエラーと欠点が見つかりました。これについては記事で説明します。 変更のために、今回はユニコーンではなく牛を同伴します。

LibreOfficeは、32/64ビットシステムと完全に互換性のある強力なオフィススイートです。 世界の30以上の言語に翻訳されています。 GNU / Linux、Microsoft Windows、Mac OS Xなど、ほとんどの一般的なオペレーティングシステムをサポートしています。

LibreOfficeは無料のオープンソースです。 Java、Python、C ++の言語で書かれています。 C ++（およびC、 C ++ / CLIで少し）で記述されたプロジェクトの部分が分析されました 。 バージョン：4.5.0.0.alpha0 +（Gitリビジョン：368367）。

分析は、 PVS-Studio静的コードアナライザーを使用して実行されました。

どのエラーが検出され、それらで何ができるかを検討してください。 一部のエラーはエラーではない可能性があることに注意してください。 私はコードに詳しくないので、完全に正しいコードをエラーと間違えた可能性があります。 しかし、このコードはアナライザーと私を困惑させたので、何かが間違っています。 このコードは臭いがするので、プロジェクトの開発中に誤解される可能性を減らすためにリファクタリングするのが良いでしょう。

タイプミス


どんなコードもタイプミスなしではできません。 もちろん、多くはテスト段階で発見され修正されますが、一部はプログラム内で長期間存続します。 原則として、それらはめったに使用されない関数にあるか、プログラムに強い影響を与えません。

たとえば、3分の1だけで機能するこのような比較関数がありました。
class SvgGradientEntry { .... bool operator==(const SvgGradientEntry& rCompare) const { return (getOffset() == rCompare.getOffset() && getColor() == getColor() && getOpacity() == getOpacity()); } .... }

PVS-Studio警告： V501 「==」演算子の左右に同じ副次式があります：getColor（）== getColor（）svggradientprimitive2d.hxx 61

おそらく、この間違いはそれほど害をもたらさないでしょう。 おそらく、この演算子「==」はまったく使用されていません。 ただし、アナライザーはこのエラーを検出できたため、新しいコードを作成した直後に、より深刻なエラーを検出できます。 したがって、静的分析の主な価値は、1回限りの実行ではなく、定期的な使用です。

このようなエラーを回避するにはどうすればよいでしょうか？ 知りません おそらく、同じタイプのコードのブロックをより注意深く整列させることに慣れると、エラーはより顕著になります。 たとえば、関数は次のように記述できます。
 bool operator==(const SvgGradientEntry& rCompare) const { return getOffset() == rCompare.getOffset() && getColor() == getColor() && getOpacity() == getOpacity(); }

右側の列に「rCompare」が欠落していることがより顕著になりました。 正直に言って、それほど顕著ではありません。 助けにならないかもしれません。 誤ることは人間です。 したがって、静的アナライザーは良いヘルパーになります。

そして、タイプミスが明らかに回避できた例です。 誰かが2つの変数間で値を交換するためにコードを記述できませんでした。



 void TabBar::ImplGetColors(....) { .... aTempColor = rFaceTextColor; rFaceTextColor = rSelectTextColor; rSelectTextColor = rFaceTextColor; .... }

警告PVS-Studio： V587この種の割り当ての奇数シーケンス：A = B; B = A;。 行を確認してください：565、566。tabbar.cxx 566

最後の行では、「rFaceTextColor」ではなく「aTempColor」を使用する必要があります。

値を「手動で」交換するコードを記述する必要はありませんでした。 標準関数std :: swap（）を使用する方が簡単で信頼性が高くなります。
 swap(rFaceTextColor, rSelectTextColor);

続けましょう。 次の間違いを防ぐことは不可能だと思います。 純粋なタイプミス：
 void SAL_CALL Theme::disposing (void) { ChangeListeners aListeners; maChangeListeners.swap(aListeners); const lang::EventObject aEvent (static_cast<XWeak*>(this)); for (ChangeListeners::const_iterator iContainer(maChangeListeners.begin()), iContainerEnd(maChangeListeners.end()); iContainerEnd!=iContainerEnd; ++iContainerEnd) { .... } }

PVS-Studio警告：V501 '！='演算子の左右に同じ副次式があります：iContainerEnd！= IContainerEnd theme.cxx 439

条件「iContainerEnd！= IContainerEnd」は常にfalseであるため、ループは実行されません。 イテレータの同様の名前をまとめました。 実際には、「iContainer！= IContainerEnd」と記述する必要がありました。 ところで、ここには別の間違いがあるようです。 iContainerEndイテレータが増えているのは奇妙です。

別の失敗したループを考えてみましょう：
 static void lcl_FillSubRegionList(....) { .... for( IDocumentMarkAccess::const_iterator_t ppMark = pMarkAccess->getBookmarksBegin(); <<<<---- ppMark != pMarkAccess->getBookmarksBegin(); <<<<---- ++ppMark) { const ::sw::mark::IMark* pBkmk = ppMark->get(); if( pBkmk->IsExpanded() ) rSubRegions.InsertEntry( pBkmk->GetName() ); } }

警告PVS-Studio：V625「for」演算子の検査を検討してください。 イテレータの初期値と最終値は同じです。 uiregionsw.cxx 120

ループは実行されません。 条件では、イテレータ 'ppMark'を 'pMarkAccess-> getBookmarksEnd（）'と比較する必要があります。 コード記述ルールの助けを借りて、このようなエラーから身を守る方法はありません。 ただのタイプミス。

ところで、時々エラーがありますが、プログラムの正しい実行には影響しません。 次に、これらのタイプミスの1つを示します。
 bool PolyPolygonEditor::DeletePoints(....) { bool bPolyPolyChanged = false; std::set< sal_uInt16 >::const_reverse_iterator aIter;( rAbsPoints.rbegin() ); for( aIter = rAbsPoints.rbegin(); aIter != rAbsPoints.rend(); ++aIter ) .... }

PVS-Studio警告： V530関数 'rbegin'の戻り値を使用する必要があります。 polypolygoneditor.cxx 38

エラーはここにあります：aIter;（rAbsPoints.rbegin（））;

彼らはイテレータを初期化したかった。 しかし、誤ってセミコロンがくさび状になっています。 イテレータは初期化されていません。 そして、式「（rAbsPoints.rbegin（））;」 空中でぶらぶらして何もしません。

この状況は、ループ内でイテレーターが目的の値で初期化されているという事実によって保存されます。 一般に、エラーはありませんが、過剰な表現を削除することをお勧めします。 ちなみに、このサイクルはコピーペーストを使用して伝播されたため、同じファイルの69行目と129行目を見る価値があります。

最後に、クラスコンストラクターのタイプミス：
 XMLTransformerOOoEventMap_Impl::XMLTransformerOOoEventMap_Impl( XMLTransformerEventMapEntry *pInit, XMLTransformerEventMapEntry *pInit2 ) { if( pInit ) AddMap( pInit ); if( pInit ) AddMap( pInit2 ); }

PVS-Studio警告：V581互いに並んでいる「if」演算子の条件式は同一です。 行を確認してください：77、79。eventoootcontext.cxx 79

2番目のifステートメントはpInit2ポインターをチェックする必要があります。

計画どおりかもしれませんが、非常に疑わしい


タイプミスを含むと思われるコードスニペットがいくつかあります。 しかし、私にはわかりません。 おそらくそれが意図されたものです。
 class VCL_DLLPUBLIC MouseSettings { .... long GetStartDragWidth() const; long GetStartDragHeight() const; .... } bool ImplHandleMouseEvent( .... ) { .... long nDragW = rMSettings.GetStartDragWidth(); long nDragH = rMSettings.GetStartDragWidth(); .... }

警告： V656変数 'nDragW'、 'nDragH'は、同じ関数の呼び出しによって初期化されます。 おそらくエラーまたは最適化されていないコードです。 「rMSettings.GetStartDragWidth（）」式を調べることを検討してください。 行を確認してください：471、472。winproc.cxx 472

変数nDragWとnDragHを同じ値で初期化する必要があるかどうかは明確ではありません。 その場合、十分なコメントがありません。 または、明示的に記述する方が良いでしょう：
 long nDragW = rMSettings.GetStartDragWidth(); long nDragH = nDragW;

同様の状況：
 void Edit::ImplDelete(....) { .... maSelection.Min() = aSelection.Min(); maSelection.Max() = aSelection.Min(); .... }

V656変数「maSelection.Min（）」、「maSelection.Max（）」は、同じ関数の呼び出しによって初期化されます。 おそらくエラーまたは最適化されていないコードです。 「aSelection.Min（）」式の検査を検討してください。 行を確認してください：756、757。edit.cxx 757

プロジェクトで作業する人にとって、ここのすべてはすぐに明らかです。 私は働いていないので、ここにエラーがあるかどうかはわかりません。

そして最後のケース。 クラスには3つの関数があります。ただし、ここでは、GetVRP（）関数を使用して定数「aVPN」を初期化します。
 void ViewContactOfE3dScene::createViewInformation3D(....) { .... const basegfx::B3DPoint aVRP(rSceneCamera.GetVRP()); const basegfx::B3DVector aVPN(rSceneCamera.GetVRP()); <<<--- const basegfx::B3DVector aVUV(rSceneCamera.GetVUV()); .... }

PVS-Studio警告：V656変数「aVRP」、「aVPN」は、同じ関数の呼び出しによって初期化されます。 おそらくエラーまたは最適化されていないコードです。 「rSceneCamera.GetVRP（）」式の検査を検討してください。 行を確認してください：177、178。viewcontactofe3dscene.cxx 178

アナライザーは別の警告V656を発行しました。 本当の間違いがあると確信しています。 しかし、面倒なのでコードは引用しません。 開発者にここを見てもらいます：

コピーペースト




Copy-Pasteがなければ、プログラミングは非常に退屈で退屈になることがあります。 Ctrl-C、Ctrl-Vがないと、これらのキーボードショートカットをどのように無効にしても、プログラミングは機能しません。 したがって、コードをコピーしないでください。 ただし、コードをコピーするときは注意してください。
 uno::Sequence< OUString > SwXTextTable::getSupportedServiceNames(void) { uno::Sequence< OUString > aRet(4); OUString* pArr = aRet.getArray(); pArr[0] = "com.sun.star.document.LinkTarget"; pArr[1] = "com.sun.star.text.TextTable"; pArr[2] = "com.sun.star.text.TextContent"; pArr[2] = "com.sun.star.text.TextSortable"; return aRet; }

PVS-Studio警告： V519 「pArr [2]」変数には値が連続して2回割り当てられます。 おそらくこれは間違いです。 行を確認してください：3735、3736。unotbl.cxx 3736

最後の行の古典的な効果 。 最後の行は最後から2番目の行から取得されたと確信しています。 「コンテンツ」を「ソート可能」に置き換えましたが、「2」インデックスを忘れていました。

非常によく似たケース：
 Sequence<OUString> FirebirdDriver::getSupportedServiceNames_Static() { Sequence< OUString > aSNS( 2 ); aSNS[0] = "com.sun.star.sdbc.Driver"; aSNS[0] = "com.sun.star.sdbcx.Driver"; return aSNS; }

PVS-Studio警告：V519「aSNS [0]」変数には、値が連続して2回割り当てられます。 おそらくこれは間違いです。 行を確認してください：137、138。driver.cxx 138

しかし、最悪のことは、コピーと貼り付けのエラーのおかげで時々伝播することです。 これを例で示します。 残念ながら、コードを読むのは少し難しいでしょう。 我慢してください。

そのため、プログラムには次の機能があります。
 static bool GetPropertyValue( ::com::sun::star::uno::Any& rAny, const ::com::sun::star::uno::Reference< ::com::sun::star::beans::XPropertySet > &, const OUString& rPropertyName, bool bTestPropertyAvailability = false );

'bTestPropertyAvailability'の最後の引数はオプションであることに注意してください。

また、「sal_True」とは次のとおりです。
 #define sal_True ((sal_Bool)1)

これで実際のエラー。 GetPropertyValue（）関数の呼び出し方法を参照してください。
 sal_Int32 PPTWriterBase::GetLayoutOffset(....) const { ::com::sun::star::uno::Any aAny; sal_Int32 nLayout = 20; if ( GetPropertyValue( aAny, rXPropSet, OUString( "Layout" ) ), sal_True ) aAny >>= nLayout; DBG(printf("GetLayoutOffset %" SAL_PRIdINT32 "\n", nLayout)); return nLayout; }

PVS-Studio警告： V639 「GetPropertyValue」関数呼び出しの式を調べることを検討してください。 閉じ括弧 '）'の1つが誤って配置された可能性があります。 pptx-epptbase.cxx 442

よく見ると、閉じ括弧の1つがその場所にないことがわかります。 その結果、GetPropertyValue（）関数は、最後の引数として「sal_True」ではなく、デフォルト値（「false」に等しい）を受け取ります。

しかし、これは問題の半分に過ぎません。 さらに、「if」演算子の作業が台無しになりました。 状態は次のようになります。
 if (foo(), sal_True)

コンマ演算子は右側を返します。 その結果、条件は常に真になります。

このコードのエラーは、Copy-Pasteとは関係ありません。 通常のタイプミス。 ブラケットはありません。 それは起こります。

悲しいことは、このエラーがプログラムの他の部分に伝播されたことです。 ある場所で間違いが修正された場合、他の場所では間違いが残る可能性が高くなります。

Copy-Pasteにより、さらに9か所でこのエラーが発生しました。結論として、最新の重要ではない警告のセクション3。 わずか1つの追加チェック：
 #define CHECK_N_TRANSLATE( name ) \ else if (sServiceName == SERVICE_PERSISTENT_COMPONENT_##name) \ sToWriteServiceName = SERVICE_##name void OElementExport::exportServiceNameAttribute() { .... CHECK_N_TRANSLATE( FORM ); <<<<---- CHECK_N_TRANSLATE( FORM ); <<<<---- CHECK_N_TRANSLATE( LISTBOX ); CHECK_N_TRANSLATE( COMBOBOX ); CHECK_N_TRANSLATE( RADIOBUTTON ); CHECK_N_TRANSLATE( GROUPBOX ); CHECK_N_TRANSLATE( FIXEDTEXT ); CHECK_N_TRANSLATE( COMMANDBUTTON ); .... }

PVS-Studio 警告 ： V517 「if（A）{...} else if（A）{...}」パターンの使用が検出されました。 論理エラーが存在する可能性があります。 行を確認してください：177、178。elementexport.cxx 177

何も問題はありませんが、欠陥があります。 さらに2つの追加チェックがここにあります：

realloc（）関数の勇敢な使用


realloc（）関数が使用されているので、明らかに安全ではないので、間違いと呼ぶリスクはありません。 どうやら、これは著者の意識的な決定です。 malloc（）/ realloc（）を使用してメモリを割り当てることはできなかったので、プログラムをすぐに落としてください。 「キック」するものは何もありません。 とにかく、プログラムが引き続き機能する場合、これがうまくいく可能性は低いです。 ただし、アナライザーによって発行されたメッセージを誤検知としてカウントすることはフェアではありません。 したがって、アナライザーが気に入らなかった点を検討してください。

たとえば、FastAttributeListクラスのadd（）関数の実装を調べます。
 void FastAttributeList::add(sal_Int32 nToken, const sal_Char* pValue, size_t nValueLength ) { maAttributeTokens.push_back( nToken ); sal_Int32 nWritePosition = maAttributeValues.back(); maAttributeValues.push_back( maAttributeValues.back() + nValueLength + 1 ); if (maAttributeValues.back() > mnChunkLength) { mnChunkLength = maAttributeValues.back(); mpChunk = (sal_Char *) realloc( mpChunk, mnChunkLength ); } strncpy(mpChunk + nWritePosition, pValue, nValueLength); mpChunk[nWritePosition + nValueLength] = '\0'; }

PVS-Studio 警告 ： V701 realloc（）リークの可能性：realloc（）がメモリの割り当てに失敗すると、元のポインター 'mpChunk'が失われます。 realloc（）を一時ポインターに割り当てることを検討してください。 fastattribs.cxx 88

このコードの主な問題は、realloc（）関数の結果がチェックされないことです。 もちろん、メモリを割り当てることができない状況は非常にまれです。 しかし、想像してください-それが起こった。 次に、realloc（）はNULLを返します。 次に、strncpy（）関数がデータのコピーを開始すると、緊急事態が発生します。
  mpChunk = (sal_Char *) realloc( mpChunk, mnChunkLength ); } strncpy(mpChunk + nWritePosition, pValue, nValueLength);

しかし、アナライザーはもう一方が好きではありません。 何らかの種類のエラーハンドラがあるとします。 そして、プログラムは実行を続けます。 これは単なるメモリリークです。 mpChunk変数に0が書き込まれ、メモリを解放できなくなります。 このエラーパターンについて詳しく説明します。 多くの人はrealloc（）を誤って考えて使用しません。

人為的なコード例を考えてみましょう：

 char *p = (char *)malloc(10); .... p = (char *)realloc(p, 10000);


メモリを割り当てることができない場合、「p」変数は「破損」します。 現在、ポインタが「p」に保存されていたメモリを解放する方法はありません。

この形式では、エラーは明らかです。 しかし実際には、このコードは非常に一般的です。 アナライザーは、このトピックに関してさらに8つの警告を生成しますが、それらを検討しても意味がありません。 とにかく、LibreOfficeはメモリは常に割り当てられると信じています。

論理エラー


条件で多くの面白い間違いに遭遇しました。 理由は、明らかに、不注意、タイプミス、言語の知識不足です。



 void ScPivotLayoutTreeListData::PushDataFieldNames(....) { .... ScDPLabelData* pLabelData = mpParent->GetLabelData(nColumn); if (pLabelData == NULL && pLabelData->maName.isEmpty()) continue; .... }

PVS-Studio警告： V522ヌルポインター 'pLabelData'の逆参照が行われる場合があります。 論理条件を確認してください。 pivotlayouttreelistdata.cxx 157

条件の論理エラー。 ポインターがゼロの場合、それを逆参照します。 私が理解しているように、演算子||はここで使用されるべきでした。

同様のエラー：
 void grabFocusFromLimitBox( OQueryController& _rController ) { .... vcl::Window* pWindow = VCLUnoHelper::GetWindow( xWindow ); if( pWindow || pWindow->HasChildPathFocus() ) { pWindow->GrabFocusToDocument(); } .... }

PVS-Studio警告：V522ヌルポインター「pWindow」の逆参照が行われる場合があります。 論理条件を確認してください。 querycontroller.cxx 293

ここでは、反対に、「||」の代わりに 「&&」と書く必要があります。

少し複雑な条件：
 enum SbxDataType { SbxEMPTY = 0, SbxNULL = 1, .... }; void SbModule::GetCodeCompleteDataFromParse(CodeCompleteDataCache& aCache) { .... if( (pSymDef->GetType() != SbxEMPTY) || (pSymDef->GetType() != SbxNULL) ) .... }

PVS-Studio警告： V547式は常に真です。 ここでは、おそらく「&&」演算子を使用する必要があります。 sbxmod.cxx 1777

簡単にするために、式を書き直します。
 if (type != 0 || type != 1)

条件は常に真です。

同様のエラーが2つあります：
条件が冗長な2つの場所を満たしました。 これらはエラーだと思います：
 sal_uInt16 ScRange::ParseCols(....) { .... const sal_Unicode* p = rStr.getStr(); .... case formula::FormulaGrammar::CONV_XL_R1C1: if ((p[0] == 'C' || p[0] != 'c') && NULL != (p = lcl_r1c1_get_col( p, rDetails, &aStart, &ignored ))) { .... }

PVS-Studio警告：V590 'p [0] ==' C '||の検査を検討してください p [0]！= 'c' '式。 表現が過剰であるか、誤植が含まれています。 address.cxx 1593

条件（p [0] == 'C' || p [0]！= 'C'）は、（p [0]！= 'C'）に縮小できます。 これは間違いであり、条件は次のようになっているはずです（p [0] == 'C' || p [0] == 'c'）。

以下の同じファイルに同じエラーがあります。
おそらく、ロジックのエラーには、ポインターが最初に間接参照され、ゼロと等しいかどうかだけがチェックされる状況が含まれます。 これは、すべてのプログラムで非常によくある間違いです。 通常、これはコードのリファクタリングのプロセスにおける不注意が原因で発生します。

典型的な例：
 IMPL_LINK(....) { .... SystemWindow *pSysWin = pWindow->GetSystemWindow(); MenuBar *pMBar = pSysWin->GetMenuBar(); if ( pSysWin && pMBar ) { AddMenuBarIcon( pSysWin, true ); } .... }

PVS-Studio警告：V595 nullptrに対して検証される前に、「pSysWin」ポインターが使用されました。 行を確認：738、739。updatecheckui.cxx 738

ポインタ「pSysWin」は、式「pSysWin-> GetMenuBar（）」で逆参照されます。 次に、ゼロと等しいかどうかがチェックされます。

LibreOfficeの作成者は、これらの場所LibreOffice-V595.txtにも注意を払うことをお勧めします。

そして最後に、今回はもっと混乱した状況です。 疲れている場合は、この場所をスキップできます。 通常の列挙を検討してください。
 enum BRC_Sides { WW8_TOP = 0, WW8_LEFT = 1, WW8_BOT = 2, WW8_RIGHT = 3, WW8_BETW = 4 };

名前付き定数は2の累乗ではないことに注意してください。 これらは単なる数字です。 0を含む。

そして、2つの累乗を表すかのように、これらの定数を使用します。 彼らはマスクによって個々のビットを選択してチェックしようとします：
 void SwWW8ImplReader::Read_Border(....) { .... if ((nBorder & WW8_LEFT)==WW8_LEFT) aBox.SetDistance( (sal_uInt16)aInnerDist.Left(), BOX_LINE_LEFT ); if ((nBorder & WW8_TOP)==WW8_TOP) aBox.SetDistance( (sal_uInt16)aInnerDist.Top(), BOX_LINE_TOP ); if ((nBorder & WW8_RIGHT)==WW8_RIGHT) aBox.SetDistance( (sal_uInt16)aInnerDist.Right(), BOX_LINE_RIGHT ); if ((nBorder & WW8_BOT)==WW8_BOT) aBox.SetDistance( (sal_uInt16)aInnerDist.Bottom(), BOX_LINE_BOTTOM ); .... }

PVS-Studio警告： V616値が0の名前付き定数「WW8_TOP」がビット演算で使用されます。 ww8par6.cxx 4742

これらは間違った行動です。 たとえば、条件（（nBorder＆WW8_TOP）== WW8_TOP）は常にtrueです。 説明のために、数字を代入します：（（nBorder＆0）== 0）。

nBorder変数に3に等しいWW8_RIGHTが含まれている場合、WW8_LEFTのチェックも失敗します。置換（（3＆1）== 1）。 WW8_RIGHTをWW8_LEFTとして受け入れます。

クローゼットの中のスケルトン


アナライザは、コード内の異常な場所を時々検出します。 これらは間違いではなく、unningなアイデアです。 それらに触れるのは意味がありませんが、見るのは面白いかもしれません。 以下に、アナライザーがfree（）関数への引数を好まなかった場合の例を示します。
 /* This operator is supposed to be unimplemented, but that now leads * to compilation and/or linking errors with MSVC2008. (Don't know * about MSVC2010.) As it can be left unimplemented just fine with * gcc, presumably it is never called. So do implement it then to * avoid the compilation and/or linking errors, but make it crash * intentionally if called. */ void SimpleReferenceObject::operator delete[](void * /* pPtr */) { free(NULL); }

安全上の注意




アナライザーは、プログラムコードを危険にする多くのポイントを特定しました。 危険は本質的に多様ですが、私はそれらを1つのセクションにまとめることにしました。
 void writeError( const char* errstr ) { FILE* ferr = getErrorFile( 1 ); if ( ferr != NULL ) { fprintf( ferr, errstr ); fflush( ferr ); } }

PVS-Studio警告： V618このような方法で「fprintf」関数を呼び出すのは危険です。渡される行には形式の仕様が含まれている可能性があるためです。 安全なコードの例：printf（ "％s"、str）; unoapploader.c 405

文字列「errstr」に制御文字が含まれている場合、何でも起こります。 プログラムがクラッシュしたり、ファイルにゴミを書き込んだり、何か他のものが発生したりすることがあります（ 詳細 ）。

このように書くのは正しいでしょう：
 fprintf( ferr, "%s", errstr );

printf（）関数が誤って使用されるさらに2つの場所：
dynamic_castの危険な使用について説明します。
 virtual ~LazyFieldmarkDeleter() { dynamic_cast<Fieldmark&> (*m_pFieldmark.get()).ReleaseDoc(m_pDoc); }

PVS-Studioの警告： V509 「dynamic_cast <T＆>」演算子は、例外を生成する可能性があるため、try..catchブロック内に配置する必要があります。 デストラクタ内で例外を発生させることは違法です。 docbm.cxx 846

リンクを操作するときに、変換を実行できない場合、dynamic_castオペレーターはstd :: bad_cast例外をスローします。

プログラムで例外が発生すると、スタックの折りたたみが開始され、その間、デストラクタを呼び出すことによりオブジェクトが破棄されます。 スタックが崩壊したときに破壊されるオブジェクトのデストラクタが別の例外をスローし、デストラクタがこの例外を残すと、C ++ライブラリはterminate（）関数を呼び出してプログラムをすぐにクラッシュさせます。 したがって、デストラクタは決して例外をスローすべきではありません。 デストラクタ内でスローされた例外は、同じデストラクタ内で処理する必要があります。

同じ理由で、デストラクタで新しい演算子を呼び出すことは危険です。 この演算子は、十分なメモリがない場合に例外std :: bad_allocをスローします。 try-catchブロックでラップすることをお勧めします。

危険なコードの例：
 WinMtfOutput::~WinMtfOutput() { mpGDIMetaFile->AddAction( new MetaPopAction() ); .... }

PVS-Studio警告：V509例外を生成する可能性があるため、「new」演算子はtry..catchブロック内に配置する必要があります。 デストラクタ内で例外を発生させることは違法です。 winmtf.cxx 852

デストラクタでの他の危険なアクション：
ところで、演算子newについて話していたので、このコードの危険性に注意します。
 extern "C" oslFileHandle SAL_CALL osl_createFileHandleFromOSHandle( HANDLE hFile, sal_uInt32 uFlags) { if ( !IsValidHandle(hFile) ) return 0; // EINVAL FileHandle_Impl * pImpl = new FileHandle_Impl(hFile); if (pImpl == 0) { // cleanup and fail (void) ::CloseHandle(hFile); return 0; // ENOMEM } .... }

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

「new」演算子は、メモリが少ない場合に例外をスローします。 したがって、オペレーターが戻したポインターを確認することは意味がありません。 常に0ではありません。十分なメモリがない場合、CloseHandle（）関数は呼び出されません。
 FileHandle_Impl * pImpl = new FileHandle_Impl(hFile); if (pImpl == 0) { // cleanup and fail (void) ::CloseHandle(hFile); return 0; // ENOMEM }

私は間違っている可能性があります。LibreOfficeプロジェクトについて知りません。おそらく開発者は、「new」演算子が例外をスローせずにnullptrを返す特別なバージョンのライブラリを使用します。この場合、番号V668の警告を無視してください。干渉しないようにオフにすることができます。

newオペレーターが例外をスローする場合、次の126メッセージを参照することをお勧めします：LibreOffice-V668.txt。

次の危険は、DllMain関数の1つの実装にあります。
 BOOL WINAPI DllMain( HINSTANCE hinstDLL, DWORD fdwReason, LPVOID lpvReserved ) { .... CreateThread( NULL, 0, ParentMonitorThreadProc, (LPVOID)dwParentProcessId, 0, &dwThreadId ); .... }

PVS-Studio警告：V718「DllMain」関数から「CreateThread」関数を呼び出さないでください。 dllentry.c 308

DllMain（）関数内で多くの関数を呼び出すことはできません。これは、アプリケーションのクラッシュやその他のエラーを引き起こす可能性があるためです。 CreateThread（）はそのような関数を指します。

DllMainの状況は、MSDNの記事：Dynamic-Link Library Best Practicesで詳しく説明されています。

このコードは機能する可能性がありますが、危険であり、いつか失敗する可能性があります。

wcsncpy（）関数がバッファオーバーフローを引き起こす可能性がある状況がありました。
 typedef struct { .... WCHAR wszTitle[MAX_COLUMN_NAME_LEN]; WCHAR wszDescription[MAX_COLUMN_DESC_LEN]; } SHCOLUMNINFO, *LPSHCOLUMNINFO; HRESULT STDMETHODCALLTYPE CColumnInfo::GetColumnInfo( DWORD dwIndex, SHCOLUMNINFO *psci) { .... wcsncpy(psci->wszTitle, ColumnInfoTable[dwIndex].wszTitle, (sizeof(psci->wszTitle) - 1)); return S_OK; }

PVS-Studio警告：V512「wcsncpy」関数を呼び出すと、バッファー「psci-> wszTitle」がオーバーフローします。columninfo.cxx 129

式（sizeof（psci-> wszTitle）-1）が正しくありません。1文字のサイズで除算するのを忘れた：
 (sizeof(psci->wszTitle) / sizeof(psci->wszTitle[0]) - 1)

このセクションでカバーする最後のタイプのエラーは、memset（）関数の呼び出しが壊れていることです。 例：
 static void __rtl_digest_updateMD2 (DigestContextMD2 *ctx) { .... sal_uInt32 state[48]; .... memset (state, 0, 48 * sizeof(sal_uInt32)); }

PVS-Studio警告：V597コンパイラーは、「状態」バッファーのフラッシュに使用される「memset」関数呼び出しを削除できました。 RtlSecureZeroMemory（）関数を使用して、プライベートデータを消去する必要があります。digest.cxx 337

この種のエラーについて何度も書いています。したがって、私はそれをほんの数語で説明し、提案されたリンクをクリックして詳細を見つけることができます。

コンパイラーは、memset（）関数の呼び出しを削除した後に、ゼロ化されたメモリーが使用されない場合、その呼び出しを削除する権利を持っています。これがここで起こることです。その結果、一部のプライベートデータがメモリに残ります。

詳細：
  1. V597 コンパイラは、「Foo」バッファをフラッシュするために使用される「memset」関数呼び出しを削除できます。
  2. メモリの上書き-なぜですか？
  3. ゼロを忘れてしまった-メモリの警告をCをゼロに。
プライベートデータがクリーンではない他の場所：LibreOfficeの-V597.txt。

その他


 Guess::Guess() { language_str = DEFAULT_LANGUAGE; country_str = DEFAULT_COUNTRY; encoding_str = DEFAULT_ENCODING; } Guess::Guess(const char * guess_str) { Guess(); .... }

PVS-Studio警告：V603オブジェクトは作成されましたが、使用されていません。コンストラクターを呼び出す場合は、「this-> Guess :: Guess（....）」を使用する必要があります。guess.cxx 56

このコードを書いたプログラマーはC ++を十分に理解していません。彼は、あるコンストラクターを別のコンストラクターから呼び出したいと考えていました。しかし、実際には、彼は一時的な名前のないオブジェクトを作成しました。エラーのため、クラスの一部のフィールドは初期化されていないままです。 詳細

別の失敗したコンストラクター：camera3d.cxx 46
 sal_uInt32 readIdent(....) { size_t nItems = rStrings.size(); const sal_Char** pStrings = new const sal_Char*[ nItems+1 ]; .... delete pStrings; return nRet; }

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

正しいでしょう：delete [] pStrings;。

不適切なメモリ割り当て解除に関する別の警告：
 static const int kConventionShift = 16; static const int kFlagMask = ~((~int(0)) << kConventionShift);

警告PVS-Studio：V610未定義の動作。シフト演算子「<<」を確認してください。左のオペランド '（〜int（0））'は負です。grammar.hxx 56

負の値のシフトにより、未定義の動作が発生します（詳細）。
 sal_Int32 GetMRest() const {return m_nRest;} OUString LwpBulletStyleMgr::RegisterBulletStyle(....) { .... if (pIndent->GetMRest() > 0.001) .... }

PVS-Studio警告：V674「double」タイプの「0.001」リテラルは、「long」タイプの値と比較されます。 「pIndent-> GetMRest（）> 0.001」式を調べることを検討してください。 lwpbulletstylemgr.cxx 177

ここで何かがおかしい。整数値を0.001と比較することは意味がありません。

戻り型との不快な混乱：
 BOOL SHGetSpecialFolderPath( HWND hwndOwner, _Out_ LPTSTR lpszPath, _In_ int csidl, _In_ BOOL fCreate ); #define FAILED(hr) (((HRESULT)(hr)) < 0) OUString UpdateCheckConfig::getDesktopDirectory() { .... if( ! FAILED( SHGetSpecialFolderPathW( .... ) ) ) .... }

PVS-Studioの警告：V716疑わしいタイプの変換：BOOL- > HRESULT。updatecheckconfig.cxx 193

プログラマーは、SHGetSpecialFolderPath（）がHRESULT型を返すことを決定しました。しかし、実際には、関数はBOOLを返します。コードを修正するには、条件からFAILEDマクロを削除します。

別のそのようなエラー：updatecheckconfig.cxx 222

そして、ここでは、反対に、FAILEDマクロがありません。したがって、タイプHRESULTのステータスを確認することはできません。
 bool UniscribeLayout::LayoutText( ImplLayoutArgs& rArgs ) { .... HRESULT nRC = ScriptItemize(....); if( !nRC ) // break loop when everything is correctly itemized break; .... }

PVS-Studio警告：V545このような「if」演算子の条件式は、HRESULT型の値「nRC」に対して正しくありません。代わりにSUCCEEDEDまたはFAILEDマクロを使用する必要があります。winlayout.cxx 1115

ここでは、コンマをセミコロンに置き換える必要があると思います。
 void Reader::ClearTemplate() { if( pTemplate ) { if( 0 == pTemplate->release() ) delete pTemplate, pTemplate = 0; } }

PVS-Studio警告：V626ミスプリントのチェックを検討してください。「、」を「;」に置き換える必要がある場合があります。shellio.cxx 549

面白くない些細なこと：
 void TabBar::ImplInit( WinBits nWinStyle ) { .... mbMirrored = false; mbMirrored = false; .... }

PVS-Studio警告：V519「mbMirrored」変数には、連続して2回値が割り当てられます。 おそらくこれは間違いです。行をチェックしてください：415、416

。おそらくこれは間違いです。行を確認してください：4561、4562。output2.cxx 4562

文字列の長さを示すマジック定数が正しくありません：
 static bool CallRsc2(....) { .... if( !rsc_strnicmp( ...., "-fp=", 4 ) || !rsc_strnicmp( ...., "-fo=", 4 ) || !rsc_strnicmp( ...., "-presponse", 9 ) || <<<<---- !rsc_strnicmp( ...., "-rc", 3 ) || !rsc_stricmp( ...., "-+" ) || !rsc_stricmp( ...., "-br" ) || !rsc_stricmp( ...., "-bz" ) || !rsc_stricmp( ...., "-r" ) || ( '-' != *.... ) ) .... }

PVS-Studio警告：V666関数「rsc_strnicmp」の3番目の引数を調べることを検討してください。値が、2番目の引数で渡された文字列の長さと一致しない可能性があります。start.cxx 179

文字列「-presponse」の長さは9文字ではなく10文字です。

ループ内の奇妙な「ブレーク」：
 OUString getExtensionFolder(....) { .... while (xResultSet->next()) { title = Reference<sdbc::XRow>( xResultSet, UNO_QUERY_THROW )->getString(1 /* Title */ ) ; break; } return title; }

PVS-Studio警告：V612ループ内の無条件の「ブレーク」。dp_manager.cxx 100

さらに3つの奇妙なサイクル：
可能性の低いヌルポインターの逆参照：
 BSTR PromptNew(long hWnd) { .... ADOConnection* piTmpConnection = NULL; ::CoInitialize( NULL ); hr = CoCreateInstance( CLSID_DataLinks, NULL, CLSCTX_INPROC_SERVER, IID_IDataSourceLocator, (void**)&dlPrompt ); if( FAILED( hr ) ) { piTmpConnection->Release(); dlPrompt->Release( ); return connstr; } .... }

PVS-Studio警告：V522ヌルポインター「piTmpConnection」の逆参照が行われる場合があります。adodatalinks.cxx 84

CoCreateInstance（）関数が突然エラーステータスを返す場合、ポインタはNULLの「piTmpConnection」で逆参照されます。

ミクロ最適化


静的アナライザーは、プロファイリングツールの代わりにはなりません。プロファイラーのみが、プログラムを最適化する価値がある場所を示すことができます。

それでも、静的アナライザーは、コード内の簡単に改善できる場所を表示できます。これからプログラムを高速で実行する必要はありません。しかし、それは確かに悪化しません。おそらく、それは優れたコーディングスタイルに関するものです。

PVS-Studioがマイクロ最適化に関して提供する推奨事項を見てみましょう。

参照によるオブジェクトの受け渡し


関数に渡されたオブジェクトが変更されない場合、審美的には値ではなく参照によって渡されます。もちろん、これはすべてのオブジェクトに適用されるわけではありません。ただし、たとえば文字列を処理している場合、メモリを割り当てて文字列の内容を無駄にコピーしても意味がありません。



例：
 string getexe(string exename, bool maybeempty) { char* cmdbuf; size_t cmdlen; _dupenv_s(&cmdbuf, &cmdlen, exename.c_str()); if(!cmdbuf) { if (maybeempty) { return string(); } cout << "Error " << exename << " not defined. " "Did you forget to source the environment?" << endl; exit(1); } string command(cmdbuf); free(cmdbuf); return command; }

オブジェクト「exename」は読み取り専用です。したがって、アナライザーは次を報告します。V813パフォーマンスの低下。「exename」引数は、おそらく定数参照としてレンダリングされる必要があります。wrapper.cxx 18

関数宣言は次のように変更できます。
 string getexe(const string &exename, bool maybeempty)

通常、定数参照を介して複雑なオブジェクトを渡す方が効率的であり、「クリッピング」問題を回避します。本質を十分に理解していない人のために、私は本からのルール20「価値による伝達によるconstへの参照による伝達を好む」を参照することを提案し

ます。プログラムの構造とコードを改善する55の確実な方法”-M .: DMK Press、2006.-300 pp。、Ill。ISBN 5-94074-304-8

別の関連する診断はV801です。アナライザーは合計で465の警告を発行しましたが、彼の意見では、参照によってオブジェクトを転送することは可能です：LibreOffice-V801-V813.txt。

プレフィックスインクリメントを使用する


イテレータの場合、接頭辞の増分操作はわずかに高速です。これについての詳細は、「

Meyers S. C ++の最も効率的な使用」の本の「ルール6.インクリメント演算子とデクリメント演算子のプレフィックス形式を区別する」を参照してください。プログラムとプロジェクトを改善するための35の新しい推奨事項：Per。英語から-M .: DMK Press、2000 .-- 304 p。：病気 （「プログラマー向け」シリーズ）。ISBN 5-94074-033-2。BBK 32.973.26-018.1。

推奨事項はとてつもないように思われ、実際には「A ++」と「++ A」の間に違いはありません。この問題を研究し、実験を行ったところ、この推奨事項を適用する必要があると考えています（詳細）。

コード例：
 typename InterfaceMap::iterator find(const key &rKey) const { typename InterfaceMap::iterator iter = m_pMap->begin(); typename InterfaceMap::iterator end = m_pMap->end(); while( iter != end ) { equalImpl equal; if( equal( iter->first, rKey ) ) break; iter++; } return iter; }

警告PVS_Studio：V803パフォーマンスの低下。「iter」がイテレータの場合、接頭辞形式のインクリメントを使用する方が効果的です。イテレータ++を++イテレータに置き換えます。interfacecontainer.h 405

「iter ++」という表現は「++ iter」に置き換える必要があります。開発者がこれに時間を費やすことが合理的であると考えているかどうかはわかりません。何を修正するかを決定すると、後置増分を接頭辞増分LibreOffice-V803.txtで置き換えることができる場所がさらに257個あります。

文字列が空であることを確認してください


文字列が空であることを確認するには、その長さを計算する必要はありません。非効率的なコードの例：
 BOOL GetMsiProp(....) { .... char* buff = reinterpret_cast<char*>( malloc( nbytes ) ); .... return ( strlen(buff) > 0 ); }

警告PVS-Studio：V805パフォーマンスの低下。'strlen（str）> 0'コンストラクトを使用して空の文字列を識別することは非効率的です。より効率的な方法は、チェックすることです：str [0]！= '\ 0'。sellang.cxx 49

非効率性は、終端のゼロに遭遇するまで文字列内のすべての文字を反復処理する必要があることです。ただし、1バイトのみをチェックするだけで十分です。
 return buff[0] != '\0';

このコードはあまり美しくありませんので、特別な機能を持つ方が良いでしょう：
 inline bool IsEmptyStr(const char *s) { return s == nullptr || s[0] == '\0'; }

ここでは、ゼロへのポインターの等価性に関する追加のチェックがありました。私はこれがあまり好きではないので、他のオプションについて考えることができます。それでも、この関数はstrlen（）よりも速く動作します。

その他の無効なチェック：LibreOffice-V805.txt。

その他


興味深いと思われるアナライザーの警告がさらにいくつかあります：LibreOffice-V804_V811.txt。

誤検知の数


記事の中で、私は240の警告について言及しました。合計で、アナライザーは約1,500の一般警告（GA）レベル1および2のアラートを生成しました。これは、アナライザーが多くの偽陽性を生成することを意味しますか？いやほとんどの警告は完全にそのケースに当てはまりますが、記事でそれらについて説明する意味はありません。

時々、ユーザーから「PVS-Studioアナライザーは誤検知をほとんど発生させず、非常に便利です」という肯定的なフィードバックを受け取ります。また、誤検知はほとんどないと考えています。しかし、どのように？この記事では、投稿の16％しか説明していません。残りは何ですか？誤検知ですか？

もちろん、誤検知があります。これから逃れることはできません。それらを抑える多くのメカニズムがあります。ただし、ほとんどの警告はエラーを検出していませんが、悪臭を放つコードを示しています。例で説明しようと思います。

アナライザーは、V690に対して206個の警告を発行しました。コピーコンストラクタはクラスに実装されていますが、代入演算子は実装されていません。これらのクラスの1つを次に示します。
 class RegistryTypeReader { public: .... inline RegistryTypeReader(const RegistryTypeReader& toCopy); .... }; inline RegistryTypeReader::RegistryTypeReader(const RegistryTypeReader& toCopy) : m_pApi(toCopy.m_pApi) , m_hImpl(toCopy.m_hImpl) { m_pApi->acquire(m_hImpl); }

高い確率で間違いはありません。ほとんどの場合、演算子=は206クラスすべてで使用されているわけではありません。使用されたらどうなりますか？

ここでは、プログラマーが選択する必要があります。

コードが危険であると考える場合、割り当て演算子を実装するか、禁止する必要があります。彼の意見で危険がなければ、彼はV690診断をオフにすることができ、疑わしい場所のリストはすぐに206警告を失います。

別の例。 この記事の前半で、次の疑わしいスニペットが言及されました。
 if( pInit ) AddMap( pInit ); if( pInit ) AddMap( pInit2 );

V581診断を使用して検出されました。しかし、正直に言うと、私はV581の警告を非常に表面的に見ており、何かを見逃していた可能性があります。実際には、さらに70あります。そして、アナライザーのせいではありません。このように書く理由を彼はどのようにして知ったのですか？
 static bool lcl_parseDate(....) { bool bSuccess = true; .... if (bSuccess) { ++nPos; } if (bSuccess) { bSuccess = readDateTimeComponent(string, nPos, nDay, 2, true); .... }

2回「bSuccess」がチェックされます。突然、2回目に別の変数をチェックする必要がありますか？

このような70個の警告をどうするかは、プログラマーが再度決定する必要があります。いくつかの論理ブロックを分離するために複数の同一のチェックを行うのが好きな場合、アナライザーは確かに正しくありません。 V581診断をオフにする必要があり、すぐに70個の警告が消えます。

プログラマーが自分にそれほど自信がない場合は、何かをしなければなりません。コードをリファクタリングできます：
 static bool lcl_parseDate(....) { bool bSuccess = true; .... if (bSuccess) { ++nPos; bSuccess = readDateTimeComponent(string, nPos, nDay, 2, true); .... }

最も重要なことは、誤検知に関して深刻な問題がないことです。警告のグループが彼のプロジェクトの人にとって無意味であると思われる場合、彼は単にそれらを無効にし、研究すべき警告の数を大幅に減らします。彼の意見では、コードを調べて修正する必要がある場合、これらは誤ったアラームではなく、実際に役立つ警告です。

ご注意数百または数千のアラートを見なくてもアナライザーの使用を開始できます。新しいメッセージマークアップメカニズムを使用できます。存在するすべての警告を非表示にし、新しいコードに表示されるメッセージのみを確認する必要があります。また、緊急の問題が発生することなく、すぐに古いコードのエラーに戻ることができます。

おわりに


いつものように、私の記事には多くのエラー、欠点、誤りがリストされていますが、LibreOfficeコードは非常に高品質です。はい。コベリティを定期的に使用することは、開発に対する真剣なアプローチを示しています。プロジェクトの場合、このエラーの量はごくわずかです。

この記事で何を言いたいですか？はい、一般的には何もありません。ちょっとした広告など。PVS-Studio静的アナライザーを定期的に使用すると、初期段階で多くの失敗が見つかります。



私は最後の写真の牛のようです。彼は来て、たくさんの間違いを重ねて逃げました。そして、LibreOfficeの作者は今それらをかき集めています。ごめんなさい それが私の仕事です。

この記事は英語です。


この記事を英語圏の聴衆と共有したい場合は、翻訳へのリンクを使用してください：Andrey Karpov。 LibreOfficeプロジェクトのチェック。

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

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

More articles:


All Articles