LibreOffice：会計士の悪夢

LibreOfficeは、個人、教育、および商業目的で無料で使用できる強力なオフィススイートです。 その開発者は、多くの分野でMicrosoft Officeの代替として使用される素晴らしい製品を作っています。 PVS-Studioチームは、このようなよく知られているプロジェクトのコードを見て、エラーを見つけようと常に心がけています。 今回は簡単でした。 プロジェクトには、深刻な問題を引き起こす可能性のある多くのエラーが含まれています。 この記事では、コードで見つかったいくつかの興味深い欠陥について説明します。

はじめに

LibreOfficeは非常に大きなC ++プロジェクトです。 このサイズのプロジェクトを維持することは、開発チームにとって難しいタスクです。 そして、残念ながら、LibreOfficeのコード品質が十分な注意を払っていないという印象を受けます。



一方では、プロジェクトは単純に巨大であり、すべての静的または動的分析ツールが13kソースコードファイルの分析を処理できるわけではありません。 非常に多くのファイルが、サードパーティのライブラリとともにオフィススイートの構築に関与しています。 メインのLibreOfficeリポジトリには、約8kのソースコードファイルが保存されます。 この量のコードは、開発者だけでなく問題を引き起こします。

一方、プロジェクトには多くのユーザーがいて、できるだけ多くのエラーを見つけて修正する必要があります。 すべての間違いは何百、何千ものユーザーを傷つける可能性があります。 したがって、コードベースのサイズが大きいことは、エラーを検出できる特定のツールの使用を拒否する言い訳になるべきではありません。 読者はすでに静的コードアナライザーについて話していると推測していると思います:)。



はい、静的アナライザーを使用しても、プロジェクトにエラーがないことを保証しません。 ただし、PVS-Studioなどのツールは、開発段階でも多数のエラーを検出できるため、デバッグおよびプロジェクトサポートに関連する作業量を削減できます。



PVS-Studio静的コードアナライザーを使用する場合、LibreOfficeのソースコードで何がおもしろいと思うか見てみましょう。 アナライザーを起動する可能性は広範囲にわたります：Windows、Linux、macOS。 このレビューを書くために、Windowsでのプロジェクトの分析中に作成されたPVS-Studioレポートが使用されました。

2015年の最後のチェック以降の変更

2015年3月、最初のLibreOffice分析（「 LibreOfficeプロジェクトの検証 」）がPVS-Studioを使用して実行されました。 それ以来、オフィススイートは製品として大きく進化しましたが、内部には多くのエラーが含まれています。 また、それ以降、一部のエラーパターンはまったく変更されていません。 ここで、たとえば、最初の記事のエラー：



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

