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

Введение

LibreOffice ^[1] — очень крупный C++ проект. Поддерживать проект такого объёма — сложная задача для команды разработчиков. И, к сожалению, складывается впечатление, что качеству кода LibreOffice не удаётся уделять достаточного внимания.

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

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

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

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

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

В марте 2015 года был выполнен первый анализ LibreOffice ("Проверка проекта LibreOffice ^[3]") с помощью PVS-Studio. С тех пор офисный пакет сильно развился как продукт, но внутри всё также содержит множество ошибок. А некоторые паттерны ошибок вообще не поменялись с тех пор. Вот, например, ошибка из первой статьи:

V656 ^[4] 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

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

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

V656 ^[4] 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

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 ^[4] 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

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

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

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

V656 ^[4] 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

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

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

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

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

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

V765 ^[5] 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();
  }
  ....
}

Вот такой вот интересный «Hack» был найден с помощью диагностики V765 ^[5]. Если упростить строку кода с комментарием, то можно получить неожиданный результат:

1-й шаг:

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

2-й шаг:

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

3-й шаг

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

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

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

V567 ^[6] 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

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

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

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

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

V557 ^[7] Array overrun is possible. The 'nPageNum' index is pointing beyond array bound. pptx-epptooxml.cxx 1168

void PowerPointExport::ImplWriteNotes(sal_uInt32 nPageNum)
{
  ....
  // add slide implicit relation to notes
  if (mpSlidesFSArray.size() >= nPageNum)
      addRelation(mpSlidesFSArray[ nPageNum ]->getOutputStream(),
                  oox::getRelationship(Relationship::NOTESSLIDE),
                  OUStringBuffer()
                  .append("../notesSlides/notesSlide")
                  .append(static_cast<sal_Int32>(nPageNum) + 1)
                  .append(".xml")
                  .makeStringAndClear());
  ....
}

Последним валидным индексом должно являться значение, равное size() — 1. Но в этом фрагменте кода допустили ситуацию, когда индекс nPageNum может иметь значение mpSlidesFSArray.size(), из-за чего происходит выход за пределы массива и работа с элементом, состоящим из «мусора».

V557 ^[7] 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();
}

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

V557 ^[7] Array overrun is possible. The 'nXFIndex' index is pointing beyond array bound. xestyle.cxx 2613

sal_Int32 XclExpXFBuffer::GetXmlStyleIndex( sal_uInt32 nXFIndex ) const
{
    OSL_ENSURE( nXFIndex < maStyleIndexes.size(), "...." );
    if( nXFIndex > maStyleIndexes.size() )
        return 0;   // should be caught/debugged via above assert;
    return maStyleIndexes[ nXFIndex ];
}

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

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

V554 ^[8] Incorrect use of shared_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. dx_vcltools.cxx 158

struct RawRGBABitmap
{
  sal_Int32                     mnWidth;
  sal_Int32                     mnHeight;
  std::shared_ptr< sal_uInt8 >  mpBitmapData;
};

RawRGBABitmap bitmapFromVCLBitmapEx( const ::BitmapEx& rBmpEx )
{
  ....
  // convert transparent bitmap to 32bit RGBA
  // ========================================

  const ::Size aBmpSize( rBmpEx.GetSizePixel() );

  RawRGBABitmap aBmpData;
  aBmpData.mnWidth     = aBmpSize.Width();
  aBmpData.mnHeight    = aBmpSize.Height();
  aBmpData.mpBitmapData.reset( new sal_uInt8[ 4*aBmpData.mnWidth
                                               *aBmpData.mnHeight ] );
  ....
}

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

std::shared_ptr< sal_uInt8[] > mpBitmapData;

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

V568 ^[9] It's odd that the argument of 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]));
  ....
}

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

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

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! Т.е. препроцессор раскрыл не тот макрос, как же это могло произойти? Нам поможет определение макроса и комментарии разработчиков:

#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. Безопасный макрос не включился в код;
2. Другим макросом всё равно невозможно пользоваться, т.к. успешное инстанцирование шаблонной функции выполняется только если в тернарный оператор передать массивы одинакового размера. А в этом случае использование такого макроса теряет смысл.

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

V1013 ^[10] Suspicious subexpression f1.Pitch == f2.CharSet in a sequence of similar comparisons. xmldlg_export.cxx 1251

inline bool equalFont( Style const & style1, Style const & style2 )
{
  awt::FontDescriptor const & f1 = style1._descr;
  awt::FontDescriptor const & f2 = style2._descr;
  return (
      f1.Name == f2.Name &&
      f1.Height == f2.Height &&
      f1.Width == f2.Width &&
      f1.StyleName == f2.StyleName &&
      f1.Family == f2.Family &&
      f1.CharSet == f2.CharSet &&    // <=
      f1.Pitch == f2.CharSet &&      // <=
      f1.CharacterWidth == f2.CharacterWidth &&
      f1.Weight == f2.Weight &&
      f1.Slant == f2.Slant &&
      f1.Underline == f2.Underline &&
      f1.Strikeout == f2.Strikeout &&
      f1.Orientation == f2.Orientation &&
      bool(f1.Kerning) == bool(f2.Kerning) &&
      bool(f1.WordLineMode) == bool(f2.WordLineMode) &&
      f1.Type == f2.Type &&
      style1._fontRelief == style2._fontRelief &&
      style1._fontEmphasisMark == style2._fontEmphasisMark
      );
}

