# LibreOffice: страшный сон бухгалтера

LibreOffice \- мощный офисный пакет, который бесплатен для частного, образовательного и коммерческого использования\. Его разработчики делают замечательный продукт, который во многих сферах используется в качестве альтернативы Microsoft Office\. Команде PVS\-Studio всегда интересно взглянуть на код таких известных проектов и попробовать найти в них ошибки\. В этот раз сделать это было легко\. Проект содержит много ошибок, которые могут привести к серьёзным проблемам\. В статье будут рассмотрены некоторые интересные дефекты, найденные в коде\.

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

## Введение

[LibreOffice](https://www.libreoffice.org/) \- очень крупный C\+\+ проект\. Поддерживать проект такого объёма \-  сложная задача для команды разработчиков\. И, к сожалению, складывается впечатление, что качеству кода LibreOffice не удаётся уделять достаточного внимания\.

С одной стороны, проект просто огромный, не каждый инструмент статического или динамического анализа осилит анализ 13к файлов исходного кода\. Столько файлов участвует в сборке офисного пакета вместе со сторонними библиотеками\. В основном репозитории LibreOffice хранится около 8к файлов исходного кода\. Такой объём кода создаёт проблемы не только разработчикам:

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

С другой стороны, у проекта много пользователей и требуется найти и исправить как можно больше ошибок\. Каждая ошибка может причинять боль сотням и тысячам пользователей\. Поэтому большой размер кодовой базы не должен становиться поводом отказаться от использования тех или иных инструментов, способных обнаружить ошибки\. Думаю, читатель уже догадался, что речь идёт о статических анализаторах кода :\)\.

Да, использование статических анализаторов не гарантирует отсутствия ошибок в проекте\. Однако такие инструменты, как PVS\-Studio, способны найти большое количество ошибок ещё на этапе разработки и тем самым уменьшить объём работ, связанных с отладкой и поддержкой проекта\.

Давайте посмотрим, что можно найти интересного в исходных кодах LibreOffice, если взять статический анализатор кода [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. Возможности запуска анализатора обширны: Windows, Linux, macOS\. Для написания этого обзора использовался отчёт PVS\-Studio, созданный при анализе проекта на Windows\.

## Изменения с последней проверки в 2015 году

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

В марте 2015 года был выполнен первый анализ LibreOffice \("[Проверка проекта LibreOffice](https://pvs-studio.ru/ru/blog/posts/cpp/0308/)"\) с помощью PVS\-Studio\. С тех пор офисный пакет сильно развился как продукт, но внутри всё также содержит множество ошибок\. А некоторые паттерны ошибок вообще не поменялись с тех пор\. Вот, например, ошибка из первой статьи:

[V656](https://pvs-studio.ru/ru/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());
  ....
}
```

Эта ошибка исправлена, но вот что нашлось в самой последней версии кода:

[V656](https://pvs-studio.ru/ru/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() ); // <=
  ....
}
```

Как вы могли заметить, едва различимые составные имена функций до сих пор являются источником ошибок\.

Ещё один интересный пример из старого кода:

[V656](https://pvs-studio.ru/ru/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();
  ....
}
```

Этот фрагмент кода действительно содержал ошибку, которая сейчас исправлена\. Но ошибок в коде меньше не становится\.\.\. Сейчас выявлена похожая ситуация:

[V656](https://pvs-studio.ru/ru/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(); // <=
  ....
}
```

Ошибки вносятся в код буквально по аналогии\.

## Не дай себя обмануть

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

[V765](https://pvs-studio.ru/ru/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();
  }
  ....
}
```

Вот такой вот интересный "Hack" был найден с помощью диагностики [V765](https://pvs-studio.ru/ru/docs/warnings/v765/)\. Если упростить строку кода с комментарием, то можно получить неожиданный результат:

1\-й шаг:

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

2\-й шаг:

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

3\-й шаг

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

И в чём тогда заключается Hack?

Ещё один пример на эту тему:

[V567](https://pvs-studio.ru/ru/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++;
  }
  ....
}
```

Выполнение кода в таких ситуациях может зависеть от компилятора и стандарта языка\. Почему бы не переписать этот фрагмент кода проще, понятнее и надёжнее?

## Как не надо использовать массивы и векторы

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

По какой\-то причине кто\-то понаделал множество однотипных ошибок при работе с массивами и векторами\. Давайте разберём эти примеры\.

