Geekly Articles each Day
📜 ⬆️ ⬇️

LibreOffice: a bad dream accountant


LibreOffice is a powerful office suite that is free for private, educational, and commercial use. Its developers make a great product that is used in many areas as an alternative to Microsoft Office. The PVS-Studio team is always interested in looking at the code of such famous projects and trying to find errors in them. This time it was easy to do. The project contains many errors that can lead to serious problems. The article will consider some interesting defects found in the code.

Introduction


LibreOffice is a very large C ++ project. Maintaining a project of this size is a difficult task for the development team. And, unfortunately, it seems that the quality of the LibreOffice code cannot be given sufficient attention.

On the one hand, the project is simply huge, not every static or dynamic analysis tool will master the analysis of 13k source code files. So many files are involved in building an office suite with third-party libraries. The main LibreOffice repository stores about 8k files of source code. This amount of code creates problems not only for developers:
')


On the other hand, the project has many users and needs to find and correct as many errors as possible. Every mistake can hurt hundreds and thousands of users. Therefore, the large size of the code base should not become a reason to abandon the use of certain tools that can detect errors. I think the reader has already guessed that we are talking about static code analyzers :).

Yes, the use of static analyzers does not guarantee the absence of errors in the project. However, tools such as PVS-Studio are able to find a large number of errors at the design stage and thereby reduce the amount of work related to debugging and supporting the project.

Let's see what you can find interesting in the LibreOffice source codes, if you take the PVS-Studio static code analyzer. The analyzer launch options are extensive: Windows, Linux, macOS. To write this review, we used the PVS-Studio report, created when analyzing a project on Windows.

Changes since the last check in 2015




In March 2015, the first analysis of LibreOffice (" Verification of the LibreOffice Project ") was performed using PVS-Studio. Since then, the office suite has evolved greatly as a product, but inside everything also contains many errors. And some error patterns have not changed at all since then. For example, the error from the first article:

V656 Variables 'aVRP', 'aVPN' are initialized. It's probably not an error or un-optimized code. Consider inspecting the 'rSceneCamera.GetVRP ()' expression. Check lines: 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()); .... }

This bug was fixed, but this is what was found in the most recent version of the code:

V656 Variables 'aSdvURL', 'aStrURL' are initialized. It's probably not an error or un-optimized code. Consider inspecting the 'pThm-> GetSdvURL ()' expression. Check lines: 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() ); // <= .... }

As you can see, subtle function names are still the source of errors.

Another interesting example from the old code:

V656 Variables 'nDragW', 'nDragH' are initialized. It's probably not an error or un-optimized code. Consider inspecting the 'rMSettings.GetStartDragWidth ()' expression. Check lines: 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(); .... }

This piece of code really contained an error that is now fixed. But there are no fewer errors in the code ... Now a similar situation has been revealed:

V656 Variables 'defaultZoomX', 'defaultZoomY' are initialized. It's probably not an error or un-optimized code. Consider inspecting the 'pViewData-> GetZoomX ()' expression. Check lines: 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(); // <= .... }

Errors are entered into the code literally by analogy.

Do not be fooled




V765 A compound assignment expression 'x - = x - ...' is suspicious. Consider inspecting it for a possible error. swdtflvr.cxx 3509

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

Here is such an interesting “Hack” was found using the diagnostics V765 . If you simplify the line of code with a comment, you can get an unexpected result:

1st step:

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

2nd step:

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

3rd step

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

And then what is Hack?

Another example on this topic:

V567 The modification of the nCount variable is unsequenced. This may lead to undefined behavior. stgio.cxx 214

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

Code execution in such situations may depend on the compiler and the language standard. Why not rewrite this piece of code easier, clearer and more reliable?

How not to use arrays and vectors




For some reason, someone has made a lot of mistakes of the same type when working with arrays and vectors. Let's look at these examples.

V557 Array overrun is possible. The 'nPageNum' index is pointing beyond array bound. 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()); .... }

The last valid index should be a value equal to size () - 1 . But in this code snippet, a situation was allowed where the nPageNum index could be mpSlidesFSArray.size () , which causes the array to exit the array and work with the element consisting of garbage.

V557 Array overrun is possible. The 'mnSelectedMenu' index is pointing beyond array bound. checklistmenu.cxx 826

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

Interestingly, in this code snippet, we wrote an index check more clearly, but at the same time made the same error.

V557 Array overrun is possible. The 'nXFIndex' index is pointing beyond array bound. 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 ]; }

And this mistake is doubly interesting! In the debug macro, they wrote the correct index check, and in another place they again made a mistake, allowing the output outside the array.

