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 well-known projects and trying to find errors in them. This time it was easy to do it. 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 enough 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 can be found 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 LibreOffice analysis (" Checking the LibreOffice Project ") was performed using PVS-Studio. Since then, the office suite has developed greatly as a product, but inside everything also contains many errors. And some error patterns have not changed at all since then. Here, for example, is the error from the first article:

V656 Variables 'aVRP', 'aVPN' are initialized through the call to the same function. 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 error was corrected, 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 through the call to the same function. It's probably not an error or un-optimized code. Consider inspecting the 'rMSettings.GetStartDragWidth ()' expression. Check lines: 471, 472. winproc.cxx 472

classVCL_DLLPUBLICMouseSettings
{
  ....
  longGetStartDragWidth()const;
  longGetStartDragHeight()const;
  ....
}
boolImplHandleMouseEvent( .... ){
  ....
  long nDragW  = rMSettings.GetStartDragWidth();
  long nDragH  = rMSettings.GetStartDragWidth();
  ....
}

This code snippet did contain an error that is now fixed. But there are no fewer errors in the code ... Now a similar situation has been identified:

V656 Variables 'defaultZoomX', 'defaultZoomY' are initialized through the call to the same function. 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 V765 diagnostic . 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 is:

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 language standard. Why not rewrite this code snippet 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 notesif (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 output to go beyond 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() )
        return0;   // 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

structRawRGBABitmap
{
  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 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 detected 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 a ternary operator. In this case, arithmetic is performed with the size of the pointers, which results in values ​​that are 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 SAL_N_ELEMENTS macros ! 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, e.g.
         *  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, i.e. 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

inlineboolequalFont( 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();
  elseif (sDataType.equalsIgnoreAsciiCase("date"))       // <=
      nColumnSize = 10;
  elseif (sDataType.equalsIgnoreAsciiCase("date"))       // <=
      nColumnSize = 8;
  elseif (sDataType.equalsIgnoreAsciiCase("datetime")
           || sDataType.equalsIgnoreAsciiCase("timestamp"))
      nColumnSize = 19;
  elseif (!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> voidTransform(....){
  ....
  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

voidprintConstructors(....){
  ....
  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();
    return1;
  }
  ....
}

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

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*constpHints(m_pNode->GetpSwpHints());
  if (pHints)
  {
    while (m_CurrentHint < pHints->Count())
    {
      SwTextAttr const*constpHint(pHints->Get(m_CurrentHint));
      ++m_CurrentHint;
      rpNode = m_pNode;
      return pHint;
    }
  }
  returnnullptr;
  ....
}

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

A few more places like this:

• V612 An unconditional 'return' within a loop. txtfrm.cxx 144
• V612 An unconditional 'return' within a loop. txtfrm.cxx 202
• V612 An unconditional 'return' within a loop. txtfrm.cxx 279

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 arrayif(*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:

• V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 1827, 1829. doctxm.cxx 1827

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

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

The problem of the given 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 extensive 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 simultaneously included 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 / numberingif (*pData <= MAXLEVEL && *pData <= 9)
  {
    ....
  }
  elseif( *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)
{
  ....
}
elseif( *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 has been defined as the m_pStream. The exception will be generated in the case of memory allocation error. zipfile.cxx 408

ZipFile::ZipFile(conststd::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 has been expanded to cases that, most likely, are not wrong, but complicate the code. And in a complex code, errors are still made sooner or later.

This condition is simplified to this:

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

In the project, there are about 60 such structures, some of which are very, very cumbersome. Project authors 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 of the function. cipher.cxx 811

staticvoidBF_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 set 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 variables in Windows applications, you can rewrite the code in the following way:

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

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

A similar warning from another function:

• V1001 The "DL" variable of the function. cipher.cxx 860

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

Note 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 were 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 copyfor(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 that 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:

• 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: 344, 356. acctable.cxx 356
• 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: 213, 219. trvlfrm.cxx 219

V530 SysAllocString 'is the required value of function. 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;
  elsereturn 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:

• V716 Suspicious type conversion in return statement: returned HRESULT, but the function actually returns BOOL. inprocembobj.cxx 1299
• V716 Suspicious type conversion in return statement: returned HRESULT, but the function actually returns BOOL. maccessible.cxx 2660

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 can 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 );
  }
  elsereturn Calendar_gregorian::getDisplayString(...., nNativeNumberMode );
}

Function arguments are rewritten 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:

• V763 Parameter 'bExtendedInfo' is always rewritten in function body before being used. graphicfilter2.cxx 442
• V763 Parameter 'nVerbID' is always rewritten in function body before being used. 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

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!

• Vk: @pvsstudio_rus
• Telegram: @pvsstudio_rus
• Instagram: @pvsstudio_rus
• Twitter: @pvsstudio_rus
• YouTube: @PVSStudioTool

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