[V557](https://pvs-studio.ru/ru/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());
  ....
}
```

Последним валидным индексом должно являться значение, равное _size\(\) \- 1_\. Но в этом фрагменте кода допустили ситуацию, когда индекс _nPageNum_ может иметь значение _mpSlidesFSArray\.size\(\)_, из\-за чего происходит выход за пределы массива и работа с элементом, состоящим из "мусора"\.

[V557](https://pvs-studio.ru/ru/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();
}
```

Интересно, что в этом фрагменте кода написали проверку индекса более понятно, но при этом допустили такую же ошибку\.

[V557](https://pvs-studio.ru/ru/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 ];
}
```

А эта ошибка вдвойне интереснее\! В отладочном макросе написали правильную проверку индекса, а в другом месте снова сделали ошибку, допустив выход за пределы массива\.

Теперь рассмотрим ошибку иного рода, не связанную с индексами\.

[V554](https://pvs-studio.ru/ru/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 ] );
  ....
}
```

Этот фрагмент кода содержит ошибку, приводящую к неопределённому поведению программы\. Дело в том, что память выделяется и освобождается разными способами\. Для правильного освобождения памяти необходимо было объявить поле класса таким образом:

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

## Как дважды ошибиться в макросах

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

[V568](https://pvs-studio.ru/ru/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]));
  ....
}
```

К сожалению для многих разработчиков, аргументы макросов ведут себя не как аргументы функций\. Игнорирование этого факта часто приводит к ошибкам\. В случаях \#1 и \#2 используется почти одинаковая конструкция с использованием тернарного оператора\. Но в первом случае \- макрос, во втором \- функция\. Однако это только вершина проблемы\.

В случае \#1 анализатор на самом деле обнаружил следующий код с ошибкой:

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

Это наш цикл с макросом _SAL\_N\_ELEMENTS_\. Оператор _sizeof_ не вычисляет выражение в тернарном операторе\. В данном случае выполняется арифметика с размером указателей, результатом которой являются значения, далёкие от реального размера указанных массивов\. На вычисление неправильных значений дополнительно влияет и разрядность приложения\.

Но потом оказалось, что существует 2 макроса _SAL\_N\_ELEMENTS_\! Т\.е\. препроцессор раскрыл не тот макрос, как же это могло произойти? Нам поможет определение макроса и комментарии разработчиков:

```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
```

Другая версия макроса содержит безопасную шаблонную функцию, но что\-то пошло не так:

1. Безопасный макрос не включился в код;
1. Другим макросом всё равно невозможно пользоваться, т\.к\. успешное инстанцирование шаблонной функции выполняется только если в тернарный оператор передать массивы одинакового размера\. А в этом случае использование такого макроса теряет смысл\.

## Опечаточки и copy\-paste

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

[V1013](https://pvs-studio.ru/ru/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
      );
}
```

Ошибка является достойным кандидатом для пополнения статьи "[Зло живёт в функциях сравнения](https://pvs-studio.ru/ru/blog/posts/cpp/0509/)", если мы когда\-нибудь решим её обновить или расширить\. Думаю, вероятность найти такую ошибку \(пропуск _f2\.Pitch_\) самостоятельно крайне мала\. А вы как считаете?

[V501](https://pvs-studio.ru/ru/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;
  ....
}
```

Результатом бездумного копирования стал такой фрагмент кода\. Возможно, условное выражение просто продублировано лишний раз, но всё равно в коде не место таким неоднозначностям\.

[V517](https://pvs-studio.ru/ru/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;
  ....
}
```

В результате копирования условных выражений, в коде была допущена ошибка, из\-за которой значение _8_ для переменной _nColumnSize_ никогда не выставляется\.

[V523](https://pvs-studio.ru/ru/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);  // <=
}
```

Тут перепутали функции _min\(\)_ и _max\(\)_\. Наверняка из\-за этой опечатки в интерфейсе что\-то странно масштабируется\.

## Странные циклы

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

[V533](https://pvs-studio.ru/ru/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");
  }
  ....
}
```

Выражение _\+\+i_ в цикле выглядит очень подозрительно\. Возможно, там должно быть _\+\+j_\.

