LibreOffice ist eine leistungsstarke Office-Suite, die für den privaten, pädagogischen und kommerziellen Gebrauch kostenlos ist. Die Entwickler stellen ein wunderbares Produkt her, das in vielen Bereichen als Alternative zu Microsoft Office verwendet wird. Das PVS-Studio-Team ist immer daran interessiert, den Code derart bekannter Projekte zu überprüfen und Fehler darin zu finden. Diesmal war es einfach zu tun. Das Projekt enthält viele Fehler, die zu ernsthaften Problemen führen können. In diesem Artikel werden einige interessante Fehler im Code erläutert.
Einführung
LibreOffice ist ein sehr großes C ++ - Projekt. Ein Projekt dieser Größe zu warten, ist eine schwierige Aufgabe für das Entwicklungsteam. Und leider hat man den Eindruck, dass die Qualität des LibreOffice-Codes nicht genügend Beachtung findet.
Einerseits ist das Projekt einfach riesig, nicht jedes statische oder dynamische Analysetool kann die Analyse von 13k-Quellcodedateien durchführen. So viele Dateien sind zusammen mit Bibliotheken von Drittanbietern am Aufbau der Office-Suite beteiligt. Das Hauptrepository von LibreOffice speichert ungefähr 8.000 Quellcodedateien. Diese Menge an Code verursacht nicht nur für Entwickler Probleme:
Andererseits hat das Projekt viele Benutzer und muss so viele Fehler wie möglich finden und beheben. Jeder Fehler kann Hunderte und Tausende von Benutzern verletzen. Daher sollte die Größe der Codebasis keine Entschuldigung dafür sein, die Verwendung bestimmter Tools, die Fehler erkennen können, abzulehnen. Ich denke, der Leser hat bereits vermutet, dass es sich um statische Code-Analysatoren handelt :).
Ja, die Verwendung von statischen Analysegeräten garantiert nicht, dass im Projekt keine Fehler auftreten. Tools wie PVS-Studio können jedoch in der Entwicklungsphase eine große Anzahl von Fehlern finden und so den Arbeitsaufwand für das Debuggen und die Projektunterstützung verringern.
Mal sehen, was Sie im LibreOffice-Quellcode interessant finden können, wenn Sie den statischen Code-Analysator
PVS-Studio verwenden. Die Möglichkeiten zum Ausführen des Analysators sind umfangreich: Windows, Linux, MacOS. Zum Schreiben dieser Bewertung wurde der PVS-Studio-Bericht verwendet, der während der Analyse des Projekts unter Windows erstellt wurde.
Änderungen seit der letzten Überprüfung im Jahr 2015
Im März 2015 wurde die erste Analyse von LibreOffice ("
Überprüfung des LibreOffice-Projekts ") mit PVS-Studio durchgeführt. Seitdem hat sich die Office-Suite als Produkt stark weiterentwickelt, enthält jedoch auch viele Fehler. Und einige Fehlermuster haben sich seitdem überhaupt nicht geändert. Hier ist zum Beispiel ein Fehler aus dem ersten Artikel:
V656 Die Variablen 'aVRP', 'aVPN' werden durch den Aufruf derselben Funktion initialisiert. Es ist wahrscheinlich ein Fehler oder ein nicht optimierter Code. Überprüfen Sie den Ausdruck 'rSceneCamera.GetVRP ()'. Überprüfen Sie die Zeilen: 177, 178. viewcontactofe3dscene.cxx 178
void ViewContactOfE3dScene::createViewInformation3D(....) { .... const basegfx::B3DPoint aVRP(rSceneCamera.GetVRP()); const basegfx::B3DVector aVPN(rSceneCamera.GetVRP());
Dieser Fehler wurde behoben, aber in der neuesten Version des Codes wurde Folgendes gefunden:
V656 Die Variablen 'aSdvURL', 'aStrURL' werden durch den Aufruf derselben Funktion initialisiert. Es ist wahrscheinlich ein Fehler oder ein nicht optimierter Code. Überprüfen Sie den Ausdruck 'pThm-> GetSdvURL ()'. Überprüfen Sie die Zeilen: 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() );
Wie Sie vielleicht bemerkt haben, sind subtile zusammengesetzte Funktionsnamen immer noch eine Fehlerquelle.
Ein weiteres interessantes Beispiel aus dem alten Code:
V656 Die Variablen 'nDragW', 'nDragH' werden durch den Aufruf derselben Funktion initialisiert. Es ist wahrscheinlich ein Fehler oder ein nicht optimierter Code. Überprüfen Sie den Ausdruck 'rMSettings.GetStartDragWidth ()'. Überprüfen Sie die Zeilen: 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(); .... }
Dieser Code enthielt wirklich einen Fehler, der jetzt behoben ist. Aber Fehler im Code werden nicht kleiner ... Eine ähnliche Situation wurde jetzt festgestellt:
V656 Die Variablen 'defaultZoomX', 'defaultZoomY' werden durch den Aufruf derselben Funktion initialisiert. Es ist wahrscheinlich ein Fehler oder ein nicht optimierter Code. Überprüfen Sie den Ausdruck 'pViewData-> GetZoomX ()'. Überprüfen Sie die Zeilen: 5673, 5674. gridwin.cxx 5674
OString ScGridWindow::getCellCursor(....) const { .... SCCOL nX = pViewData->GetCurX(); SCROW nY = pViewData->GetCurY(); Fraction defaultZoomX = pViewData->GetZoomX(); Fraction defaultZoomY = pViewData->GetZoomX();
Fehler werden buchstäblich analog in den Code eingeführt.
Lass dich nicht täuschen
V765 Ein zusammengesetzter Zuweisungsausdruck 'x - = x - ...' ist verdächtig. Überprüfen Sie es auf einen möglichen Fehler. swdtflvr.cxx 3509
bool SwTransferable::PrivateDrop(...) { .... if ( rSrcSh.IsSelFrameMode() ) {
Hier ist so ein interessanter "Hack" mit der
V765 Diagnose gefunden worden. Wenn Sie eine Codezeile mit einem Kommentar vereinfachen, erhalten Sie ein unerwartetes Ergebnis:
1. Schritt:
aSttPt = aSttPt - (aSttPt - rSrcSh.GetObjRect().Pos());
2. Schritt:
aSttPt = aSttPt - aSttPt + rSrcSh.GetObjRect().Pos();
3. Schritt
aSttPt = rSrcSh.GetObjRect().Pos();
Und was ist dann Hack?
Ein weiteres Beispiel zu diesem Thema:
V567 Die Änderung der Variablen 'nCount' ist relativ zu einer anderen Operation für dieselbe Variable nicht sequenziert. Dies kann zu undefiniertem Verhalten führen. stgio.cxx 214
FatError EasyFat::Mark(....) { if( nCount > 0 ) { --nCount /= GetPageSize(); nCount++; } .... }
Die Codeausführung in solchen Situationen kann vom Compiler und vom Sprachstandard abhängen. Warum nicht dieses Code-Snippet einfacher, klarer und zuverlässiger umschreiben?
Wie man keine Arrays und Vektoren verwendet
Aus irgendeinem Grund hat jemand bei der Arbeit mit Arrays und Vektoren viele ähnliche Fehler gemacht. Schauen wir uns diese Beispiele an.
V557 Array-Überlauf ist möglich. Der 'nPageNum'-Index zeigt über die Array-Grenze hinaus. pptx-epptooxml.cxx 1168
void PowerPointExport::ImplWriteNotes(sal_uInt32 nPageNum) { ....
Der letzte gültige Index sollte ein Wert sein, der der
Größe () - 1 entspricht . In diesem Codefragment war jedoch eine Situation zulässig, in der der
nPageNum- Index den Wert
mpSlidesFSArray.size () haben kann , weshalb ein Array außerhalb der Grenzen vorhanden ist und mit einem Element arbeitet, das aus "Garbage" besteht.
V557 Array-Überlauf ist möglich. Der 'mnSelectedMenu'-Index zeigt über die Array-Grenze hinaus. checklistmenu.cxx 826
void ScMenuFloatingWindow::ensureSubMenuNotVisible() { if (mnSelectedMenu <= maMenuItems.size() && maMenuItems[mnSelectedMenu].mpSubMenuWin && maMenuItems[mnSelectedMenu].mpSubMenuWin->IsVisible()) { maMenuItems[mnSelectedMenu].mpSubMenuWin->ensureSubMenuNotVisible(); } EndPopupMode(); }
Interessanterweise haben sie in diesem Codefragment eine Indexprüfung deutlicher geschrieben, aber gleichzeitig den gleichen Fehler gemacht.
V557 Array-Überlauf ist möglich. Der 'nXFIndex'-Index zeigt über die Array-Grenze hinaus. xestyle.cxx 2613
sal_Int32 XclExpXFBuffer::GetXmlStyleIndex( sal_uInt32 nXFIndex ) const { OSL_ENSURE( nXFIndex < maStyleIndexes.size(), "...." ); if( nXFIndex > maStyleIndexes.size() ) return 0;
Und dieser Fehler ist doppelt interessant! Im Debugging-Makro haben sie die korrekte Indexprüfung geschrieben und an einer anderen Stelle erneut einen Fehler gemacht, sodass sie das Array verlassen konnten.
Betrachten Sie nun eine andere Art von Fehler, die nicht mit Indizes zusammenhängt.
V554 Falsche Verwendung von shared_ptr. Der mit 'new []' zugewiesene Speicher wird mit 'delete' bereinigt. dx_vcltools.cxx 158
struct RawRGBABitmap { sal_Int32 mnWidth; sal_Int32 mnHeight; std::shared_ptr< sal_uInt8 > mpBitmapData; }; RawRGBABitmap bitmapFromVCLBitmapEx( const ::BitmapEx& rBmpEx ) { ....
Dieser Code enthält einen Fehler, der zu einem undefinierten Programmverhalten führt. Tatsache ist, dass Speicher auf unterschiedliche Weise zugewiesen und freigegeben wird. Um Speicher ordnungsgemäß freizugeben, mussten Sie ein Klassenfeld wie folgt deklarieren:
std::shared_ptr< sal_uInt8[] > mpBitmapData;
Wie man Makros zweimal macht
V568 Es ist seltsam, dass das Argument des Operators sizeof () der 'bTextFrame? aProps: Ausdruck von 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);
Leider verhalten sich Makroargumente für viele Entwickler nicht wie Funktionsargumente. Das Ignorieren dieser Tatsache führt häufig zu Fehlern. In den Fällen Nr. 1 und Nr. 2 wird unter Verwendung des ternären Operators eine nahezu identische Konstruktion verwendet. Aber im ersten Fall - ein Makro, im zweiten - eine Funktion. Dies ist jedoch nur die Spitze des Problems.
Im Fall Nr. 1 hat der Analysator tatsächlich den folgenden Fehlercode erkannt:
for (std::size_t i = 0; i < (sizeof (bTextFrame ? aProps : aShapeProps) / sizeof ((bTextFrame ? aProps : aShapeProps)[0])); ++i)
Dies ist unsere Schleife mit dem Makro
SAL_N_ELEMENTS . Der Operator
sizeof wertet den Ausdruck im ternären Operator nicht aus. In diesem Fall wird die Arithmetik mit der Größe der Zeiger ausgeführt, deren Ergebnis Werte sind, die weit von der tatsächlichen Größe der angegebenen Arrays entfernt sind. Die Berechnung falscher Werte wird zusätzlich durch die Anwendungsbittiefe beeinflusst.
Aber dann stellte sich heraus, dass es 2
SAL_N_ELEMENTS- Makros gibt! Das heißt, Der Präprozessor hat das falsche Makro geöffnet. Wie konnte das passieren? Die Makrodefinition und die Entwicklerkommentare helfen uns:
#ifndef SAL_N_ELEMENTS # if defined(__cplusplus) && ( defined(__GXX_EXPERIMENTAL_CXX0X__) || __cplusplus >= 201103L ) 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
Eine andere Version des Makros enthält eine sichere Vorlagenfunktion, aber etwas ist schiefgegangen:
- Das sichere Makro war nicht im Code enthalten.
- Es ist immer noch unmöglich, ein anderes Makro zu verwenden, weil Eine erfolgreiche Instanziierung einer Vorlagenfunktion wird nur durchgeführt, wenn Arrays derselben Größe an den ternären Operator übertragen werden. In diesem Fall verliert die Verwendung eines solchen Makros seine Bedeutung.
Tippfehler und Copy-Paste
V1013 Verdächtiger Unterausdruck f1.Pitch == f2.CharSet in einer Folge ähnlicher Vergleiche. 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 &&
Der Fehler ist ein würdiger Kandidat für die Aktualisierung des Artikels „Das
Böse lebt in Vergleichsfunktionen “, falls wir uns jemals dazu entschließen, ihn zu aktualisieren oder zu erweitern. Ich denke, die Wahrscheinlichkeit, einen solchen Fehler zu finden (pass
f2.Pitch ), ist extrem gering. Was denkst du?
V501 Links und rechts vom Operator '&&' befinden sich identische Unterausdrücke '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; .... }
Das Ergebnis des gedankenlosen Kopierens war solch ein Code. Vielleicht wird der bedingte Ausdruck einfach noch einmal dupliziert, aber im Code ist noch kein Platz für solche Mehrdeutigkeiten.
V517 Die Verwendung des
Musters 'if (A) {...} else if (A) {...}' wurde erkannt. Es besteht die Wahrscheinlichkeit eines logischen Fehlers. Überprüfen Sie die Zeilen: 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"))
Beim Kopieren von bedingten Ausdrücken ist im Code ein Fehler
aufgetreten , aufgrund dessen der Wert
8 für die Variable
nColumnSize niemals festgelegt wird.
V523 Die Anweisung 'then' entspricht der Anweisung 'else'. svdpdf.hxx 146
Hier werden die Funktionen
min () und
max () verwechselt. Sicherlich ist aufgrund dieses Tippfehlers in der Benutzeroberfläche etwas seltsam skaliert.
Seltsame Zyklen
V533 Es ist wahrscheinlich, dass eine falsche Variable innerhalb des Operators 'for' inkrementiert wird. Betrachten Sie die Überprüfung von '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"); } .... }
Der Ausdruck
++ i in der Schleife sieht sehr verdächtig aus. Vielleicht sollte es dort
++ j geben .
V756 Der Zähler 'nIndex2' wird nicht in einer verschachtelten Schleife verwendet. Überprüfen Sie die Verwendung des Zählers '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; } .... }
Es gibt eine Art Fehler in der
inneren for- Schleife. Weil Die Variable
nIndex ändert sich nicht. Bei jeder Iteration werden dieselben zwei Elemente des Arrays überschrieben. Anstelle von
nIndex sollte höchstwahrscheinlich
überall die Variable
nIndex2 verwendet
werden .
V1008 Überprüfen Sie den '
for' -Bediener. Es wird nicht mehr als eine Iteration der Schleife durchgeführt. diagrammhelper.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] ); .... } .... }
Die
for- Schleife ist absichtlich auf 1 Iteration beschränkt. Es ist nicht klar, warum dies so gemacht wird.
V612 Eine bedingungslose 'Rückkehr' innerhalb einer Schleife. 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; .... }
Ein Beispiel für eine einfachere seltsame Schleife aus einer Iteration, die besser in eine bedingte Anweisung umgeschrieben werden kann.
Noch ein paar solche Orte:
- V612 Eine bedingungslose 'Rückkehr' innerhalb einer Schleife. txtfrm.cxx 144
- V612 Eine bedingungslose 'Rückkehr' innerhalb einer Schleife. txtfrm.cxx 202
- V612 Eine bedingungslose 'Rückkehr' innerhalb einer Schleife. txtfrm.cxx 279
Seltsame Bedingungen
V637 Es wurden zwei entgegengesetzte Bedingungen
festgestellt . Die zweite Bedingung ist immer falsch. Überprüfen Sie die Zeilen: 281, 285. authfld.cxx 281
sal_uInt16 SwAuthorityFieldType::GetSequencePos(sal_IntPtr nHandle) { .... SwTOXSortTabBase* pOld = aSortArr[i].get(); if(*pOld == *pNew) {
Der Analysator fand widersprüchliche Vergleiche. Mit diesem Code stimmt eindeutig etwas nicht.
Der gleiche Code ist an dieser Stelle zu sehen:
- V637 Es wurden zwei entgegengesetzte Bedingungen festgestellt. Die zweite Bedingung ist immer falsch. Überprüfen Sie die Zeilen: 1827, 1829. doctxm.cxx 1827
V590 Überprüfen Sie diesen Ausdruck. Der Ausdruck ist übertrieben oder enthält einen Druckfehler. fileurl.cxx 55
OUString convertToFileUrl(char const * filename, ....) { .... if ((filename[0] == '.') || (filename[0] != SEPARATOR)) { .... } .... }
Das Problem mit dem obigen Codefragment besteht darin, dass der erste bedingte Ausdruck das Ergebnis des gesamten Ausdrucks nicht beeinflusst.
Aufgrund solcher Fehler schrieb ich sogar einen theoretischen Artikel: "
Logische Ausdrücke in C / C ++. Wie Profis falsch liegen ."
V590 Überprüfen Sie diesen Ausdruck. Der Ausdruck ist übertrieben oder enthält einen Druckfehler. 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; } .... }
Ich verstehe nicht sofort, was das Problem dieser Bedingung ist, daher wurde ein erweitertes Codefragment aus der vorverarbeiteten Datei geschrieben:
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; }
Es ist also vorgekommen, dass nicht eine einzige Zahl gleichzeitig in den 4 Bereichen enthalten ist, die in der Bedingung durch Zahlen angegeben sind. Die Entwickler haben einen Fehler gemacht.
V590 Überprüfen Sie den Ausdruck '* pData <= MAXLEVEL && * pData <= 9'. Der Ausdruck ist übertrieben oder enthält einen Druckfehler. ww8par2.cxx 756
const sal_uInt8 MAXLEVEL = 10; void SwWW8ImplReader::Read_ANLevelNo(....) { ....
Aufgrund der Tatsache, dass die erste Bedingung eine Konstante mit einem Wert von
10 verwendet , erwies sich die Bedingung als redundant. Dieser Code kann wie folgt umgeschrieben werden:
if (*pData <= 9) { .... } else if( *pData == 10 || *pData == 11 ) { .... }
Aber vielleicht gab es ein anderes Problem im vorgestellten Code.
V757 Es ist möglich, dass eine falsche Variable nach der Typkonvertierung mit 'dynamic_cast' mit nullptr verglichen wird. Überprüfen Sie die Zeilen: 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); }
Höchstwahrscheinlich enthält die Bedingung einen Fehler. Es war notwendig, die Zeiger
p und
pPopup zu überprüfen.
V668 Es macht keinen Sinn, den Zeiger 'm_pStream' gegen Null zu testen, da der Speicher mit dem Operator 'new' zugewiesen wurde. Die Ausnahme wird bei einem Speicherzuordnungsfehler generiert. 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; } }
Der Analysator hat eine Situation festgestellt, in der der vom
neuen Operator zurückgegebene Wert des Zeigers mit Null verglichen wird. Gemäß dem C ++ - Sprachstandard
löst der
neue Operator eine Ausnahme
std :: bad_alloc aus , wenn keine Speicherzuordnung möglich ist. Im LibreOffice-Projekt wurden nur 45 solcher Stellen gefunden, was für ein solches Codevolumen sehr klein ist. Dies kann jedoch zu Problemen für die Benutzer führen. Entwickler sollten unnötige Überprüfungen entfernen oder Objekte auf folgende Weise erstellen:
m_pStream = new (std::nothrow) FileStream(FileName.c_str());
V728 Eine übermäßige Überprüfung kann vereinfacht werden. Das '(A &&! B) || (! A && B) 'Ausdruck entspricht dem Ausdruck' 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) ||
Vor sehr langer Zeit wurde die
V728- Diagnose auf Fälle erweitert, die höchstwahrscheinlich nicht fehlerhaft sind, aber den Code komplizieren. Und in komplexem Code werden früher oder später immer noch Fehler gemacht.
Diese Bedingung wird wie folgt vereinfacht:
if (pItem->mbMirrorMode != bMirror) { .... }
Das Projekt enthält etwa 60 ähnliche Strukturen, von denen einige sehr, sehr sperrig sind. Die Autoren des Projekts sollten sich mit dem vollständigen Bericht des PVS-Studio-Analysators vertraut machen.
Sicherheitsprobleme
V523 Die Anweisung 'then' entspricht der Anweisung '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); } }
Hier wird ein sehr großer Code kopiert. Für eine Funktion, die bestimmte Rechte ändert, sieht das identifizierte Problem sehr verdächtig aus.
V1001 Die Variable 'DL' wird zugewiesen, aber am Ende der Funktion nicht verwendet. 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; }
Die
DL- und
DR- Variablen werden durch eine einfache Zuweisung am Ende der Funktion aufgehoben und nicht mehr verwendet. Für den Compiler ist dies eine Ausrede, um Befehle zur Optimierung zu entfernen.
Um Variablen in Windows-Anwendungen ordnungsgemäß zu bereinigen, können Sie den Code folgendermaßen umschreiben:
RtlSecureZeroMemory(&DL, sizeof(DL)); RtlSecureZeroMemory(&DR, sizeof(DR));
Für LibreOffice ist hier jedoch eine plattformübergreifende Lösung erforderlich.
Eine ähnliche Warnung von einer anderen Funktion:
- V1001 Die Variable 'DL' wird zugewiesen, aber am Ende der Funktion nicht verwendet. cipher.cxx 860
V764 Möglicherweise falsche Reihenfolge der an die Funktion 'queryStream' übergebenen Argumente: 'rUri' und '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 ); .... }
Bitte beachten Sie, dass in der
queryStream- Funktion in der Parameterliste zuerst
rPassword und dann
rUri verwendet wird . Es gab eine Stelle im Code, an der die entsprechenden Argumente verwechselt wurden, als diese Funktion aufgerufen wurde.
V794 Der Zuweisungsoperator sollte vor dem Fall 'this == & rToBeCopied' geschützt werden. hommatrixtemplate.hxx 121
ImplHomMatrixTemplate& operator=(....) {
In diesem Fall geht es mehr um sicheres Schreiben von Code. In der copy-Anweisung wird nicht überprüft, ob ein Objekt sich selbst zugewiesen wurde. Insgesamt verfügt das Projekt über etwa 30 solcher Implementierungen des Kopieroperators.
Fehler mit SysAllocString
V649 Es gibt zwei '
if' -Anweisungen mit identischen bedingten Ausdrücken. Die erste 'if'-Anweisung enthält die Funktionsrückgabe. Dies bedeutet, dass die zweite 'if'-Anweisung sinnlos ist. Überprüfen Sie die Zeilen: 125, 137. acctable.cxx 137
STDMETHODIMP CAccTable::get_columnDescription(long column, BSTR * description) { SolarMutexGuard g; ENTER_PROTECTED_BLOCK
Die Funktion
SysAllocString () gibt einen Zeiger zurück, der
NULL sein kann . Der Autor dieses Codes hat etwas Seltsames geschrieben. Der Zeiger auf den zugewiesenen Speicher wird nicht überprüft, was zu Problemen im Programm führen kann.
Ähnliche Warnungen von anderen Funktionen:
- V649 Es gibt zwei 'if'-Anweisungen mit identischen bedingten Ausdrücken. Die erste 'if'-Anweisung enthält die Funktionsrückgabe. Dies bedeutet, dass die zweite 'if'-Anweisung sinnlos ist. Überprüfen Sie die Zeilen: 344, 356. acctable.cxx 356
- V649 Es gibt zwei 'if'-Anweisungen mit identischen bedingten Ausdrücken. Die erste 'if'-Anweisung enthält die Funktionsrückgabe. Dies bedeutet, dass die zweite 'if'-Anweisung sinnlos ist. Überprüfen Sie die Zeilen: 213, 219. trvlfrm.cxx 219
V530 Der Rückgabewert der Funktion 'SysAllocString' muss verwendet werden. maccessible.cxx 1077
STDMETHODIMP CMAccessible::put_accValue(....) { .... if(varChild.lVal==CHILDID_SELF) { SysAllocString(m_pszValue); m_pszValue=SysAllocString(szValue); return S_OK; } .... }
Das Ergebnis eines
Aufrufs der Funktion
SysAllocString () wird nicht verwendet. Entwickler sollten auf diesen Code achten.
Verschiedenes
V716 Verdächtige Typkonvertierung in return-Anweisung: HRESULT zurückgegeben, aber die Funktion gibt tatsächlich BOOL zurück. maccessible.cxx 2649
BOOL CMAccessible::get_IAccessibleFromXAccessible(....) { ENTER_PROTECTED_BLOCK
Einer der Zweige der Funktionsausführung gibt einen Wert zurück, dessen Typ nicht mit dem Typ des Rückgabewerts der Funktion übereinstimmt.
Der HRESULT- Typ hat ein komplexeres Format als der BOOL- Typ und dient zum Speichern von Betriebsstatus. Der Wert von E_INVALIDARG ist beispielsweise 0x80070057L . Es ist richtig, zum Beispiel so zu schreiben: return FAILED(E_INVALIDARG);
Weitere Informationen hierzu finden Sie in der V716-Diagnosedokumentation.Noch ein paar ähnliche Orte:- V716 Verdächtige Typkonvertierung in return-Anweisung: HRESULT zurückgegeben, aber die Funktion gibt tatsächlich BOOL zurück. inprocembobj.cxx 1299
- V716 Verdächtige Typkonvertierung in return-Anweisung: HRESULT zurückgegeben, aber die Funktion gibt tatsächlich BOOL zurück. maccessible.cxx 2660
V670 Das nicht initialisierte Klassenmitglied 'm_aMutex' wird verwendet, um das Mitglied 'm_aModifyListeners' zu initialisieren. Denken Sie daran, dass Mitglieder in der Reihenfolge ihrer Deklarationen innerhalb einer Klasse initialisiert werden. 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) {
Gemäß dem Sprachstandard erfolgt die Initialisierungsreihenfolge der Klassenmitglieder im Konstruktor in der Reihenfolge, in der sie in der Klasse deklariert sind. In unserem Fall wird das Feld m_aMutex initialisiert, nachdem an der Initialisierung von fünf anderen Feldern der Klasse teilgenommen wurde.So sieht der Konstruktor eines der Klassenfelder aus: OInterfaceContainerHelper2( ::osl::Mutex & rMutex );
Das Mutex-Objekt wird als Referenz übergeben. In diesem Fall können verschiedene Probleme auftreten: vom Zugriff auf nicht initialisierten Speicher bis zum anschließenden Verlust von Objektänderungen.V763 Der Parameter 'nNativeNumberMode' wird vor seiner Verwendung immer im Funktionskörper neu geschrieben. calendar_jewish.cxx 286 OUString SAL_CALL Calendar_jewish::getDisplayString(...., sal_Int16 nNativeNumberMode ) {
Funktionsargumente werden aus verschiedenen Gründen überschrieben: Bekämpfung von Compiler-Warnungen, Abwärtskompatibilität, Krücke usw. Jede dieser Lösungen ist jedoch nicht gut und der Code sieht verwirrend aus.Sie sollten den Code für diesen Ort und einige ähnliche überprüfen:- V763 Der Parameter 'bExtendedInfo' wird vor seiner Verwendung immer im Funktionskörper neu geschrieben. Grafikfilter2.cxx 442
- V763 Der Parameter 'nVerbID' wird vor seiner Verwendung immer im Funktionskörper neu geschrieben. 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
Fazit
Der Wunsch, den LibreOffice-Code zu überprüfen, trat nach meinem persönlichen Gebrauch des Produkts auf. Aus irgendeinem Grund verschwindet der Text bei einigen zufälligen Starts aus absolut allen Menüelementen. Es bleiben nur Symbole und monotone Streifen übrig. Der Fehler ist höchstwahrscheinlich hochgradig und kann möglicherweise mit Hilfe statischer Analysewerkzeuge nicht gefunden werden. Trotzdem hat der Analysator viele Probleme festgestellt, die nicht damit zusammenhängen, und ich bin froh, wenn die Entwickler von LibreOffice auf statische Codeanalysatoren achten und versuchen, sie zur Verbesserung der Qualität und Zuverlässigkeit des Projekts zu verwenden. Es wird für alle nützlich sein.Vielen Dank für Ihre Aufmerksamkeit.
Abonnieren Sie unsere Kanäle und bleiben Sie auf dem Laufenden, um Neuigkeiten aus der Programmwelt zu erhalten!
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Link zur Übersetzung: Svyatoslav Razmyslov. LibreOffice: Der Albtraum des Buchhalters