Хабрахабр

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

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

Введение

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

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

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

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

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

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

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

It's probably an error or un-optimized code. V656 Variables 'aVRP', 'aVPN' are initialized through the call to the same function. GetVRP()' expression. Consider inspecting the 'rSceneCamera. viewcontactofe3dscene.cxx 178 Check lines: 177, 178.

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

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

It's probably an error or un-optimized code. V656 Variables 'aSdvURL', 'aStrURL' are initialized through the call to the same function. Check lines: 658, 659. Consider inspecting the 'pThm->GetSdvURL()' expression. gallery1.cxx 659

const INetURLObject& GetThmURL() const
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() ); // <= ....
}

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

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

It's probably an error or un-optimized code. V656 Variables 'nDragW', 'nDragH' are initialized through the call to the same function. GetStartDragWidth()' expression. Consider inspecting the 'rMSettings. winproc.cxx 472 Check lines: 471, 472.

class VCL_DLLPUBLIC MouseSettings
{ .... long GetStartDragWidth() const; long GetStartDragHeight() const; ....
} bool ImplHandleMouseEvent( .... )
{ .... long nDragW = rMSettings.GetStartDragWidth(); long nDragH = rMSettings.GetStartDragWidth(); ....
}

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

It's probably an error or un-optimized code. V656 Variables 'defaultZoomX', 'defaultZoomY' are initialized through the call to the same function. Check lines: 5673, 5674. Consider inspecting the 'pViewData->GetZoomX()' expression. gridwin.cxx 5674

OString ScGridWindow::getCellCursor(....) const
{ .... SCCOL nX = pViewData->GetCurX(); SCROW nY = pViewData->GetCurY(); Fraction defaultZoomX = pViewData->GetZoomX(); Fraction defaultZoomY = pViewData->GetZoomX(); // <= ....
}

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

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

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

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

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

1-й шаг:

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

2-й шаг:

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

3-й шаг

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

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

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

This may lead to undefined behavior. V567 The modification of the 'nCount' variable is unsequenced relative to another operation on the same variable. stgio.cxx 214

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

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

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

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

The 'nPageNum' index is pointing beyond array bound. V557 Array overrun is possible. 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(), из-за чего происходит выход за пределы массива и работа с элементом, состоящим из «мусора».

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

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

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

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

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

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

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

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

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

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

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

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

formulacompiler.cxx 632 V501 There are identical sub-expressions 'mpTable[ocArrayColSep] != mpTable[eOp]' to the left and to the right of the '&&' operator.

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

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

There is a probability of logical error presence. V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. mysqlc_databasemetadata.cxx 781 Check lines: 781, 783.

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 никогда не выставляется.

svdpdf.hxx 146 V523 The 'then' statement is equivalent to the 'else' statement.

/// 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(). Наверняка из-за этой опечатки в интерфейсе что-то странно масштабируется.

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

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

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

No more than one iteration of the loop will be performed. V1008 Consider inspecting the 'for' operator. 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 итерации. Непонятно, зачем это сделано именно таким способом.

pormulti.cxx 891 V612 An unconditional 'return' within a loop.

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

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

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

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 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++.

The expression is excessive or contains a misprint. V590 Consider inspecting this expression. 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 диапазона, заданных в условии числами. Разработчики допустили ошибку.

The expression is excessive or contains a misprint. V590 Consider inspecting the '* pData <= MAXLEVEL && * pData <= 9' expression. 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 )
{ ....
}

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

Check lines: 2709, 2710. V757 It is possible that an incorrect variable is compared with nullptr after type conversion using 'dynamic_cast'. 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.

The exception will be generated in the case of memory allocation error. V668 There is no sense in testing the 'm_pStream' pointer against null, as the memory was allocated using the 'new' operator. 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 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 была расширена до случаев, которые, скорое всего, не являются ошибочными, но усложняют код. А в сложном коде рано или поздно всё равно допускается ошибки.

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

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

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

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

docxattributeoutput.cxx 1571 V523 The 'then' statement is equivalent to the 'else' statement.

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

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

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

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 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. В коде нашлось место, где соответствующие аргументы перепутаны местами при вызове этой функции.

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

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

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

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

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

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

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

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

Теги
Показать больше

Похожие статьи

Добавить комментарий

Ваш e-mail не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»
Закрыть