Now consider a different kind of error not related to indices.

V554 Incorrect use of shared_ptr. The memory is allocated with 'new []' will be cleaned using '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 ] ); .... }

This code snippet contains an error leading to undefined program behavior. The fact is that the memory is allocated and released in different ways. To release the memory correctly, it was necessary to declare the class field as follows:

 std::shared_ptr< sal_uInt8[] > mpBitmapData;

How to make a mistake twice in macros




V568 It's odd that the sizeof () operator is the 'bTextFrame? aProps: aShapeProps' expression. 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])); .... }

Unfortunately for many developers, macro arguments do not behave like function arguments. Ignoring this fact often leads to errors. In cases # 1 and # 2, almost the same construction is used using the ternary operator. But in the first case - a macro, in the second - a function. However, this is only the top of the problem.

In case # 1, the analyzer actually found the following code with an error:

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

This is our loop with the SAL_N_ELEMENTS macro. The sizeof operator does not evaluate an expression in the ternary operator. In this case, arithmetic is performed with the size of the pointers, which results in values ​​far from the actual size of the specified arrays. The calculation of incorrect values ​​additionally affects the width of the application.

But then it turned out that there are 2 macro SAL_N_ELEMENTS ! Those. the preprocessor opened the wrong macro, how could this happen? Macro definition and developer comments will help us:

 #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

Another version of the macro contains a secure template function, but something went wrong:

  1. The safe macro is not included in the code;
  2. It is still impossible to use another macro, since successful instantiation of the template function is performed only if arrays of the same size are passed to the ternary operator. And in this case, the use of such a macro loses its meaning.

Typing and copy-paste




V1013 Suspicious subexpression f1.Pitch == f2.CharSet in a sequence of similar comparisons. 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 ); }

Error is a worthy candidate for replenishing the article " Evil lives in comparison functions " if we ever decide to update or expand it. I think the probability of finding such an error (skipping f2.Pitch ) on its own is extremely small. What do you think?

V501 There are identical sub-expressions 'mpTable [ocArrayColSep]! = MpTable [eOp]' the operator. formulacompiler.cxx 632

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

The result of mindless copying was such a code fragment. Perhaps the conditional expression is simply duplicated once again, but still there is no place for such ambiguities in the code.

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 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; .... }

As a result of copying conditional expressions, an error was made in the code, due to which the value 8 for the variable nColumnSize is never set.

V523 The 'then' statement is equivalent to the 'else' statement. 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); // <= }

Here the functions min () and max () are confused. Surely because of this typo in the interface, something strange scaled.

Strange cycles




V533 It is likely that a variable is being incremented inside the 'for' operator. Consider reviewing '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"); } .... }

The ++ i expression in the loop looks very suspicious. Perhaps there should be ++ j .

V756 The 'nIndex2' counter is not used inside a loop. Consider inspecting usage of 'nIndex' counter. 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; } .... }

There is some kind of error in the inner for loop. Since the nIndex variable does not change, the same two array elements are overwritten at each iteration. Most likely, the variable nIndex2 should have been used everywhere instead of nIndex .

V1008 Consider inspecting the 'for' operator. Will be performed. 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] ); .... } .... }

The for loop is intentionally limited to 1 iteration. It is not clear why this is done in this way.

V612 An unconditional 'return' within a loop. 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; .... }

An example of a simpler, odd loop from one iteration, which is best rewritten to a conditional operator.

A few more places like this:


Strange conditions




V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 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; } .... }

The analyzer found inconsistent comparisons. Something with this code snippet is clearly wrong.

The same code is seen in this place:


V590 Consider inspecting this expression. The expression is misprint. fileurl.cxx 55

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

The problem with the above code fragment is that the first conditional expression does not affect the result of the whole expression.

Based on these errors, I even wrote a theoretical article: " Logical expressions in C / C ++. How professionals are mistaken ."

V590 Consider inspecting this expression. The expression is misprint. 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; } .... }

Immediately do not understand what the problem is this condition, so an expanded code fragment was written out from the preprocessed file:

 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; }

It turned out that not a single number is included simultaneously in the 4 ranges specified in the condition by numbers. The developers made a mistake.

V590 Consider inspecting the '* pData <= MAXLEVEL && * pData <= 9' expression. The expression is misprint. 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; } .... }

Because the first condition uses a constant with a value of 10 , the condition turned out to be redundant. This code snippet can be rewritten as follows:

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

But perhaps there was another problem with the code presented.

