# LibreOffice: Accountant's Nightmare

LibreOffice is a powerful Office package, which is free for private, educational and commercial use\. Programmers working on it, develop a wonderful product that is used in many areas as an alternative to Microsoft Office\. PVS\-Studio team is always interested in analyzing the code of such well\-known projects and trying to find errors in them\. This time it was simple\. The project contains many bugs that can lead to serious problems\. The article will give some interesting defects found in the code\.

![0586_LibreOffice2/image1.png](https://import.viva64.com/docx/blog/0586_LibreOffice2/image1.png)

## Introduction

[LibreOffice](https://www.libreoffice.org/) is a very large C\+\+ project\. Support of a project of this size is a challenge for a team of developers\. Unfortunately, it seems that the quality of LibreOffice code lacks sufficient attention\.

On the one hand, the project is simply huge, not every static or dynamic analysis tool will cope with the analysis of 13k source code files\. This is the number of files involved in the build of an Office package together with third\-party libraries\. The main LibreOffice repository contains about 8k of source code files\. Such amount of code creates problems not only for developers:

![0586_LibreOffice2/image2.png](https://import.viva64.com/docx/blog/0586_LibreOffice2/image2.png)

On the other hand, the project has many users, and you want to find and fix as many bugs as possible\. Each bug can cause pain to hundreds and thousands of users\. Therefore, the large size of codebase should not become an excuse to stop using tools for detecting errors\. I think the reader has already guessed that I'm talking about static code analyzers :\)\.

Yes, usage of static analyzers does not guarantee the absence of errors in a project\. However, such tools as PVS\-Studio can find many errors at the development stage and thus reduce the amount of work related to debugging and support of the project\.

Let's see what interesting things can be found in source codes of LibreOffice, if you apply a [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) static code analyzer\. There is extensive variety of analyzer running alternatives Windows, Linux, macOS\. For writing this review, we used the PVS\-Studio report created when analyzing the project on Windows\.

## Changes since the Last Check in 2015

![0586_LibreOffice2/image3.png](https://import.viva64.com/docx/blog/0586_LibreOffice2/image3.png)

In March of 2015, we performed the first analysis of LibreOffice \("[LibreOffice Project's Check](https://pvs-studio.com/en/blog/posts/cpp/0308/)"\) using PVS\-Studio\. Since then, the Office package has incredibly evolved as a product, but inside it still contains as many errors as it did before\. In addition, some patterns of errors haven't changed at all since then\. For example, here is the error from the first article:

[V656](https://pvs-studio.com/en/docs/warnings/v656/) Variables 'aVRP', 'aVPN' are initialized through the call to the same function\. It's probably an error or un\-optimized code\. Consider inspecting the 'rSceneCamera\.GetVRP\(\)' expression\. Check lines: 177, 178\. viewcontactofe3dscene\.cxx 178

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

This bug is fixed, but here's what was found in the latest version of the code:

[V656](https://pvs-studio.com/en/docs/warnings/v656/) Variables 'aSdvURL', 'aStrURL' are initialized through the call to the same function\. It's probably an error or un\-optimized code\. Consider inspecting the 'pThm\-\>GetSdvURL\(\)' expression\. Check lines: 658, 659\. gallery1\.cxx 659

```cpp
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 may have noticed, subtle composite function names are still a source of errors\.

Another interesting example from the old code:

[V656](https://pvs-studio.com/en/docs/warnings/v656/) Variables 'nDragW', 'nDragH' are initialized through the call to the same function\. It's probably an error or un\-optimized code\. Consider inspecting the 'rMSettings\.GetStartDragWidth\(\)' expression\. Check lines: 471, 472\. winproc\.cxx 472

```cpp
class VCL_DLLPUBLIC MouseSettings
{
  ....
  long GetStartDragWidth() const;
  long GetStartDragHeight() const;
  ....
}

bool ImplHandleMouseEvent( .... )
{
  ....
  long nDragW  = rMSettings.GetStartDragWidth();
  long nDragH  = rMSettings.GetStartDragWidth();
  ....
}
```

This code fragment has really contained an error, which is now fixed\. Nevertheless, the number of errors doesn't reduce\.\.\. Now we have a similar situation:

[V656](https://pvs-studio.com/en/docs/warnings/v656/) Variables 'defaultZoomX', 'defaultZoomY' are initialized through the call to the same function\. It's probably an error or un\-optimized code\. Consider inspecting the 'pViewData\-\>GetZoomX\(\)' expression\. Check lines: 5673, 5674\. gridwin\.cxx 5674

```cpp
OString ScGridWindow::getCellCursor(....) const
{
  ....

  SCCOL nX = pViewData->GetCurX();
  SCROW nY = pViewData->GetCurY();

  Fraction defaultZoomX = pViewData->GetZoomX();
  Fraction defaultZoomY = pViewData->GetZoomX(); // <=
  ....
}
```

Errors are introduced in code literally by analogy\.

## Don't Let It Trick You

![0586_LibreOffice2/image4.png](https://import.viva64.com/docx/blog/0586_LibreOffice2/image4.png)

[V765](https://pvs-studio.com/en/docs/warnings/v765/) A compound assignment expression 'x \-\= x \- \.\.\.' is suspicious\. Consider inspecting it for a possible error\. swdtflvr\.cxx 3509

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

Such an interesting "Hack" was found using the [V765](https://pvs-studio.com/en/docs/warnings/v765/) diagnostic\. If you simplify the line of code with the comment, you can get unexpected results:

Step 1\.

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

Step 2\.

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

Step 3\.

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

Then what is the Hack?

Another example on this topic:

[V567](https://pvs-studio.com/en/docs/warnings/v567/) The modification of the 'nCount' variable is unsequenced relative to another operation on the same variable\. This may lead to undefined behavior\. stgio\.cxx 214

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

Code execution in such situations might depend on the compiler and the language standard\. Why not rewrite this code fragment in a clearer and more reliable way?

## How Not to Use Arrays and Vectors

![0586_LibreOffice2/image5.png](https://import.viva64.com/docx/blog/0586_LibreOffice2/image5.png)

For some reason someone made many similar errors when working with arrays and vectors\. Let's go through these examples\.

[V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The 'nPageNum' index is pointing beyond array bound\. pptx\-epptooxml\.cxx 1168

```cpp
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 the value equal to _size\(\) \- 1_\. But this code allowed a situation where index _nPageNum _can be of the value _mpSlidesFSArray\.size\(\)_, because of which array overrun is occurring as well as working with an element, consisting of "garbage"\.

[V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The 'mnSelectedMenu' index is pointing beyond array bound\. checklistmenu\.cxx 826

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

  EndPopupMode();
}
```

Interestingly, in this code fragment a developer wrote an index check more clearly, but has made the same mistake\.

[V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The 'nXFIndex' index is pointing beyond array bound\. xestyle\.cxx 2613

```cpp
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 ];
}
```

This error is twice exciting\! A developer had written a correct index check, and in another place made an error, allowing array overrun occur\.

Now let's look at a different kind of error, not related to indexes\.

[V554](https://pvs-studio.com/en/docs/warnings/v554/) Incorrect use of shared\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. dx\_vcltools\.cxx 158

```cpp
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 fragment contains an error, resulting in undefined behavior\. The fact of the matter is that memory is allocated and deallocated in different ways\. For proper deallocation, it was necessary to declare a class field as follows:

```cpp
std::shared_ptr< sal_uInt8[] > mpBitmapData;
```

## How to Make a Mistake Twice in Macros

![0586_LibreOffice2/image6.png](https://import.viva64.com/docx/blog/0586_LibreOffice2/image6.png)

[V568](https://pvs-studio.com/en/docs/warnings/v568/) It's odd that the argument of sizeof\(\) operator is the 'bTextFrame ? aProps : aShapeProps' expression\. wpscontext\.cxx 134

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

Sadly for many developers, macros' arguments behave not as function arguments\. Ignoring of this fact often leads to errors\. In cases \#1 and \#2, almost the same construction with the ternary operator is used\. In the first case \- a macro, in the second \- a function\. However, this is only the peak of the problem\.

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

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

This is our loop with the macro _SAL\_N\_ELEMENTS_\. The_ Sizeof_ operator does not evaluate the expression in the ternary operator\. In this case, the arithmetic with the size of pointers is performed\. The result of this arithmetic is the values far from real size of specified arrays\. The bitness of the application additionally affects calculations of incorrect values\.

Then it turned out that there are 2 macros _SAL\_N\_ELEMENTS_\! I\.e\. the preprocessor closed the wrong macro, how could this happen? Macro definition and developers' comments will help us\.

```cpp
#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 the secure template function, but something went wrong:

1. Secure macro is not involved in the code;
1. The other macro is still impossible to use because the successful instancing of the template function is executed only if arrays of the same size are passed to the ternary operator\. And in this case, the use of this macro is meaningless\.

## Typos and Copy\-Paste

![0586_LibreOffice2/image7.png](https://import.viva64.com/docx/blog/0586_LibreOffice2/image7.png)

[V1013](https://pvs-studio.com/en/docs/warnings/v1013/) Suspicious subexpression f1\.Pitch \=\= f2\.CharSet in a sequence of similar comparisons\. xmldlg\_export\.cxx 1251

```cpp
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
      );
}
```

The error is a worthy candidate for adding in the article "[The Evil within the Comparison Functions](https://pvs-studio.com/en/blog/posts/cpp/0509/)", if we ever decide to update or extend it\. I think, the likelihood of finding such an error \(skipping of _f2\. Pitch_\) is extremely low\. What do you think?

[V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'mpTable\[ocArrayColSep\] \!\= mpTable\[eOp\]' to the left and to the right of the '&&' operator\. formulacompiler\.cxx 632

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

Mindless copying resulted in such a code fragment\. Perhaps, a conditional expression is simply duplicated once again, but still in the code, there is no place for such ambiguities\.

[V517](https://pvs-studio.com/en/docs/warnings/v517/) The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 781, 783\. mysqlc\_databasemetadata\.cxx 781

```cpp
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;
  ....
}
```

Because of copying the conditional expressions, an error was made in code due to which the value _8_ for variable _nColumnSize_ is never set\.

[V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. svdpdf\.hxx 146

```cpp
/// 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 functions _min\(\)_ and _max\(\)_ were mixed up\. Something in the interface is strangely scaled for sure, because of this misprint\.

## Strange Loops

![0586_LibreOffice2/image8.gif](https://import.viva64.com/docx/blog/0586_LibreOffice2/image8.gif)

[V533](https://pvs-studio.com/en/docs/warnings/v533/) It is likely that a wrong variable is being incremented inside the 'for' operator\. Consider reviewing 'i'\. javatypemaker\.cxx 602

```cpp
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 expression _\+\+i_ in the loop looks very suspicious\. Perhaps there should be _\+\+j_\.

[V756](https://pvs-studio.com/en/docs/warnings/v756/) The 'nIndex2' counter is not used inside a nested loop\. Consider inspecting usage of 'nIndex' counter\. treex\.cxx 34

```cpp
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 an error in **the internal** loop _'for'_\. As the variable _nIndex _does not change, overwriting of the same two elements of the array occurs on each iteration\. Most likely, everywhere instead of _nIndex_ the variable _**nIndex2**_ had to be used\.

[V1008](https://pvs-studio.com/en/docs/warnings/v1008/) Consider inspecting the 'for' operator\. No more than one iteration of the loop will be performed\. diagramhelper\.cxx 292

```cpp
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 deliberately limited to iteration 1\. It is unclear why it was done this way\.

[V612](https://pvs-studio.com/en/docs/warnings/v612/) An unconditional 'return' within a loop\. pormulti\.cxx 891

```cpp
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 strange loop from one iteration, which is better to be rewritten on the conditional operator\.

Couple more errors that are similar:

* 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

![0586_LibreOffice2/image9.png](https://import.viva64.com/docx/blog/0586_LibreOffice2/image9.png)

[V637](https://pvs-studio.com/en/docs/warnings/v637/) Two opposite conditions were encountered\. The second condition is always false\. Check lines: 281, 285\. authfld\.cxx 281

```cpp
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 has detected inconsistent comparisons\. Something is clearly incorrect with this code fragment\. 

The same code is seen in this place as well:

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

[V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting this expression\. The expression is excessive or contains a misprint\. fileurl\.cxx 55

```cpp
OUString convertToFileUrl(char const * 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 entire expression\.

I even wrote a theoretical article based on similar errors: "[Logical Expressions in C/C\+\+\. Mistakes Made by Professionals](https://pvs-studio.com/en/blog/posts/cpp/0390/)"\. 

[V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting this expression\. The expression is excessive or contains a misprint\. unoobj\.cxx 1895

```cpp
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;
  }
  ....
}
```

It's not immediately clear what is the problem of this condition, so an extended code fragment was written from the preprocessed file:

```cpp
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 so happened that not a single number is included simultaneously in 4 ranges specified in the condition with numbers\. Developers made a mistake\.

[V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting the '\* pData <\= MAXLEVEL && \* pData <\= 9' expression\. The expression is excessive or contains a misprint\. ww8par2\.cxx 756

```cpp
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 in the first condition a constant with a value of _10_ is used, the condition turned out to be redundant\. This code fragment can be rewritten as follows:

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

However, perhaps, the code contained another problem\.

[V757](https://pvs-studio.com/en/docs/warnings/v757/) It is possible that an incorrect variable is compared with nullptr after type conversion using 'dynamic\_cast'\. Check lines: 2709, 2710\. menu\.cxx 2709

```cpp
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 pointers _p_ and _pPopup_\.

[V668](https://pvs-studio.com/en/docs/warnings/v668/) There is no sense in testing the 'm\_pStream' pointer against null, as the memory was allocated using the 'new' operator\. The exception will be generated in the case of memory allocation error\. zipfile\.cxx 408

```cpp
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 pointer value, returned by the _new _operator is compared with zero\. According to the C\+\+ language standard, in case when it is impossible to allocate memory, the operator _new _generates the exception _std::bad\_alloc_\. In the project LibreOffice, we found only 45 of such places, very few for such code amount\. Still this can cause problems for users\. Developers should remove unnecessary checks or create objects in the following way:

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

[V728](https://pvs-studio.com/en/docs/warnings/v728/) An excessive check can be simplified\. The '\(A && \!B\) \|\| \(\!A && B\)' expression is equivalent to the 'bool\(A\) \!\= bool\(B\)' expression\. toolbox2\.cxx 1042

```cpp
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))     // <=
    {
      ....
    }
  }
}
```

Long ago, the [V728](https://pvs-studio.com/en/docs/warnings/v728/) diagnostic was extended with cases, which most likely are not erroneous but complicate the code\.  Eventually errors will be made in complex code\.

This condition simplifies to:

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

There are about 60 of such constructions in the project; some of them are very cumbersome\. It would be good for project if authors could be acquainted with the full PVS\-Studio analyzer report\. 

## Security Issues

![0586_LibreOffice2/image10.png](https://import.viva64.com/docx/blog/0586_LibreOffice2/image10.png)

[V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. docxattributeoutput\.cxx 1571

```cpp
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 a large fragment of code is copied\.  For a function that modifies some rights, the identified problem looks very suspicious\.

[V1001](https://pvs-studio.com/en/docs/warnings/v1001/) The 'DL' variable is assigned but is not used by the end of the function\. cipher\.cxx 811

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

Variables _DL_ and _DR_ are nullified by simple assignment at the end of the function, and are no longer used\. The compiler has a reason to delete commands for optimization\. 

To properly clean up variables in Windows applications, you can rewrite the code this way:

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

Nevertheless, here LibreOffice requires a cross\-platform solution\.

A similar warning from another function:

* V1001 The 'DL' variable is assigned but is not used by the end of the function\. cipher\.cxx 860

[V764](https://pvs-studio.com/en/docs/warnings/v764/) Possible incorrect order of arguments passed to 'queryStream' function: 'rUri' and 'rPassword'\. tdoc\_storage\.cxx 271

```cpp
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 list of parameters _rPassword _comes first and then \- _rUri_\. There was a place in code where appropriate arguments were swapped when calling this function\.

[V794](https://pvs-studio.com/en/docs/warnings/v794/) The assignment operator should be protected from the case of 'this \=\= &rToBeCopied'\. hommatrixtemplate\.hxx 121

```cpp
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 refers to secure coding\. In the copy operator, there is no check on assigning object to itself\. In total, there are about 30 of such implementations of the copy operator\.

## Errors with SysAllocString

![0586_LibreOffice2/image11.png](https://import.viva64.com/docx/blog/0586_LibreOffice2/image11.png)

[V649](https://pvs-studio.com/en/docs/warnings/v649/) There are two 'if' statements with identical conditional expressions\. The first 'if' statement contains function return\. This means that the second 'if' statement is senseless\. Check lines: 125, 137\. acctable\.cxx 137

```cpp
STDMETHODIMP CAccTable::get_columnDescription(long column, BSTR * description)
{
    SolarMutexGuard g;

    ENTER_PROTECTED_BLOCK

    // #CHECK#
    if(description == nullptr)
        return E_INVALIDARG;

    // #CHECK XInterface#
    if(!pRXTable.is())
        return E_FAIL;
    ....
    SAFE_SYSFREESTRING(*description);
    *description = SysAllocString(o3tl::toW(ouStr.getStr()));
    if(description==nullptr) // <=
        return E_FAIL;
    return S_OK;

    LEAVE_PROTECTED_BLOCK
}
```

_SysAllocString\(\)_ function_ _returns a pointer that can be of the _NULL_ value\. The author of this code wrote something weird\. A pointer to allocated memory is not checked, which can lead to problems in program work\.

Similar warnings from other functions:

* V649 There are two 'if' statements with identical conditional expressions\. The first 'if' statement contains function return\. This means that the second 'if' 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 second 'if' statement is senseless\. Check lines: 213, 219\. trvlfrm\.cxx 219

[V530](https://pvs-studio.com/en/docs/warnings/v530/) The return value of function 'SysAllocString' is required to be utilized\. maccessible\.cxx 1077

```cpp
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 of the_ SysAllocString\(\)_ function is not used\. Developers should pay attention to this code\.

## Other Issues

[V716](https://pvs-studio.com/en/docs/warnings/v716/) Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL\. maccessible\.cxx 2649

```cpp
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 execution branches of the function returns a value whose type does not match the type of the return value of the function\. The type _HRESULT _has a more complex format than the _BOOL _type and is meant for storing operations statuses\. For example, a value _E\_INVALIDARG_ is equal to _0x80070057L_\. The correct variant of this code will be as follows:

```cpp
return FAILED(E_INVALIDARG);
```

More on this can be found in the documentation for the diagnostic V716\.

Couple more fragments that are similar:

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

[V670](https://pvs-studio.com/en/docs/warnings/v670/) The uninitialized class member 'm\_aMutex' is used to initialize the 'm\_aModifyListeners' member\. Remember that members are initialized in the order of their declarations inside a class\. fmgridif\.cxx 1033

```cpp
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 initialization order of class members in the constructor takes place in the order they are declared in the class\. In our case, the field _m\_aMutex_ will be initialized after it takes part in the initialization of five other fields of the class\.

Here is how one of the fields of the class constructor looks like:

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

The mutex object is passed by reference\. In this case, various problems might happen starting from accessing uninitialized memory, to the subsequent loss of an object changes\.

[V763](https://pvs-studio.com/en/docs/warnings/v763/) Parameter 'nNativeNumberMode' is always rewritten in function body before being used\. calendar\_jewish\.cxx 286

```cpp
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 different reasons: struggling with compiler warnings, backward compatibility, "ugly hack", etc\. However, any of these decisions is not good, and the code looks confusing\.

Code review should be made of this place and a few 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

I wanted to test the LibreOffice code after I personally used the product\. For some strange reason, during some random runs, the text disappears from all menu items\. There are only icons and monotonous strips\. The error is probably high\-level and perhaps it cannot be found using static analysis tools\. However, the analyzer found so many problems that are not related to this, and I will be happy if LibreOffice developers 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 everyone\.

**Addition to the article dated 26\.10\.2018**

Conclusions of the project quality are made according to the analysis results but it is doesn't depreciate contribution of developers in a certain open source project in any way\.

LibreOffice project is an example of active and what is more important productive cooperation of us and open source project developers\. The main work on processing PVS\-Studio analysis results is carried out on the page [Bug 120703](https://bugs.documentfoundation.org/show_bug.cgi?id=120703)\. During one week more that 100 fixes have been introduced in code\. All these changes are available on official repository\.

In all that time I managed to interact with three project developers\. Thus, for example, I found out that my problem with text disappearing in menu items is related to the OpenGL driver\. In LibreOffice there are additional rendering settings that help to solve this problem\.

We'll continue working with LibreOfiice developers for a while to eliminate all problems found using PVS\-Studio\.

Thank you for your attention\. Subscribe to our channels and follow the news from programming world\!

* Telegram: [@pvsstudio\_en](https://t.me/pvsstudio_en)
* Twitter: [@Code\_Analysis](https://twitter.com/Code_Analysis)
* YouTube: [@PVSStudioTool](https://www.youtube.com/c/PVSStudioTool)