void ViewContactOfE3dScene::createViewInformation3D(....) { .... const basegfx::B3DPoint aVRP(rSceneCamera.GetVRP()); const basegfx::B3DVector aVPN(rSceneCamera.GetVRP()); // <= const basegfx::B3DVector aVUV(rSceneCamera.GetVUV()); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このバグは修正されましたが、最新バージョンのコードで見つかったものは次のとおりです。



V656変数「aSdvURL」、「aStrURL」は、同じ関数の呼び出しによって初期化されます。 おそらくエラーまたは最適化されていないコードです。 「pThm-> GetSdvURL（）」式の検査を検討してください。 行を確認してください：658、659。gallery1.cxx 659

 const INetURLObject& GetThmURL() const { return aThmURL; } const INetURLObject& GetSdgURL() const { return aSdgURL; } const INetURLObject& GetSdvURL() const { return aSdvURL; } const INetURLObject& GetStrURL() const { return aStrURL; } bool Gallery::RemoveTheme( const OUString& rThemeName ) { .... INetURLObject aThmURL( pThm->GetThmURL() ); INetURLObject aSdgURL( pThm->GetSdgURL() ); INetURLObject aSdvURL( pThm->GetSdvURL() ); INetURLObject aStrURL( pThm->GetSdvURL() ); // <= .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

お気づきかもしれませんが、微妙な複合関数名は依然としてエラーの原因です。



古いコードからの別の興味深い例：



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

 class VCL_DLLPUBLIC MouseSettings { .... long GetStartDragWidth() const; long GetStartDragHeight() const; .... } bool ImplHandleMouseEvent( .... ) { .... long nDragW = rMSettings.GetStartDragWidth(); long nDragH = rMSettings.GetStartDragWidth(); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このコードには、実際には修正されたバグが含まれていました。 しかし、コードのエラーは小さくなっていません...同様の状況が特定されました。



V656変数「defaultZoomX」、「defaultZoomY」は、同じ関数の呼び出しによって初期化されます。 おそらくエラーまたは最適化されていないコードです。 「pViewData-> GetZoomX（）」式の検査を検討してください。 行を確認してください：5673、5744。gridwin.cxx 5674

 OString ScGridWindow::getCellCursor(....) const { .... SCCOL nX = pViewData->GetCurX(); SCROW nY = pViewData->GetCurY(); Fraction defaultZoomX = pViewData->GetZoomX(); Fraction defaultZoomY = pViewData->GetZoomX(); // <= .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

エラーは類推によって文字通りコードに導入されます。

だまされてはいけません

V765複合代入式「x-= x -... 」は疑わしいです。 エラーの可能性を調べることを検討してください。 swdtflvr.cxx 3509

 bool SwTransferable::PrivateDrop(...) { .... if ( rSrcSh.IsSelFrameMode() ) { //Hack: fool the special treatment aSttPt -= aSttPt - rSrcSh.GetObjRect().Pos(); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V765診断を使用して発見されたこのような興味深い「ハック」は次のとおりです。 コメントを使用してコード行を単純化すると、予期しない結果が生じる可能性があります。



最初のステップ：

 aSttPt = aSttPt - (aSttPt - rSrcSh.GetObjRect().Pos());
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

第2ステップ：

 aSttPt = aSttPt - aSttPt + rSrcSh.GetObjRect().Pos();
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

第三段階

 aSttPt = rSrcSh.GetObjRect().Pos();
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

そして、ハックとは何ですか？



このトピックの別の例：



V567 「nCount」変数の変更は、同じ変数に対する別の操作に対して順序付けられていません。 これにより、未定義の動作が発生する場合があります。 stgio.cxx 214

 FatError EasyFat::Mark(....) { if( nCount > 0 ) { --nCount /= GetPageSize(); nCount++; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このような状況でのコードの実行は、コンパイラと言語標準に依存する場合があります。 このコードスニペットを、よりシンプルで明確で信頼性の高い方法で書き換えてみませんか？

配列とベクトルを使用しない方法

何らかの理由で、配列とベクターを操作するときに誰かが同様のエラーをたくさん犯しました。 これらの例を見てみましょう。



V557配列のオーバーランが可能です。 「nPageNum」インデックスは、配列の境界を超えています。 pptx-epptooxml.cxx 1168

 void PowerPointExport::ImplWriteNotes(sal_uInt32 nPageNum) { .... // add slide implicit relation to notes if (mpSlidesFSArray.size() >= nPageNum) addRelation(mpSlidesFSArray[ nPageNum ]->getOutputStream(), oox::getRelationship(Relationship::NOTESSLIDE), OUStringBuffer() .append("../notesSlides/notesSlide") .append(static_cast<sal_Int32>(nPageNum) + 1) .append(".xml") .makeStringAndClear()); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

最後の有効なインデックスは、 size（）-1に等しい値でなければなりません。 ただし、このコードフラグメントでは、 nPageNumインデックスが値mpSlidesFSArray.size（）を持つことができる状況が許可されました。これは、範囲外の配列があり、「ガベージ」で構成される要素を処理するためです。



V557配列のオーバーランが可能です。 「mnSelectedMenu」インデックスは配列の境界を超えています。 checklistmenu.cxx 826

 void ScMenuFloatingWindow::ensureSubMenuNotVisible() { if (mnSelectedMenu <= maMenuItems.size() && maMenuItems[mnSelectedMenu].mpSubMenuWin && maMenuItems[mnSelectedMenu].mpSubMenuWin->IsVisible()) { maMenuItems[mnSelectedMenu].mpSubMenuWin->ensureSubMenuNotVisible(); } EndPopupMode(); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

興味深いことに、このコードフラグメントでは、インデックスチェックをより明確に記述しましたが、同時に同じ間違いを犯しました。



V557配列のオーバーランが可能です。 「nXFIndex」インデックスは、配列の境界を超えています。 xestyle.cxx 2613

 sal_Int32 XclExpXFBuffer::GetXmlStyleIndex( sal_uInt32 nXFIndex ) const { OSL_ENSURE( nXFIndex < maStyleIndexes.size(), "...." ); if( nXFIndex > maStyleIndexes.size() ) return 0; // should be caught/debugged via above assert; return maStyleIndexes[ nXFIndex ]; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

そして、この間違いは二重に興味深いです！ デバッグマクロでは、正しいインデックスチェックを作成し、別の場所で再びミスを犯し、配列の外に出ることを許可しました。



次に、インデックスに関連しない別の種類のエラーについて考えてみましょう。



V554 shared_ptrの誤った使用。 「new []」で割り当てられたメモリは、「delete」を使用して消去されます。 dx_vcltools.cxx 158

 struct RawRGBABitmap { sal_Int32 mnWidth; sal_Int32 mnHeight; std::shared_ptr< sal_uInt8 > mpBitmapData; }; RawRGBABitmap bitmapFromVCLBitmapEx( const ::BitmapEx& rBmpEx ) { .... // convert transparent bitmap to 32bit RGBA // ======================================== const ::Size aBmpSize( rBmpEx.GetSizePixel() ); RawRGBABitmap aBmpData; aBmpData.mnWidth = aBmpSize.Width(); aBmpData.mnHeight = aBmpSize.Height(); aBmpData.mpBitmapData.reset( new sal_uInt8[ 4*aBmpData.mnWidth *aBmpData.mnHeight ] ); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このコードには、未定義のプログラムの動作につながるエラーが含まれています。 実際には、メモリはさまざまな方法で割り当てられ、解放されます。 メモリを適切に解放するには、次のようなクラスフィールドを宣言する必要がありました。

 std::shared_ptr< sal_uInt8[] > mpBitmapData;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

マクロを2回作成する方法

V568 sizeof（）演算子の引数が 'bTextFrame？ aProps：aShapePropsの式。 wpscontext.cxx 134

 oox::core::ContextHandlerRef WpsContext::onCreateContext(....) { .... OUString aProps[] = { .... }; OUString aShapeProps[] = { .... }; for (std::size_t i = 0; i < SAL_N_ELEMENTS(bTextFrame ? aProps : aShapeProps); //1 ++i) if (oInsets[i]) xPropertySet->setPropertyValue((bTextFrame ? aProps : aShapeProps)[i], //2 uno::makeAny(*oInsets[i])); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

多くの開発者にとって残念なことに、マクロ引数は関数引数のようには動作しません。 この事実を無視すると、多くの場合エラーが発生します。 ケース＃1と＃2では、三項演算子を使用してほぼ同じ構造が使用されます。 しかし、最初の場合-マクロ、2番目の場合-関数。 ただし、これは問題のトップにすぎません。



ケース1では、アナライザーは実際に次のエラーコードを検出しました。

 for (std::size_t i = 0; i < (sizeof (bTextFrame ? aProps : aShapeProps) / sizeof ((bTextFrame ? aProps : aShapeProps)[0])); ++i)
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

これは、 SAL_N_ELEMENTSマクロを使用したループです。 sizeof演算子は、三項演算子の式を評価しません。 この場合、算術はポインターのサイズを使用して実行されます。その結果は、示された配列の実際のサイズからかけ離れた値になります。 誤った値の計算は、アプリケーションのビット深度によってさらに影響を受けます。



しかし、その後2つのSAL_N_ELEMENTSマクロがあることが判明しました！ つまり プリプロセッサが間違ったマクロを開いたのですが、これはどのように起こりますか？ マクロ定義と開発者のコ​​メントは私たちを助けます：

 #ifndef SAL_N_ELEMENTS # if defined(__cplusplus) && ( defined(__GXX_EXPERIMENTAL_CXX0X__) || __cplusplus >= 201103L ) /* * Magic template to calculate at compile time the number of elements * in an array. Enforcing that the argument must be a array and not * a pointer, eg * char *pFoo="foo"; * SAL_N_ELEMENTS(pFoo); * fails while * SAL_N_ELEMENTS("foo"); * or * char aFoo[]="foo"; * SAL_N_ELEMENTS(aFoo); * pass * * Unfortunately if arr is an array of an anonymous class then we need * C++0x, ie see * http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#757 */ template <typename T, size_t S> char (&sal_n_array_size( T(&)[S] ))[S]; # define SAL_N_ELEMENTS(arr) (sizeof(sal_n_array_size(arr))) # else # define SAL_N_ELEMENTS(arr) (sizeof (arr) / sizeof ((arr)[0])) # endif #endif
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

マクロの別のバージョンには安全なテンプレート関数が含まれていますが、何か問題がありました。

1. 安全なマクロはコードに含まれていません。
2. 別のマクロを使用することはまだ不可能です。 テンプレート関数の正常なインスタンス化は、同じサイズの配列が三項演算子に渡される場合にのみ実行されます。 そしてこの場合、そのようなマクロの使用はその意味を失います。

入力ミスとコピーアンドペースト

V1013疑わしい部分式f1.Pitch == f2.CharSet同様の比較のシーケンス。 xmldlg_export.cxx 1251

 inline bool equalFont( Style const & style1, Style const & style2 ) { awt::FontDescriptor const & f1 = style1._descr; awt::FontDescriptor const & f2 = style2._descr; return ( f1.Name == f2.Name && f1.Height == f2.Height && f1.Width == f2.Width && f1.StyleName == f2.StyleName && f1.Family == f2.Family && f1.CharSet == f2.CharSet && // <= f1.Pitch == f2.CharSet && // <= f1.CharacterWidth == f2.CharacterWidth && f1.Weight == f2.Weight && f1.Slant == f2.Slant && f1.Underline == f2.Underline && f1.Strikeout == f2.Strikeout && f1.Orientation == f2.Orientation && bool(f1.Kerning) == bool(f2.Kerning) && bool(f1.WordLineMode) == bool(f2.WordLineMode) && f1.Type == f2.Type && style1._fontRelief == style2._fontRelief && style1._fontEmphasisMark == style2._fontEmphasisMark ); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このエラーは、「 悪が比較機能に住んでいる 」という記事を更新または拡張することに決めた場合に、更新する価値のある候補です。 そのようなエラー（パスf2.Pitch ）を見つける可能性は、それ自体では非常に小さいと思います。 どう思いますか？



V501 「&&」演算子の左側と右側には、同一のサブ式「mpTable [ocArrayColSep]！= MpTable [eOp]」があります。 formulacompiler.cxx 632

 void FormulaCompiler::OpCodeMap::putOpCode(....) { .... case ocSep: bPutOp = true; bRemoveFromMap = (mpTable[eOp] != ";" && mpTable[ocArrayColSep] != mpTable[eOp] && mpTable[ocArrayColSep] != mpTable[eOp]); break; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

軽率なコピーの結果は、そのようなコードでした。 おそらく、条件式はもう一度単純に複製されますが、それでもコードにはそのようなあいまいさの場所はありません。



V517 「if（A）{...} else if（A）{...}」パターンの使用が検出されました。 論理エラーが存在する可能性があります。 行を確認します：781、783。mysqlc_databasemetadata.cxx 781

 Reference<XResultSet> SAL_CALL ODatabaseMetaData::getColumns(....) { .... bool bIsCharMax = !xRow->wasNull(); if (sDataType.equalsIgnoreAsciiCase("year")) nColumnSize = sColumnType.copy(6, 1).toInt32(); else if (sDataType.equalsIgnoreAsciiCase("date")) // <= nColumnSize = 10; else if (sDataType.equalsIgnoreAsciiCase("date")) // <= nColumnSize = 8; else if (sDataType.equalsIgnoreAsciiCase("datetime") || sDataType.equalsIgnoreAsciiCase("timestamp")) nColumnSize = 19; else if (!bIsCharMax) nColumnSize = xRow->getShort(7); else nColumnSize = nCharMaxLen; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

条件式をコピーした結果、コードでエラーが発生しました。そのため、変数nColumnSizeの値8は設定されません。



V523 「then」ステートメントは「else」ステートメントと同等です。 svdpdf.hxx 146

 /// Transform the rectangle (left, right, top, bottom) by this Matrix. template <typename T> void Transform(....) { .... if (top > bottom) top = std::max(leftTopY, rightTopY); else top = std::min(leftTopY, rightTopY); if (top > bottom) bottom = std::max(leftBottomY, rightBottomY); // <= else bottom = std::max(leftBottomY, rightBottomY); // <= }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ここで、関数min（）とmax（）は混同されています。 確かにインターフェイスのこのタイプミスのために、何かが奇妙にスケーリングされます。

奇妙なサイクル

V533 「for」演算子内で誤った変数がインクリメントされている可能性があります。 「i」の検討を検討してください。 javatypemaker.cxx 602

 void printConstructors(....) { .... for (std::vector< unoidl::SingleInterfaceBasedServiceEntity::Constructor:: Parameter >::const_iterator j(i->parameters.begin()); j != i->parameters.end(); ++i) { o << ", "; printType(o, options, manager, j->type, false); if (j->rest) { o << "..."; } o << ' ' << codemaker::java::translateUnoToJavaIdentifier( u2b(j->name), "param"); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ループ内の式++ iは非常に疑わしく見えます。 たぶん++ jがあるはずです。



V756 「nIndex2」カウンターは、ネストされたループ内では使用されません。 「nIndex」カウンターの使用を検査することを検討してください。 treex.cxx 34

 SAL_IMPLEMENT_MAIN_WITH_ARGS(argc, argv) { OString sXHPRoot; for (int nIndex = 1; nIndex != argc; ++nIndex) { if (std::strcmp(argv[nIndex], "-r") == 0) { sXHPRoot = OString( argv[nIndex + 1] ); for( int nIndex2 = nIndex+3; nIndex2 < argc; nIndex2 = nIndex2 + 2 ) { argv[nIndex-3] = argv[nIndex-1]; argv[nIndex-2] = argv[nIndex]; } argc = argc - 2; break; } } common::HandledArgs aArgs; if( !common::handleArguments(argc, argv, aArgs) ) { WriteUsage(); return 1; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

内側の forループに何らかのエラーがあります。 なぜなら nIndex変数は変更されず、配列の同じ2つの要素が各反復で上書きされます。 ほとんどの場合、 nIndexの代わりに、変数nIndex2がどこでも使用されているはずです。



V1008 「for」演算子の検査を検討してください。 ループの反復は1回しか実行されません。 diagramhelper.cxx 292

 void DiagramHelper::setStackMode( const Reference< XDiagram > & xDiagram, StackMode eStackMode ) { .... sal_Int32 nMax = aChartTypeList.getLength(); if( nMax >= 1 ) nMax = 1; for( sal_Int32 nT = 0; nT < nMax; ++nT ) { uno::Reference< XChartType > xChartType( aChartTypeList[nT] ); .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

forループは、意図的に1回の反復に制限されています。 これがなぜこのように行われるのかは明らかではありません。



V612ループ内の無条件の「戻り」。 pormulti.cxx 891

 SwTextAttr const* MergedAttrIterMulti::NextAttr(....) { .... SwpHints const*const pHints(m_pNode->GetpSwpHints()); if (pHints) { while (m_CurrentHint < pHints->Count()) { SwTextAttr const*const pHint(pHints->Get(m_CurrentHint)); ++m_CurrentHint; rpNode = m_pNode; return pHint; } } return nullptr; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

1つの反復からのより単純なストレンジループの例。条件付きステートメントに書き直す方が適切です。



さらにいくつかのそのような場所：

• V612ループ内の無条件の「戻り」。 txtfrm.cxx 144
• V612ループ内の無条件の「戻り」。 txtfrm.cxx 202
• V612ループ内の無条件の「戻り」。 txtfrm.cxx 279

奇妙な条件

V637反対の2つの条件が発生しました。 2番目の条件は常にfalseです。 行を確認：281、285。authfld.cxx 281

 sal_uInt16 SwAuthorityFieldType::GetSequencePos(sal_IntPtr nHandle) { .... SwTOXSortTabBase* pOld = aSortArr[i].get(); if(*pOld == *pNew) { //only the first occurrence in the document //has to be in the array if(*pOld < *pNew) pNew.reset(); else // remove the old content aSortArr.erase(aSortArr.begin() + i); break; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーは競合する比較を見つけました。 このコードには何か明らかに問題があります。



同じ場所にこのコードがあります：

• V637反対の2つの条件が発生しました。 2番目の条件は常にfalseです。 行を確認してください：1827、1829。doctxm.cxx 1827

V590この表現を調べることを検討してください。 表現が過剰であるか、誤植が含まれています。 fileurl.cxx 55

 OUString convertToFileUrl(char const * filename, ....) { .... if ((filename[0] == '.') || (filename[0] != SEPARATOR)) { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

上記のコードフラグメントの問題は、最初の条件式が式全体の結果に影響を与えないことです。



このようなエラーに基づいて、理論的な記事である「 C / C ++の論理式。専門家はどのように間違っているか」まで書きました 。



V590この表現を調べることを検討してください。 表現が過剰であるか、誤植が含まれています。 unoobj.cxx 1895

 uno::Sequence< beans::PropertyState > SwUnoCursorHelper::GetPropertyStates(....) { .... if (((SW_PROPERTY_STATE_CALLER_SWX_TEXT_PORTION == eCaller) || (SW_PROPERTY_STATE_CALLER_SWX_TEXT_PORTION_TOLERANT == eCaller)) && pEntry->nWID < FN_UNO_RANGE_BEGIN && pEntry->nWID > FN_UNO_RANGE_END && pEntry->nWID < RES_CHRATR_BEGIN && pEntry->nWID > RES_TXTATR_END ) { pStates[i] = beans::PropertyState_DEFAULT_VALUE; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

この状態の問題が何であるかすぐにはわかりません。したがって、前処理されたファイルから詳細なコードフラグメントが書き出されました。

 if (((SW_PROPERTY_STATE_CALLER_SWX_TEXT_PORTION == eCaller) || (SW_PROPERTY_STATE_CALLER_SWX_TEXT_PORTION_TOLERANT == eCaller)) && pEntry->nWID < (20000 + 1600) && pEntry->nWID > ((20000 + 2400) + 199) && pEntry->nWID < 1 && pEntry->nWID > 63 ) { pStates[i] = beans::PropertyState_DEFAULT_VALUE; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

条件によって番号で指定された4つの範囲に同時に含まれる数値は1つではないことが判明しました。 開発者は間違いを犯しました。



V590 「* pData <= MAXLEVEL && * pData <= 9」式の検査を検討してください。 表現が過剰であるか、誤植が含まれています。 ww8par2.cxx 756

 const sal_uInt8 MAXLEVEL = 10; void SwWW8ImplReader::Read_ANLevelNo(....) { .... // Range WW:1..9 -> SW:0..8 no bullets / numbering if (*pData <= MAXLEVEL && *pData <= 9) { .... } else if( *pData == 10 || *pData == 11 ) { // remember type, the rest happens at Sprm 12 m_xStyles->mnWwNumLevel = *pData; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

最初の条件は値10の定数を使用するという事実により、条件は冗長であることが判明しました。 このコードは次のように書き換えることができます。

 if (*pData <= 9) { .... } else if( *pData == 10 || *pData == 11 ) { .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

しかし、おそらく提示されたコードに別の問題があった。



V757 「dynamic_cast」を使用した型変換後、誤った変数がnullptrと比較される可能性があります。 行を確認してください：2709、2710。menu.cxx 2709

 void PopupMenu::ClosePopup(Menu* pMenu) { MenuFloatingWindow* p = dynamic_cast<MenuFloatingWindow*>(ImplGetWindow()); PopupMenu *pPopup = dynamic_cast<PopupMenu*>(pMenu); if (p && pMenu) p->KillActivePopup(pPopup); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ほとんどの場合、条件にエラーが含まれています。 ポインターpおよびpPopupを確認する必要がありました 。



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

 ZipFile::ZipFile(const std::wstring &FileName) : m_pStream(nullptr), m_bShouldFree(true) { m_pStream = new FileStream(FileName.c_str()); if (m_pStream && !isZipStream(m_pStream)) { delete m_pStream; m_pStream = nullptr; } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーは、 newオペレーターによって返されたポインターの値がゼロと比較される状況を検出しました。 C ++言語標準によれば、メモリ割り当てが不可能な場合、 new演算子は例外std :: bad_allocをスローします。 LibreOfficeプロジェクトでは、このような場所が45個しか見つかりませんでした。これは、このような大量のコードに対しては非常に小さいものです。 それでも、これはユーザーの問題につながる可能性があります。 開発者は、次の方法で不要なチェックを削除するか、オブジェクトを作成する必要があります。

 m_pStream = new (std::nothrow) FileStream(FileName.c_str());
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V728過剰なチェックを簡素化できます。 '（A &&！B）|| （！A && B） '式は、' bool（A）！= Bool（B） '式と同等です。 toolbox2.cxx 1042

 void ToolBox::SetItemImageMirrorMode( sal_uInt16 nItemId, bool bMirror ) { ImplToolItems::size_type nPos = GetItemPos( nItemId ); if ( nPos != ITEM_NOTFOUND ) { ImplToolItem* pItem = &mpData->m_aItems[nPos]; if ((pItem->mbMirrorMode && !bMirror) || // <= (!pItem->mbMirrorMode && bMirror)) // <= { .... } } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

かなり前に、 V728診断は、 おそらくエラーではないが、コードを複雑にするケースに拡張されました。 また、複雑なコードでは、遅かれ早かれエラーが発生します。



この状態は次のように簡略化されています

 if (pItem->mbMirrorMode != bMirror) { .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

プロジェクトには約60の同様の構造があり、その一部は非常にかさばっています。 プロジェクトの作成者は、PVS-Studioアナライザーの完全なレポートに精通しています。

セキュリティの問題

V523 「then」ステートメントは「else」ステートメントと同等です。 docxattributeoutput.cxx 1571

 void DocxAttributeOutput::DoWritePermissionTagEnd( const OUString & permission) { OUString permissionIdAndName; if (permission.startsWith("permission-for-group:", &permissionIdAndName)) { const sal_Int32 sparatorIndex = permissionIdAndName.indexOf(':'); const OUString permissionId = permissionIdAndName.copy(....); const OString rId = OUStringToOString(....).getStr(); m_pSerializer->singleElementNS(XML_w, XML_permEnd, FSNS(XML_w, XML_id), rId.getStr(), FSEND); } else { const sal_Int32 sparatorIndex = permissionIdAndName.indexOf(':'); const OUString permissionId = permissionIdAndName.copy(....); const OString rId = OUStringToOString(....).getStr(); m_pSerializer->singleElementNS(XML_w, XML_permEnd, FSNS(XML_w, XML_id), rId.getStr(), FSEND); } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

非常に大きなコードがここにコピーされます。 特定の権限を変更する機能の場合、特定された問題は非常に疑わしく見えます。



V1001 「DL」変数が割り当てられていますが、関数の終わりまで使用されていません。 cipher.cxx 811

 static void BF_updateECB( CipherContextBF *ctx, rtlCipherDirection direction, const sal_uInt8 *pData, sal_uInt8 *pBuffer, sal_Size nLength) { CipherKeyBF *key; sal_uInt32 DL, DR; key = &(ctx->m_key); if (direction == rtl_Cipher_DirectionEncode) { RTL_CIPHER_NTOHL64(pData, DL, DR, nLength); BF_encode(key, &DL, &DR); RTL_CIPHER_HTONL(DL, pBuffer); RTL_CIPHER_HTONL(DR, pBuffer); } else { RTL_CIPHER_NTOHL(pData, DL); RTL_CIPHER_NTOHL(pData, DR); BF_decode(key, &DL, &DR); RTL_CIPHER_HTONL64(DL, DR, pBuffer, nLength); } DL = DR = 0; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

DL変数とDR変数は、関数の最後の単純な割り当てによって無効化され、使用されなくなりました。 コンパイラにとって、これは最適化のためにコマンドを削除する言い訳です。



Windowsアプリケーションの変数を適切に消去するには、次の方法でコードを書き直すことができます。

 RtlSecureZeroMemory(&DL, sizeof(DL)); RtlSecureZeroMemory(&DR, sizeof(DR));
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ただし、LibreOfficeの場合は、ここでクロスプラットフォームソリューションが必要です。



別の機能からの同様の警告：

• V1001「DL」変数が割り当てられていますが、関数の終わりまで使用されていません。 cipher.cxx 860

V764 「queryStream」関数に渡される引数の誤った順序の可能性：「rUri」および「rPassword」。 tdoc_storage.cxx 271

 css::uno::Reference< css::io::XStream > queryStream( const css::uno::Reference< css::embed::XStorage > & xParentStorage, const OUString & rPassword, const OUString & rUri, StorageAccessMode eMode, bool bTruncate ); uno::Reference< io::XOutputStream > StorageElementFactory::createOutputStream( const OUString & rUri, const OUString & rPassword, bool bTruncate ) { .... uno::Reference< io::XStream > xStream = queryStream( xParentStorage, rUri, rPassword, READ_WRITE_CREATE, bTruncate ); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

queryStream関数のパラメーターリストでは、最初にrPassword 、次にrUriに注意してください。 この関数が呼び出されたときに、対応する引数が混同される場所がコードにありました。



V794代入演算子は、「this ==＆rToBeCopied」のケースから保護する必要があります。 hommatrixtemplate.hxx 121

 ImplHomMatrixTemplate& operator=(....) { // complete initialization using copy for(sal_uInt16 a(0); a < (RowSize - 1); a++) { memcpy(&maLine[a], &rToBeCopied.maLine[a], sizeof(....)); } if(rToBeCopied.mpLine) { mpLine.reset( new ImplMatLine< RowSize >(....) ); } return *this; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このケースは、安全なコード記述に関するものです。 copyステートメントには、オブジェクトを自分自身に割り当てるためのチェックはありません。 合計で、プロジェクトには約30のそのようなコピー演算子の実装があります。

SysAllocStringのエラー

V649同一の条件式を持つ2つの「if」ステートメントがあります。 最初の「if」ステートメントには関数の戻り値が含まれます。 これは、2番目の「if」ステートメントが無意味であることを意味します。 チェック行：125、137。acctable.cxx 137

 STDMETHODIMP CAccTable::get_columnDescription(long column, BSTR * description) { SolarMutexGuard g; ENTER_PROTECTED_BLOCK // #CHECK# if(description == nullptr) return E_INVALIDARG; // #CHECK XInterface# if(!pRXTable.is()) return E_FAIL; .... SAFE_SYSFREESTRING(*description); *description = SysAllocString(o3tl::toW(ouStr.getStr())); if(description==nullptr) // <= return E_FAIL; return S_OK; LEAVE_PROTECTED_BLOCK }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

SysAllocString（）関数は、 NULLの可能性があるポインターを返します 。 奇妙な何かがこのコードの作者によって書かれました。 割り当てられたメモリへのポインタはチェックされないため、プログラムで問題が発生する可能性があります。



他の機能からの同様の警告：

• V649同一の条件式を持つ2つの「if」ステートメントがあります。 最初の「if」ステートメントには関数の戻り値が含まれます。 これは、2番目の「if」ステートメントが無意味であることを意味します。 行を確認してください：344、356。acctable.cxx 356
• V649同一の条件式を持つ2つの「if」ステートメントがあります。 最初の「if」ステートメントには関数の戻り値が含まれます。 これは、2番目の「if」ステートメントが無意味であることを意味します。 行を確認：213、219。trvlfrm.cxx 219

V530関数 'SysAllocString'の戻り値を使用する必要があります。 maccessible.cxx 1077

 STDMETHODIMP CMAccessible::put_accValue(....) { .... if(varChild.lVal==CHILDID_SELF) { SysAllocString(m_pszValue); m_pszValue=SysAllocString(szValue); return S_OK; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

SysAllocString（）関数の呼び出しの1つの結果は使用されません。 開発者はこのコードに注意を払う必要があります。

その他

V716 returnステートメントでの疑わしい型変換：HRESULTを返しましたが、関数は実際にはBOOLを返します。 maccessible.cxx 2649

 BOOL CMAccessible::get_IAccessibleFromXAccessible(....) { ENTER_PROTECTED_BLOCK // #CHECK# if(ppIA == nullptr) { return E_INVALIDARG; // <= } BOOL isGet = FALSE; if(g_pAgent) isGet = g_pAgent->GetIAccessibleFromXAccessible(....); if(isGet) return TRUE; else return FALSE; LEAVE_PROTECTED_BLOCK }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

関数の実行の分岐の1つは、関数の戻り値の型と一致しない型の値を返します。HRESULT型は、BOOL型よりも複雑な形式であり、操作ステータスを格納するように設計されています。たとえば、E_INVALIDARGの値は0x80070057Lです。たとえば、次のように書くのが正しいでしょう。

 return FAILED(E_INVALIDARG);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

詳細については、V716診断資料をご覧ください。



似たような場所：

• V716 returnステートメントでの疑わしい型変換：HRESULTを返しましたが、関数は実際にはBOOLを返します。inprocembobj.cxx 1299
• V716 returnステートメントでの疑わしい型変換：HRESULTを返しましたが、関数は実際にはBOOLを返します。maccessible.cxx 2660

V670初期化されていないクラスメンバー「m_aMutex」は、「m_aModifyListeners」メンバーを初期化するために使用されます。メンバーは、クラス内の宣言の順序で初期化されることに注意してください。fmgridif.cxx 1033

 FmXGridPeer::FmXGridPeer(const Reference< XComponentContext >& _rxContext) :m_aModifyListeners(m_aMutex) ,m_aUpdateListeners(m_aMutex) ,m_aContainerListeners(m_aMutex) ,m_aSelectionListeners(m_aMutex) ,m_aGridControlListeners(m_aMutex) ,m_aMode( getDataModeIdentifier() ) ,m_nCursorListening(0) ,m_bInterceptingDispatch(false) ,m_xContext(_rxContext) { // Create must be called after this constructor m_pGridListener.reset( new GridListenerDelegator( this ) ); } class __declspec(dllexport) FmXGridPeer: public cppu::ImplInheritanceHelper<....> { .... ::comphelper::OInterfaceContainerHelper2 m_aModifyListeners, m_aUpdateListeners, m_aContainerListeners, m_aSelectionListeners, m_aGridControlListeners; .... protected: css::uno::Reference< css::uno::XComponentContext > m_xContext; ::osl::Mutex m_aMutex; .... };
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

言語標準によれば、コンストラクターでのクラスメンバーの初期化の順序は、クラスでの宣言の順序で発生します。この場合、クラスの他の5つのフィールドの初期化に参加した後、m_aMutexフィールドが初期化されます。



クラスフィールドの1つのコンストラクタは次のようになります。

 OInterfaceContainerHelper2( ::osl::Mutex & rMutex );
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

相互排他オブジェクトは参照渡しされます。この場合、さまざまな問題が発生する可能性があります。初期化されていないメモリへのアクセスから、その後のオブジェクト変更の損失まで。



V763パラメーター 'nNativeNumberMode'は、使用される前に常に関数本体で書き換えられます。calendar_jewish.cxx 286

 OUString SAL_CALL Calendar_jewish::getDisplayString(...., sal_Int16 nNativeNumberMode ) { // make Hebrew number for Jewish calendar nNativeNumberMode = NativeNumberMode::NATNUM2; if (nCalendarDisplayCode == CalendarDisplayCode::SHORT_YEAR) { sal_Int32 value = getValue(CalendarFieldIndex::YEAR) % 1000; return mxNatNum->getNativeNumberString(...., nNativeNumberMode ); } else return Calendar_gregorian::getDisplayString(...., nNativeNumberMode ); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

関数の引数はさまざまな理由で上書きされます：コンパイラの警告との戦い、後方互換性、松葉杖など。ただし、これらのソリューションはどれも良くなく、コードはわかりにくいように見えます。



この場所のコードと他の類似のコードを確認する必要があります。

• V763パラメーター 'bExtendedInfo'は、使用される前に常に関数本体で書き換えられます。graphicfilter2.cxx 442
• V763パラメーター 'nVerbID'は、使用される前に常に関数本体で書き換えられます。oleembed.cxx 841
• V763 Parameter 'pCursor' is always rewritten in function body before being used. edlingu.cxx 843
• V763 Parameter 'pOutput' is always rewritten in function body before being used. vnew.cxx 181
• V763 Parameter 'pOutput' is always rewritten in function body before being used. vnew.cxx 256

おわりに

LibreOfficeコードを確認したいという思いは、製品を個人的に使用した後に現れました。何らかの理由で、ランダムに起動すると、すべてのメニュー項目からテキストが消えます。アイコンと単調なストライプのみが残ります。エラーは高レベルである可能性が高く、おそらく、静的分析ツールの助けを借りて見つけることはできません。それでも、アナライザーはこれに関連しない多くの問題を発見しました。LibreOffice開発者が静的コードアナライザーに注意を払い、それらを使用してプロジェクトの品質と信頼性を向上させてくれたら嬉しいです。誰にとっても役立つでしょう。



ご清聴ありがとうございました。 私たちのチャンネルを購読して、プログラミングの世界からのニュースをお楽しみに！

• Vk：@pvsstudio_rus
• 電報：@pvsstudio_rus
• Instagram：@pvsstudio_rus
• Twitter: @pvsstudio_rus
• YouTube: @PVSStudioTool

英語を話す聴衆とこの記事を共有したい場合は、翻訳へのリンクを使用してください：Svyatoslav Razmyslov。 LibreOffice: Accountant's Nightmare