V757 It is possible that the variable is compared with nullptr after type conversion using 'dynamic_cast'. Check lines: 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); }

Most likely, the condition contains an error. It was necessary to check the p and pPopup pointers .

V668 using m_pStream p against p p was _ _ _ _ _ _ _ _ _ _ The exception will be generated in the case of memory allocation error. 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; } }

The analyzer detected a situation when the value of the pointer returned by the new operator is compared with zero. According to the C ++ standard of the language, in case of impossibility of allocating memory, the operator new throws an exception std :: bad_alloc . In the LibreOffice project, there were only 45 such places, which is very small for this amount of code. But still it can lead to problems for users. Developers should remove unnecessary checks or create objects in this way:

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

V728 Anonymous check can be simplified. The '(A &&! B) || (! A && B) 'expression is equivalent to the' bool (A)! = Bool (B) 'expression. 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)) // <= { .... } } }

A long time ago, the diagnosis of the V728 was expanded to cases that, most likely, are not wrong, but complicate the code. And in a complex code, errors will be made sooner or later.

This condition is simplified to this:

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

About 60 of these structures are found in the project, some of which are very, very bulky. The authors of the project are better acquainted with the full report of the PVS-Studio analyzer.

Safety problems




V523 The 'then' statement is equivalent to the 'else' statement. 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); } }

Here is copied a very large piece of code. For a function that changes certain rights, the identified problem looks very suspicious.

V1001 The "DL" variable is assigned. 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; }

The variables DL and DR are reset to zero by simple assignment at the end of the function and are no longer used. For the compiler, this is the reason to remove the optimization commands.

To properly clean up variables in Windows applications, you can rewrite the code as follows:

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

But for LibreOffice, a cross-platform solution is required here.

A similar warning from another function:
V764 possible query order passed to 'queryStream' function: 'rUri' and '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 ); .... }

Notice that in the queryStream function in the parameter list, rPassword first comes, and then rUri . In the code there was a place where the corresponding arguments are interchanged when calling this function.

V794 The assignment operator should not be protected from the case of 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; }

The given case concerns safe code writing more. In the copy statement there is no check for assigning an object to itself. There are about 30 such implementations of the copy operator in the project.

Errors with SysAllocString




V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the statement is senseless. Check lines: 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 }

The SysAllocString () function returns a pointer, which can be NULL . Something strange was written by the author of this code. A pointer to allocated memory is not checked, which can lead to problems in the program.

Similar warnings from other features:


V530 SysAllocString 'is the value required to be used. maccessible.cxx 1077

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

The result of one of the calls to the SysAllocString () function is not used. Developers should pay attention to this code snippet.

miscellanea


V716 Suspicious type conversion in return statement: returned HRESULT, but the function actually returns 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 }

One of the branches of the execution of the function returns a value whose type does not correspond to the type of the return value of the function.The HRESULT type has a more complex format than the BOOL type , and is intended for storing operation statuses. For example, the value of E_INVALIDARG is 0x80070057L . It will be correct to write, for example, like this:

 return FAILED(E_INVALIDARG);

You can read more about this in the V716 diagnostic documentation.

Some more similar places:


V670 The uninitialized class member 'm_aMutex' is used to initialize the 'm_aModifyListeners' member. Remember that members are initialized in the order of declarations inside a class. 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; .... };

According to the language standard, the order of initialization of class members in the constructor occurs in the order they are declared in the class. In our case, the m_aMutex field will be initialized after it participates in the initialization of five other fields of the class.

Here is the constructor of one of the class fields:

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

The mutex object is passed by reference. In this case, various problems may arise: from accessing uninitialized memory to the subsequent loss of changes to the object.

V763 Parameter 'nNativeNumberMode' is always rewritten in function body before being used. 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 ); }

Function arguments are overwritten for various reasons: fighting compiler warnings, backward compatibility, “crutch”, etc. However, any of these solutions is not good, and the code looks confusing.

You should review the code of this place and a few more similar ones:


Conclusion


The desire to check the code LibreOffice I had after personal use of the product. For some reason, with some random launches, absolutely all menu items disappear. There are only icons and monotonous stripes. The error is most likely a high-level one and, probably, it cannot be found with the help of static analysis tools. Nevertheless, the analyzer has found a lot of problems not related to this, and I will be happy if the developers of LibreOffice pay attention to static code analyzers and try to use them to improve the quality and reliability of the project. It will be useful to all.

Thanks for attention. Subscribe to our channels and follow the news from the world of programming!




If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. LibreOffice: Accountant's Nightmare

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

More articles:


All Articles