[V756](https://pvs-studio.ru/ru/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;
  }
  ....
}
```

Есть какая\-то ошибка во **внутреннем** цикле _for_\. Т\.к\. переменная _nIndex_ не изменяется, происходит перезаписывание одних и тех же двух элементов массива на каждой итерации\. Скорее всего, везде вместо _nIndex_ должна была использоваться переменная _**nIndex2**_\.

[V1008](https://pvs-studio.ru/ru/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] );
    ....
  }
  ....
}
```

Цикл _for_ намеренно ограничивается до 1 итерации\. Непонятно, зачем это сделано именно таким способом\.

[V612](https://pvs-studio.ru/ru/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;
  ....
}
```

Пример более простого странного цикла из одной итерации, который лучше переписать на условный оператор\.

Ещё несколько таких мест:

* 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

## Странные условия

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

[V637](https://pvs-studio.ru/ru/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;
  }
  ....
}
```

Анализатор обнаружил противоречивые сравнения\. Что\-то с этим фрагментом кода явно не так\.

Такой же код замечен и в этом месте:

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

[V590](https://pvs-studio.ru/ru/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))
  {
    ....
  }
  ....
}
```

Проблема приведённого фрагмента кода заключается в том, что первое условное выражение не влияет на результат всего выражения\.

По мотивам подобных ошибок я даже написал теоретическую статью: "[Логические выражения в C/C\+\+\. Как ошибаются профессионалы](https://pvs-studio.ru/ru/blog/posts/cpp/0390/)"\.

[V590](https://pvs-studio.ru/ru/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;
  }
  ....
}
```

Сразу не понять, в чём проблема данного условия, поэтому из препроцессированного файла был выписан развёрнутый фрагмент кода:

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

Получилось так, что ни одно число не входит одновременно в 4 диапазона, заданных в условии числами\. Разработчики допустили ошибку\.

[V590](https://pvs-studio.ru/ru/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;
  }
  ....
}
```

Из\-за того, что в первом условии используется константа со значением _10_, условие получилось избыточным\. Этот фрагмент кода можно переписать следующим образом:

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

Но, возможно, в представленном коде была другая проблема\.

[V757](https://pvs-studio.ru/ru/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);
}
```

Скорее всего, условие содержит ошибку\. Необходимо было проверить указатели _p_ и _pPopup_\.

[V668](https://pvs-studio.ru/ru/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;
    }
}
```

Анализатор обнаружил ситуацию, когда значение указателя, возвращаемого оператором _new_, сравнивается с нулём\. Согласно стандарту языка C\+\+, в случае невозможности выделения памяти, оператор _new_ сгенерирует исключение _std::bad\_alloc_\. В проекте LibreOffice нашлось всего 45 подобных мест, что очень мало для такого объёма кода\. Но всё равно это может приводить к проблемам у пользователей\. Разработчикам следует убрать лишние проверки или создавать объекты таким образом:

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

[V728](https://pvs-studio.ru/ru/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))     // <=
    {
      ....
    }
  }
}
```

Очень давно диагностика [V728](https://pvs-studio.ru/ru/docs/warnings/v728/) была расширена до случаев, которые, скорое всего, не являются ошибочными, но усложняют код\. А в сложном коде рано или поздно всё равно допускается ошибки\.

Данное условие упрощается до такого:

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

В проекте встречается около 60 подобных конструкций, некоторые из них очень и очень громоздкие\. Авторам проекта лучше ознакомиться с полным отчётом анализатора PVS\-Studio\.

## Проблемы с безопасностью

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

[V523](https://pvs-studio.ru/ru/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);
    }
}
```

Здесь скопирован очень крупный фрагмент кода\. Для функции, которая изменяет некие права, выявленная проблема выглядит очень подозрительно\.

[V1001](https://pvs-studio.ru/ru/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;
}
```

Переменные _DL_ и _DR_ обнуляются простым присваиванием в конце функции и больше не используются\. Для компилятора это повод удалить команды для оптимизации\. 

Для правильной очистки переменных в Windows приложениях можно переписать код таким образом:

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

Но для LibreOffice здесь потребуется кросс\-платформенное решение\.

Похожее предупреждение из другой функции:

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

[V764](https://pvs-studio.ru/ru/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 );
  ....
}
```

Обратите внимание, что в функции _queryStream_ в списке параметров сначала идёт _rPassword_, а затем – _rUri_\. В коде нашлось место, где соответствующие аргументы перепутаны местами при вызове этой функции\.

[V794](https://pvs-studio.ru/ru/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;
}
```