Ошибка является достойным кандидатом для пополнения статьи "Зло живёт в функциях сравнения ^[11]", если мы когда-нибудь решим её обновить или расширить. Думаю, вероятность найти такую ошибку (пропуск f2.Pitch) самостоятельно крайне мала. А вы как считаете?

V501 ^[12] There are identical sub-expressions 'mpTable[ocArrayColSep] != mpTable[eOp]' to the left and to the right of 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;
  ....
}

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

V517 ^[13] 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

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 ^[14] The 'then' statement is equivalent to the 'else' statement. svdpdf.hxx 146

/// Transform the rectangle (left, right, top, bottom) by this Matrix.
template <typename T> void Transform(....)
{
  ....
  if (top > bottom)
      top = std::max(leftTopY, rightTopY);
  else
      top = std::min(leftTopY, rightTopY);

  if (top > bottom)
      bottom = std::max(leftBottomY, rightBottomY);  // <=
  else
      bottom = std::max(leftBottomY, rightBottomY);  // <=
}

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

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

V533 ^[15] It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. javatypemaker.cxx 602

void printConstructors(....)
{
  ....
  for (std::vector<
                   unoidl::SingleInterfaceBasedServiceEntity::Constructor::
                   Parameter >::const_iterator j(i->parameters.begin());
                   j != i->parameters.end(); ++i)
  {
      o << ", ";
      printType(o, options, manager, j->type, false);
      if (j->rest) {
          o << "...";
      }
      o << ' '
        << codemaker::java::translateUnoToJavaIdentifier(
            u2b(j->name), "param");
  }
  ....
}

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

V756 ^[16] The 'nIndex2' counter is not used inside a nested loop. Consider inspecting usage of 'nIndex' counter. treex.cxx 34

SAL_IMPLEMENT_MAIN_WITH_ARGS(argc, argv)
{
  OString sXHPRoot;
  for (int nIndex = 1; nIndex != argc; ++nIndex)
  {
    if (std::strcmp(argv[nIndex], "-r") == 0)
    {
      sXHPRoot = OString( argv[nIndex + 1] );
      for( int nIndex2 = nIndex+3; nIndex2 < argc; nIndex2 = nIndex2 + 2 )
      {
        argv[nIndex-3] = argv[nIndex-1];
        argv[nIndex-2] = argv[nIndex];
      }
      argc = argc - 2;
      break;
    }
  }
  common::HandledArgs aArgs;
  if( !common::handleArguments(argc, argv, aArgs) )
  {
    WriteUsage();
    return 1;
  }
  ....
}

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

V1008 ^[17] Consider inspecting the 'for' operator. No more than one iteration of the loop 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] );
    ....
  }
  ....
}

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

V612 ^[18] An unconditional 'return' within a loop. pormulti.cxx 891

SwTextAttr const* MergedAttrIterMulti::NextAttr(....)
{
  ....
  SwpHints const*const pHints(m_pNode->GetpSwpHints());
  if (pHints)
  {
    while (m_CurrentHint < pHints->Count())
    {
      SwTextAttr const*const pHint(pHints->Get(m_CurrentHint));
      ++m_CurrentHint;
      rpNode = m_pNode;
      return pHint;
    }
  }
  return nullptr;
  ....
}

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

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

• 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

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

V637 ^[19] Two opposite conditions were encountered. The second condition is always false. Check lines: 281, 285. authfld.cxx 281

sal_uInt16  SwAuthorityFieldType::GetSequencePos(sal_IntPtr nHandle)
{
  ....
  SwTOXSortTabBase* pOld = aSortArr[i].get();
  if(*pOld == *pNew)
  {
    //only the first occurrence in the document
    //has to be in the array
    if(*pOld < *pNew)
      pNew.reset();
    else // remove the old content
      aSortArr.erase(aSortArr.begin() + i);
    break;
  }
  ....
}

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

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

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

V590 ^[20] Consider inspecting this expression. The expression is excessive or contains a misprint. fileurl.cxx 55

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

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

По мотивам подобных ошибок я даже написал теоретическую статью: "Логические выражения в C/C++. Как ошибаются профессионалы ^[21]".

V590 ^[20] Consider inspecting this expression. The expression is excessive or contains a 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;
  }
  ....
}

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

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 ^[20] Consider inspecting the '* pData <= MAXLEVEL && * pData <= 9' expression. The expression is excessive or contains a misprint. ww8par2.cxx 756

const sal_uInt8 MAXLEVEL = 10;