Приведённый случай больше относится к безопасному написанию кода\. В операторе копирования нет проверки на присвоение объекта самому себе\. Всего в проекте около 30 таких реализаций оператора копирования\.

## Ошибки с SysAllocString

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

[V649](https://pvs-studio.ru/ru/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\(\)_ возвращает указатель, который может иметь значение _NULL_\. Что\-то странное написал автор этого кода\. Указатель на аллоцированную память не проверяется, что может привести к проблемам в работе программы\.

Похожие предупреждения из других функций:

* 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.ru/ru/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;
  }
  ....
}
```

Результат одного из вызовов функции _SysAllocString\(\)_ не используется\. Разработчикам следует обратить внимание на этот фрагмент кода\.

## Разное

[V716](https://pvs-studio.ru/ru/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
}
```

Одна из ветвей исполнения функции возвращает значение, тип которого не соответствует типу возвращаемого значения функции\. Тип _HRESULT_ имеет более сложный формат, чем тип _BOOL_, и предназначен для хранения статусов операций\. Например, значение _E\_INVALIDARG_ равно _0x80070057L_\. Правильно будет написать, например, так:

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

Более подробно об этом можно прочесть в документации к диагностике V716\.

Ещё несколько подобных мест:

* 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.ru/ru/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;
    ....
};
```

Согласно стандарту языка, порядок инициализации членов класса в конструкторе происходит в порядке их объявления в классе\. В нашем случае поле _m\_aMutex_ будет инициализировано уже после того, как поучаствует в инициализации пяти других полей класса\.

Вот как выглядит конструктор одного из полей класса:

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

Объект мьютекса передаётся по ссылке\. В этом случае могут возникать разные проблемы: от обращения к неинициализированной памяти, до последующей потери изменений объекта\.

[V763](https://pvs-studio.ru/ru/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 );
}
```

Аргументы функции перезаписывают по разным причинам: борьба с предупреждениями компилятора, обратная совместимость, "костыль" и т\.п\. Тем не менее, любое из этих решений не является хорошим, а код выглядит запутанным\.

Следует сделать обзор кода этого места и ещё несколько похожих:

* 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

## Заключение

Желание проверить код LibreOffice у меня появилось после личного использования продукта\. По непонятной причине, при некоторых случайных запусках абсолютно из всех пунктов меню исчезает текст\. Остаются только иконки и монотонные полоски\. Ошибка, скорее всего, высокоуровневая и, возможно, её нельзя найти с помощью инструментов статического анализа\. Тем не менее, анализатор нашёл очень много проблем, не связанных с этим, и я буду рад, если разработчики LibreOffice обратят внимание на статические анализаторы кода и попробуют использовать их для повышения качества и надёжности проекта\. Это будет полезно всем\.

**Дополнение от 26\.10\.2018**

Выводы о качестве проекта делаются на основе результатов анализа, но это никоим образом не обесценивает вклад разработчиков в тот или иной проект с открытым исходным кодом\. 

Проект LibreOffice – пример активного, а самое главное, плодотворного сотрудничества нас и разработчиков открытого проекта\. Основная работа по обработке результатов анализа PVS\-Studio ведётся на странице [Bug 120703](https://bugs.documentfoundation.org/show_bug.cgi?id=120703)\. За неделю в код было внесено более сотни исправлений\. Посмотреть все изменения можно на странице официального репозитория\.

За это время мне удалось пообщаться с тремя разработчиками проекта\. Так, например, я узнал, что моя проблема с исчезновением текста в пунктах меню связана с драйвером OpenGL\. В LibreOffice есть дополнительные настройки рендеринга, помогающие исправить проблему\.

Ещё некоторое время мы поработаем с разработчиками LibreOfiice, чтобы исправить абсолютно все проблемы, найденные с помощью PVS\-Studio\.

Спасибо за внимание\. Подписывайтесь на наши каналы и следите за новостями из мира программирования\!

* Vk: [@pvsstudio\_rus](https://vk.com/pvsstudio_rus) 
* Telegram: [@pvsstudio\_rus](https://t.me/pvsstudio_rus)
* Twitter: [@pvsstudio\_rus](https://twitter.com/pvsstudio_rus)
* YouTube: [@PVSStudioTool](https://www.youtube.com/c/PVSStudioTool)