void SwWW8ImplReader::Read_ANLevelNo(....)
{
  ....
  // Range WW:1..9 -> SW:0..8 no bullets / numbering
  if (*pData <= MAXLEVEL && *pData <= 9)
  {
    ....
  }
  else if( *pData == 10 || *pData == 11 )
  {
      // remember type, the rest happens at Sprm 12
      m_xStyles->mnWwNumLevel = *pData;
  }
  ....
}

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

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

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

V757 ^[22] It is possible that an incorrect 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);
}

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

V668 ^[23] 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

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 подобных мест, что очень мало для такого объёма кода. Но всё равно это может приводить к проблемам у пользователей. Разработчикам следует убрать лишние проверки или создавать объекты таким образом:

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

V728 ^[24] An excessive 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))     // <=
    {
      ....
    }
  }
}

Очень давно диагностика V728 ^[24] была расширена до случаев, которые, скорое всего, не являются ошибочными, но усложняют код. А в сложном коде рано или поздно всё равно допускается ошибки.

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

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

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

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

V523 ^[14] 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);
    }
}

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

V1001 ^[25] The 'DL' variable is assigned but is not used by the end of the function. cipher.cxx 811

static void BF_updateECB(
    CipherContextBF    *ctx,
    rtlCipherDirection  direction,
    const sal_uInt8    *pData,
    sal_uInt8          *pBuffer,
    sal_Size            nLength)
{
    CipherKeyBF *key;
    sal_uInt32   DL, DR;

    key = &(ctx->m_key);
    if (direction == rtl_Cipher_DirectionEncode)
    {
        RTL_CIPHER_NTOHL64(pData, DL, DR, nLength);

        BF_encode(key, &DL, &DR);

        RTL_CIPHER_HTONL(DL, pBuffer);
        RTL_CIPHER_HTONL(DR, pBuffer);
    }
    else
    {
        RTL_CIPHER_NTOHL(pData, DL);
        RTL_CIPHER_NTOHL(pData, DR);

        BF_decode(key, &DL, &DR);

        RTL_CIPHER_HTONL64(DL, DR, pBuffer, nLength);
    }
    DL = DR = 0;
}

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

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

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 ^[26] Possible incorrect order of arguments 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 );
  ....
}

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

V794 ^[27] The assignment operator should be protected from the case of 'this == &rToBeCopied'. hommatrixtemplate.hxx 121

ImplHomMatrixTemplate& operator=(....)
{
  // complete initialization using copy
  for(sal_uInt16 a(0); a < (RowSize - 1); a++)
  {
    memcpy(&maLine[a], &rToBeCopied.maLine[a], sizeof(....));
  }
  if(rToBeCopied.mpLine)
  {
    mpLine.reset( new ImplMatLine< RowSize >(....) );
  }
  return *this;
}

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

Ошибки с SysAllocString

V649 ^[28] 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

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 ^[29] The return value of function 'SysAllocString' is required to be utilized. maccessible.cxx 1077

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

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

Разное

V716 ^[30] Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. maccessible.cxx 2649

BOOL
CMAccessible::get_IAccessibleFromXAccessible(....)
{
  ENTER_PROTECTED_BLOCK

  // #CHECK#
  if(ppIA == nullptr)
  {
      return E_INVALIDARG; // <=
  }
  BOOL isGet = FALSE;
  if(g_pAgent)
      isGet = g_pAgent->GetIAccessibleFromXAccessible(....);

  if(isGet)
      return TRUE;
  else
      return FALSE;

  LEAVE_PROTECTED_BLOCK
}

Одна из ветвей исполнения функции возвращает значение, тип которого не соответствует типу возвращаемого значения функции. Тип HRESULT имеет более сложный формат, чем тип BOOL, и предназначен для хранения статусов операций. Например, значение E_INVALIDARG равно 0x80070057L. Правильно будет написать, например, так:

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 ^[31] 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

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 будет инициализировано уже после того, как поучаствует в инициализации пяти других полей класса.

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

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

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

V763 ^[32] Parameter 'nNativeNumberMode' is always rewritten in function body before being used. calendar_jewish.cxx 286

OUString SAL_CALL
Calendar_jewish::getDisplayString(...., sal_Int16 nNativeNumberMode )
{
  // make Hebrew number for Jewish calendar
  nNativeNumberMode = NativeNumberMode::NATNUM2;

  if (nCalendarDisplayCode == CalendarDisplayCode::SHORT_YEAR) {
    sal_Int32 value = getValue(CalendarFieldIndex::YEAR) % 1000;
    return mxNatNum->getNativeNumberString(...., nNativeNumberMode );
  }
  else
    return Calendar_gregorian::getDisplayString(...., nNativeNumberMode );
}

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

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

• 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 обратят внимание на статические анализаторы кода и попробуют использовать их для повышения качества и надёжности проекта. Это будет полезно всем.

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

• Vk: @pvsstudio_rus ^[33]
• Telegram: @pvsstudio_rus ^[34]
• Instagram: @pvsstudio_rus ^[35]
• Twitter: @pvsstudio_rus ^[36]
• YouTube: @PVSStudioTool ^[37]

^[38]

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. LibreOffice: Accountant's Nightmare ^[38]

Автор: SvyatoslavMC

Источник ^[39]