Geekly Articles each Day
📜 ⬆️ ⬇️

Check LibreOffice project

We offer the reader another article about checking the well-known open-source project. This time we checked the LibreOffice project, which is an office suite. More than 480 programmers take part in its development. The code turned out to be a very high quality and regularly tested static Coverity analyzer. But, as in any other large project, new errors and omissions were found, which we will describe in the article. For a change, this time we will be accompanied not by unicorns, but by cows.

LibreOffice is a powerful office suite, fully compatible with 32/64-bit systems. Translated into more than 30 languages ​​of the world. Supports most popular operating systems, including GNU / Linux, Microsoft Windows and Mac OS X.

LibreOffice is free and open source. Written in languages: Java, Python, C ++. The part of the project that was written in C ++ (and a little bit in C, C ++ / CLI ) was subjected to analysis. Version: 4.5.0.0.alpha0 + (Git revision: 368367).

The analysis was performed using the PVS-Studio static code analyzer.
')
Consider what errors were found, and what can be done with them. I want to note that some errors may not be errors. I am not familiar with the code, and could have mistaken a completely correct code. However, since this code is confusing the analyzer and me, something is wrong. This code smells, and it is good to refactor it in order to reduce the likelihood of its misunderstanding in the process of project development.

Typos


Any code is not without typos. Many, of course, are and are corrected at the testing stage, but some remain to live inside programs for a long time. As a rule, they are in rarely used functions or do not have a strong influence on the operation of the program.

For example, we encountered the following comparison function, which only works for one third:
class SvgGradientEntry { .... bool operator==(const SvgGradientEntry& rCompare) const { return (getOffset() == rCompare.getOffset() && getColor() == getColor() && getOpacity() == getOpacity()); } .... }

PVS-Studio warning : V501 operator: getColor () == getColor () svggradientprimitive2d.hxx 61

This mistake probably doesn’t do much harm. Probably, this operator '==' is not used at all. However, once the analyzer was able to find this error, it will be able to find more serious ones immediately after writing a new code. Therefore, the main value of static analysis is not in one-time runs, but in regular use.

How could one try to avoid such an error? I do not know. Perhaps, if we train ourselves to more carefully align the blocks of the same type of code, then the error would be more noticeable. For example, the function can be arranged as follows:
 bool operator==(const SvgGradientEntry& rCompare) const { return getOffset() == rCompare.getOffset() && getColor() == getColor() && getOpacity() == getOpacity(); }

Now it has become more noticeable that in the right column there is not enough “rCompare”. Although to be honest, not very noticeable. May not help. Humans tend to make mistakes. And therefore the static analyzer is a good helper.

And here is an example where a typo could clearly have been avoided. Someone unsuccessfully wrote code to exchange values ​​between two variables.



 void TabBar::ImplGetColors(....) { .... aTempColor = rFaceTextColor; rFaceTextColor = rSelectTextColor; rSelectTextColor = rFaceTextColor; .... }

PVS-Studio warning: V587 An odd sequence of assignments of this kind: A = B; B = A ;. Check lines: 565, 566. tabbar.cxx 566

In the last line, instead of 'rFaceTextColor' one should use 'aTempColor'.

There was no need to write code for exchanging values ​​"manually." It would be easier and more reliable to use the standard function std :: swap ():
 swap(rFaceTextColor, rSelectTextColor);

We continue. I think it is impossible to defend myself from the next mistake. Pure typo:
 void SAL_CALL Theme::disposing (void) { ChangeListeners aListeners; maChangeListeners.swap(aListeners); const lang::EventObject aEvent (static_cast<XWeak*>(this)); for (ChangeListeners::const_iterator iContainer(maChangeListeners.begin()), iContainerEnd(maChangeListeners.end()); iContainerEnd!=iContainerEnd; ++iContainerEnd) { .... } }

PVS-Studio warning: V501 operator: iContainerEnd! = IContainerEnd theme.cxx 439

The loop will not be executed, since the condition “iContainerEnd! = IContainerEnd” is always false. Summed up the similar names of iterators. In fact, it was necessary to write: "iContainer! = IContainerEnd". By the way, there seems to be another mistake here. It is strange that the iterator “iContainerEnd” increases.

Consider another bad cycle:
 static void lcl_FillSubRegionList(....) { .... for( IDocumentMarkAccess::const_iterator_t ppMark = pMarkAccess->getBookmarksBegin(); <<<<---- ppMark != pMarkAccess->getBookmarksBegin(); <<<<---- ++ppMark) { const ::sw::mark::IMark* pBkmk = ppMark->get(); if( pBkmk->IsExpanded() ) rSubRegions.InsertEntry( pBkmk->GetName() ); } }

PVS-Studio Warning: V625 Consider inspecting the 'for' operator. Initial and final values ​​of the iterator are the same. uiregionsw.cxx 120

The cycle will not be executed. In the condition, the iterator 'ppMark' must be compared with 'pMarkAccess-> getBookmarksEnd ()'. I don’t have any ideas how to protect myself from such an error with the help of code-writing rules. Just a typo.

By the way, sometimes there is an error, but it does not affect the correct execution of the program. Now I’ll demonstrate one of these typos:
 bool PolyPolygonEditor::DeletePoints(....) { bool bPolyPolyChanged = false; std::set< sal_uInt16 >::const_reverse_iterator aIter;( rAbsPoints.rbegin() ); for( aIter = rAbsPoints.rbegin(); aIter != rAbsPoints.rend(); ++aIter ) .... }

PVS-Studio warning : V530 The return value of the function 'rbegin' is required to be utilized. polypolygoneditor.cxx 38

Error here: aIter; (rAbsPoints.rbegin ());

We wanted to initialize an iterator. But accidentally wedged a semicolon. The iterator remains uninitialized. And the expression "(rAbsPoints.rbegin ());" dangles in the air and does nothing.

The situation is saved by the fact that in the loop the iterator is still initialized with the desired value. In general, there is no error, but it is better to remove the extra expression. By the way, this cycle was duplicated using Copy-Paste, so it is worth looking back at line 69 and 129 in the same file.

Finally a typo in the class constructor:
 XMLTransformerOOoEventMap_Impl::XMLTransformerOOoEventMap_Impl( XMLTransformerEventMapEntry *pInit, XMLTransformerEventMapEntry *pInit2 ) { if( pInit ) AddMap( pInit ); if( pInit ) AddMap( pInit2 ); }

PVS-Studio warning: V581 The conditional expressions of the 'if' are agreed alongside each other are identical. Check lines: 77, 79. eventoootcontext.cxx 79

The second 'if' statement should check the 'pInit2' pointer.

May be so conceived, but very suspicious


There are several code snippets that seem to contain typos. But I'm not sure. Perhaps so conceived.
 class VCL_DLLPUBLIC MouseSettings { .... long GetStartDragWidth() const; long GetStartDragHeight() const; .... } bool ImplHandleMouseEvent( .... ) { .... long nDragW = rMSettings.GetStartDragWidth(); long nDragH = rMSettings.GetStartDragWidth(); .... }

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

It is not clear whether the variables nDragW and nDragH must be initialized with the same value or not. If yes, then there is not enough comment. Or it would be better to explicitly write:
 long nDragW = rMSettings.GetStartDragWidth(); long nDragH = nDragW;

Similar situation:
 void Edit::ImplDelete(....) { .... maSelection.Min() = aSelection.Min(); maSelection.Max() = aSelection.Min(); .... }

V656 Variables 'maSelection.Min ()', 'maSelection.Max ()' are initialized through the call to the same function. It's probably not an error or un-optimized code. Consider inspecting the 'aSelection.Min ()' expression. Check lines: 756, 757. edit.cxx 757

For those who work with the project, everything is immediately clear. I do not work, and therefore I don’t know if there is a mistake or not.

And the last case. There are three functions in a class:However, here, to initialize the constant 'aVPN', the function GetVRP () is used.
 void ViewContactOfE3dScene::createViewInformation3D(....) { .... const basegfx::B3DPoint aVRP(rSceneCamera.GetVRP()); const basegfx::B3DVector aVPN(rSceneCamera.GetVRP()); <<<--- const basegfx::B3DVector aVUV(rSceneCamera.GetVUV()); .... }

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

The analyzer issued another warning V656. I am pretty sure that there is a real mistake. But I will not give the code, as it is cumbersome. I ask the developers to look here:

Copy paste




I have to admit that, without Copy-Paste, programming will at times be extremely tedious and boring. Without Ctrl-C, Ctrl-V programming will not work, as it would not want to disable these shortcuts. Therefore, I will not urge not to copy the code. But I urge everyone: copying the code, be careful and alert!
 uno::Sequence< OUString > SwXTextTable::getSupportedServiceNames(void) { uno::Sequence< OUString > aRet(4); OUString* pArr = aRet.getArray(); pArr[0] = "com.sun.star.document.LinkTarget"; pArr[1] = "com.sun.star.text.TextTable"; pArr[2] = "com.sun.star.text.TextContent"; pArr[2] = "com.sun.star.text.TextSortable"; return aRet; }

PVS-Studio warning : V519 The 'pArr [2]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 3735, 3736. unotbl.cxx 3736

Classic last line effect . I am almost sure that the last line was obtained from the penultimate. Replaced "Content" with "Sortable", but forgot about the index '2'.

Very similar case:
 Sequence<OUString> FirebirdDriver::getSupportedServiceNames_Static() { Sequence< OUString > aSNS( 2 ); aSNS[0] = "com.sun.star.sdbc.Driver"; aSNS[0] = "com.sun.star.sdbcx.Driver"; return aSNS; }

PVS-Studio warning: V519 The 'aSNS [0]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 137, 138. driver.cxx 138

But the worst thing is that sometimes due to Copy-Paste, errors multiply. I will show it on an example. Unfortunately, the code will be somewhat difficult to read. Be patient.

So, the program has this function:
 static bool GetPropertyValue( ::com::sun::star::uno::Any& rAny, const ::com::sun::star::uno::Reference< ::com::sun::star::beans::XPropertySet > &, const OUString& rPropertyName, bool bTestPropertyAvailability = false );

Note that the last 'bTestPropertyAvailability' argument is optional.

I also have to say what sal_True is:
 #define sal_True ((sal_Bool)1)

Now actually a mistake. See how the function GetPropertyValue () is called:
 sal_Int32 PPTWriterBase::GetLayoutOffset(....) const { ::com::sun::star::uno::Any aAny; sal_Int32 nLayout = 20; if ( GetPropertyValue( aAny, rXPropSet, OUString( "Layout" ) ), sal_True ) aAny >>= nLayout; DBG(printf("GetLayoutOffset %" SAL_PRIdINT32 "\n", nLayout)); return nLayout; }

PVS-Studio warning : Consider inspecting the expression for 'GetPropertyValue' function call. It is possible that one of the closing ')' brackets was positioned incorrectly. pptx-epptbase.cxx 442

If you look closely, it turns out that one of the closing parentheses is not in its place. As a result, the GetPropertyValue () function receives not the 'sal_True' as the last argument, but the default value (equal to 'false').

But this is only half the trouble. Additionally, the work of the 'if' operator deteriorated. The condition looks like this:
 if (foo(), sal_True)

Operator comma returns its right hand side. As a result, the condition is always true.

The error in this code is not related to Copy-Paste. Common typo. Not there is a bracket. It happens.

The sad thing is that this error was propagated in other parts of the program. If in one place the error is corrected, then the probability is high that in other places it will remain.

Copy-Paste led to this error in 9 more places:In conclusion, section 3 of the last non-critical warning. Just one extra check:
 #define CHECK_N_TRANSLATE( name ) \ else if (sServiceName == SERVICE_PERSISTENT_COMPONENT_##name) \ sToWriteServiceName = SERVICE_##name void OElementExport::exportServiceNameAttribute() { .... CHECK_N_TRANSLATE( FORM ); <<<<---- CHECK_N_TRANSLATE( FORM ); <<<<---- CHECK_N_TRANSLATE( LISTBOX ); CHECK_N_TRANSLATE( COMBOBOX ); CHECK_N_TRANSLATE( RADIOBUTTON ); CHECK_N_TRANSLATE( GROUPBOX ); CHECK_N_TRANSLATE( FIXEDTEXT ); CHECK_N_TRANSLATE( COMMANDBUTTON ); .... }

PVS-Studio warning : V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 177, 178. elementexport.cxx 177

Nothing terrible, but a defect. Two more extra checks can be found here:

Brave use of the realloc () function


The realloc () function is used so obviously insecurely that I don’t risk calling it a mistake. Apparently, this is a conscious decision of the authors. If the memory could not be allocated using malloc () / realloc (), then let the program better immediately fall. There is nothing to "kick". Anyway, if the program continues to work, it is unlikely that any good will come of this. But it is not fair to count the messages issued by the analyzer for false positives. Therefore, consider what the analyzer did not like.

For example, let's study the implementation of the add () function in the FastAttributeList class:
 void FastAttributeList::add(sal_Int32 nToken, const sal_Char* pValue, size_t nValueLength ) { maAttributeTokens.push_back( nToken ); sal_Int32 nWritePosition = maAttributeValues.back(); maAttributeValues.push_back( maAttributeValues.back() + nValueLength + 1 ); if (maAttributeValues.back() > mnChunkLength) { mnChunkLength = maAttributeValues.back(); mpChunk = (sal_Char *) realloc( mpChunk, mnChunkLength ); } strncpy(mpChunk + nWritePosition, pValue, nValueLength); mpChunk[nWritePosition + nValueLength] = '\0'; }

PVS-Studio warning : V701 realloc () possible leak: when realloc () fails in allocating memory, original pointer 'mpChunk' is lost. Consider assigning realloc () to a temporary pointer. fastattribs.cxx 88

The main problem with this code is that the result of the realloc () function is not checked. Of course, a situation where it will not be possible to allocate memory is very rare. But imagine - it happened. Then realloc () returns NULL. Then an emergency situation will arise, when the function strncpy () starts copying the data do not understand where:
  mpChunk = (sal_Char *) realloc( mpChunk, mnChunkLength ); } strncpy(mpChunk + nWritePosition, pValue, nValueLength);

But the analyzer does not like the other. Suppose there is some kind of error handler. And the program will continue its execution. Here only memory leak arises. The variable mpChunk will be written to 0, and it is no longer possible to free memory. I will explain this error pattern in a little more detail. Many do not think and misuse realloc ().

Consider an artificial code example:

 char *p = (char *)malloc(10); .... p = (char *)realloc(p, 10000);


If the memory cannot be allocated, the variable 'p' will be “corrupted”. Now there is no way to free the memory, the pointer to which was stored in the 'p'.

In this form, the error is obvious. But in practice, such code is quite common. The analyzer generates 8 more warnings on this topic, but there is no sense to consider them. Anyway, LibreOffice believes that memory can always be allocated.

Errors in logic


Met a series of funny mistakes in the conditions. The reasons, apparently, are different: inattention, typos, insufficient knowledge of the language.



 void ScPivotLayoutTreeListData::PushDataFieldNames(....) { .... ScDPLabelData* pLabelData = mpParent->GetLabelData(nColumn); if (pLabelData == NULL && pLabelData->maName.isEmpty()) continue; .... }

PVS-Studio warning : V522 Dereferencing of the null pointer 'pLabelData' might take place. Check the logical condition. pivotlayouttreelistdata.cxx 157

Logical error in the condition. If the pointer is zero, then we dereference it. As I understand it, here it was necessary to use the operator ||.

Similar error:
 void grabFocusFromLimitBox( OQueryController& _rController ) { .... vcl::Window* pWindow = VCLUnoHelper::GetWindow( xWindow ); if( pWindow || pWindow->HasChildPathFocus() ) { pWindow->GrabFocusToDocument(); } .... }

PVS-Studio warning: V522 Dereferencing of the null pointer 'pWindow' might take place. Check the logical condition. querycontroller.cxx 293

Here, on the contrary, instead of '||' should have written '&&'.

Now for a slightly more complicated condition:
 enum SbxDataType { SbxEMPTY = 0, SbxNULL = 1, .... }; void SbModule::GetCodeCompleteDataFromParse(CodeCompleteDataCache& aCache) { .... if( (pSymDef->GetType() != SbxEMPTY) || (pSymDef->GetType() != SbxNULL) ) .... }

PVS-Studio warning : V547 Expression is always true. Probably the '&&' operator should be used here. sbxmod.cxx 1777

For simplicity, I will rewrite the expression:
 if (type != 0 || type != 1)

The condition is always true.

Two similar errors can be found here:
There are two places where the condition is redundant. I think these are errors:
 sal_uInt16 ScRange::ParseCols(....) { .... const sal_Unicode* p = rStr.getStr(); .... case formula::FormulaGrammar::CONV_XL_R1C1: if ((p[0] == 'C' || p[0] != 'c') && NULL != (p = lcl_r1c1_get_col( p, rDetails, &aStart, &ignored ))) { .... }

PVS-Studio warning: V590 Consider inspecting the 'p [0] ==' C '|| p [0]! = 'c' 'expression. The expression is misprint. address.cxx 1593

The condition (p [0] == 'C' || p [0]! = 'C') can be shortened to (p [0]! = 'C'). I am sure that this is an error and the condition should be like this: (p [0] == 'C' || p [0] == 'c').

An identical error can be found in the same file below:
Perhaps, errors in logic can be attributed to the situation when the pointer is dereferenced at the beginning, and then only checked for equality to zero. This is a very common mistake in all programs. Usually it arises due to inattention in the process of refactoring code.

Typical example:
 IMPL_LINK(....) { .... SystemWindow *pSysWin = pWindow->GetSystemWindow(); MenuBar *pMBar = pSysWin->GetMenuBar(); if ( pSysWin && pMBar ) { AddMenuBarIcon( pSysWin, true ); } .... }

PVS-Studio warning: V595 The 'pSysWin' pointer was used before it was verified against nullptr. Check lines: 738, 739. updatecheckui.cxx 738

The 'pSysWin' pointer is dereferenced in the 'pSysWin-> GetMenuBar ()' expression. Then it is checked for equality to zero.

I suggest that the creators of LibreOffice also pay attention to these places: LibreOffice-V595.txt .

And last, this time a more complicated situation. If you are tired, you can skip this place. Consider the usual listing:
 enum BRC_Sides { WW8_TOP = 0, WW8_LEFT = 1, WW8_BOT = 2, WW8_RIGHT = 3, WW8_BETW = 4 };

Note that named constants are not powers of two. These are just numbers. Including there is 0.

They work with these constants as if they represent a power of two. Try to mask select and check individual bits:
 void SwWW8ImplReader::Read_Border(....) { .... if ((nBorder & WW8_LEFT)==WW8_LEFT) aBox.SetDistance( (sal_uInt16)aInnerDist.Left(), BOX_LINE_LEFT ); if ((nBorder & WW8_TOP)==WW8_TOP) aBox.SetDistance( (sal_uInt16)aInnerDist.Top(), BOX_LINE_TOP ); if ((nBorder & WW8_RIGHT)==WW8_RIGHT) aBox.SetDistance( (sal_uInt16)aInnerDist.Right(), BOX_LINE_RIGHT ); if ((nBorder & WW8_BOT)==WW8_BOT) aBox.SetDistance( (sal_uInt16)aInnerDist.Bottom(), BOX_LINE_BOTTOM ); .... }

PVS-Studio warning : V616 The 'WW8_TOP' is called in the bitwise operation. ww8par6.cxx 4742

These are wrong actions. For example, the condition ((nBorder & WW8_TOP) == WW8_TOP) is always true. For the explanation I will substitute numbers: ((nBorder & 0) == 0).

The check on WW8_LEFT will also work incorrectly if the nBorder variable has the WW8_RIGHT value equal to 3. Substitute ((3 & 1) == 1). It turns out that WW8_RIGHT is taken as WW8_LEFT.

Skeleton in the closet


The analyzer from time to time detects abnormal places in the code. This is not a mistake, but a cunning idea. Touching them makes no sense, but it may be interesting to see. Here is one of those cases where the analyzer did not like the argument of the free () function:
 /* This operator is supposed to be unimplemented, but that now leads * to compilation and/or linking errors with MSVC2008. (Don't know * about MSVC2010.) As it can be left unimplemented just fine with * gcc, presumably it is never called. So do implement it then to * avoid the compilation and/or linking errors, but make it crash * intentionally if called. */ void SimpleReferenceObject::operator delete[](void * /* pPtr */) { free(NULL); }

Safety engineering




The analyzer revealed a number of points that make the program code dangerous. The dangers are diverse in nature, but I decided to put them in one section.
 void writeError( const char* errstr ) { FILE* ferr = getErrorFile( 1 ); if ( ferr != NULL ) { fprintf( ferr, errstr ); fflush( ferr ); } }

PVS-Studio warning : V618 It’s dangerous to call the format. The example of the safe code: printf ("% s", str); unoapploader.c 405

If there are control characters in the string 'errstr', anything can happen. The program may fall, it may write garbage to the file, or something else will happen ( details ).

It will be correct to write like this:
 fprintf( ferr, "%s", errstr );

Two more places where the printf () function is incorrectly used:
Now about the dangerous use of dynamic_cast .
 virtual ~LazyFieldmarkDeleter() { dynamic_cast<Fieldmark&> (*m_pFieldmark.get()).ReleaseDoc(m_pDoc); }

PVS-Studio warning: V509 The 'dynamic_cast <T &>' operator should be located inside the try..catch block. Raising exception inside the destructor is illegal. docbm.cxx 846

When working with links, if the conversion cannot be performed, the dynamic_cast statement throws an exception std :: bad_cast.

If an exception occurs in the program, the stack collapses, during which objects are destroyed by calling destructors. If the destructor of the object being destroyed when the stack collapses throws another exception, and this exception leaves the destructor, the C ++ library immediately crashes the program by calling the terminate () function. It follows from this that destructors should never propagate exceptions. The exception thrown inside the destructor must be processed inside the same destructor.

For the same reason, it is dangerous to call the operator new in destructors. This statement, when there is a shortage of memory, will throw an exception std :: bad_alloc. It is a good idea to wrap it in a try-catch block.

Example of dangerous code:
 WinMtfOutput::~WinMtfOutput() { mpGDIMetaFile->AddAction( new MetaPopAction() ); .... }

Warnings PVS-Studio: V509 The operator should not be located inside the camera. Raising exception inside the destructor is illegal. winmtf.cxx 852

Other dangerous actions in the destructor:
By the way, since we were talking about operator new, I would note the danger of this code:
 extern "C" oslFileHandle SAL_CALL osl_createFileHandleFromOSHandle( HANDLE hFile, sal_uInt32 uFlags) { if ( !IsValidHandle(hFile) ) return 0; // EINVAL FileHandle_Impl * pImpl = new FileHandle_Impl(hFile); if (pImpl == 0) { // cleanup and fail (void) ::CloseHandle(hFile); return 0; // ENOMEM } .... }

PVS-Studio warning : V668 It was no use. The exception will be generated in the case of memory allocation error. file.cxx 663

The 'new' operator generates an exception when there is insufficient memory. Thus, checking the pointer that the operator returned does not make sense. It is always not equal to 0. If there is not enough memory, the CloseHandle () function will not be called:
 FileHandle_Impl * pImpl = new FileHandle_Impl(hFile); if (pImpl == 0) { // cleanup and fail (void) ::CloseHandle(hFile); return 0; // ENOMEM }

I could be wrong. I do not know the project LibreOffice. Perhaps the developers use a special version of the libraries in which the operator 'new' does not throw an exception, but returns nullptr. If this is the case, then please simply ignore the warnings with the number V668. They can be turned off so that they do not interfere.

If the new operator throws an exception, I recommend viewing the following 126 messages: LibreOffice-V668.txt .

The following danger lies in the implementation of one of the functions of DllMain:
 BOOL WINAPI DllMain( HINSTANCE hinstDLL, DWORD fdwReason, LPVOID lpvReserved ) { .... CreateThread( NULL, 0, ParentMonitorThreadProc, (LPVOID)dwParentProcessId, 0, &dwThreadId ); .... }

PVS-Studio warning : V718 The 'CreateThread' function should not be called from the 'DllMain' function. dllentry.c 308

Many functions cannot be called inside the DllMain () function, as this may cause the application to freeze or other errors. These functions include CreateThread ().

The situation with DllMain is well described in an article on MSDN: Dynamic-Link Library Best Practices .

This code may work, but it is dangerous and someday may fail.

There is a situation where the wcsncpy () function can cause a buffer overflow:
 typedef struct { .... WCHAR wszTitle[MAX_COLUMN_NAME_LEN]; WCHAR wszDescription[MAX_COLUMN_DESC_LEN]; } SHCOLUMNINFO, *LPSHCOLUMNINFO; HRESULT STDMETHODCALLTYPE CColumnInfo::GetColumnInfo( DWORD dwIndex, SHCOLUMNINFO *psci) { .... wcsncpy(psci->wszTitle, ColumnInfoTable[dwIndex].wszTitle, (sizeof(psci->wszTitle) - 1)); return S_OK; }

PVS-Studio: V512 A call of the 'wcsncpy' function will lead to overflow of the buffer 'psci->wszTitle'. columninfo.cxx 129

(sizeof(psci->wszTitle) — 1) . :
 (sizeof(psci->wszTitle) / sizeof(psci->wszTitle[0]) - 1)

, , memset(). Example:
 static void __rtl_digest_updateMD2 (DigestContextMD2 *ctx) { .... sal_uInt32 state[48]; .... memset (state, 0, 48 * sizeof(sal_uInt32)); }

PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'state' buffer. The RtlSecureZeroMemory () function should be used to erase the private data. digest.cxx 337

. , , .

memset(), . . - .

Details:
  1. V597. The compiler could delete the 'memset' function call, which is used to flush 'Foo' buffer .
  2. — ?
  3. Zero and forget — caveats of zeroing memory in C .
, : LibreOffice-V597.txt .


 Guess::Guess() { language_str = DEFAULT_LANGUAGE; country_str = DEFAULT_COUNTRY; encoding_str = DEFAULT_ENCODING; } Guess::Guess(const char * guess_str) { Guess(); .... }

PVS-Studio: V603 The object was created but it is not being used. If you wish to call constructor, 'this->Guess::Guess(....)' should be used. guess.cxx 56

, , ++. . , , . - . .

: camera3d.cxx 46
 sal_uInt32 readIdent(....) { size_t nItems = rStrings.size(); const sal_Char** pStrings = new const sal_Char*[ nItems+1 ]; .... delete pStrings; return nRet; }

PVS-Studio warning : V611, it was allocated using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] pStrings;'. profile.hxx 103

Correctly: delete [] pStrings ;.

Another warning about incorrect memory release:
 static const int kConventionShift = 16; static const int kFlagMask = ~((~int(0)) << kConventionShift);

PVS-Studio warning: V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~int (0))' is negative. grammar.hxx 56

- ( ).
 sal_Int32 GetMRest() const {return m_nRest;} OUString LwpBulletStyleMgr::RegisterBulletStyle(....) { .... if (pIndent->GetMRest() > 0.001) .... }

PVS-Studio: V674 The '0.001' literal of the 'double' type is compared to a value of the 'long' type. Consider inspecting the 'pIndent->GetMRest() > 0.001' expression. lwpbulletstylemgr.cxx 177

Something is wrong here. 0.001.

:
 BOOL SHGetSpecialFolderPath( HWND hwndOwner, _Out_ LPTSTR lpszPath, _In_ int csidl, _In_ BOOL fCreate ); #define FAILED(hr) (((HRESULT)(hr)) < 0) OUString UpdateCheckConfig::getDesktopDirectory() { .... if( ! FAILED( SHGetSpecialFolderPathW( .... ) ) ) .... }

PVS-Studio: V716 Suspicious type conversion: BOOL -> HRESULT. updatecheckconfig.cxx 193

, SHGetSpecialFolderPath() HRESULT. , , BOOL. , FAILED.

: updatecheckconfig.cxx 222

FAILED. HRESULT :
 bool UniscribeLayout::LayoutText( ImplLayoutArgs& rArgs ) { .... HRESULT nRC = ScriptItemize(....); if( !nRC ) // break loop when everything is correctly itemized break; .... }

PVS-Studio: V545 Such conditional expression of 'if' operator is incorrect for the HRESULT type value 'nRC'. The SUCCEEDED or FAILED macro should be used instead. winlayout.cxx 1115

, :
 void Reader::ClearTemplate() { if( pTemplate ) { if( 0 == pTemplate->release() ) delete pTemplate, pTemplate = 0; } }

PVS-Studio: V626 Consider checking for misprints. It's possible that ',' should be replaced by ';'. shellio.cxx 549

:
 void TabBar::ImplInit( WinBits nWinStyle ) { .... mbMirrored = false; mbMirrored = false; .... }

PVS-Studio: V519 The 'mbMirrored' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 415, 416. tabbar.cxx 416

: V519 The 'aParam.mpPreviewFontSet' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 4561, 4562. output2.cxx 4562

, :
 static bool CallRsc2(....) { .... if( !rsc_strnicmp( ...., "-fp=", 4 ) || !rsc_strnicmp( ...., "-fo=", 4 ) || !rsc_strnicmp( ...., "-presponse", 9 ) || <<<<---- !rsc_strnicmp( ...., "-rc", 3 ) || !rsc_stricmp( ...., "-+" ) || !rsc_stricmp( ...., "-br" ) || !rsc_stricmp( ...., "-bz" ) || !rsc_stricmp( ...., "-r" ) || ( '-' != *.... ) ) .... }

PVS-Studio: V666 Consider inspecting third argument of the function 'rsc_strnicmp'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. start.cxx 179

"-presponse" 10, 9 .

'break' :
 OUString getExtensionFolder(....) { .... while (xResultSet->next()) { title = Reference<sdbc::XRow>( xResultSet, UNO_QUERY_THROW )->getString(1 /* Title */ ) ; break; } return title; }

PVS-Studio: V612 An unconditional 'break' within a loop. dp_manager.cxx 100

:
:
 BSTR PromptNew(long hWnd) { .... ADOConnection* piTmpConnection = NULL; ::CoInitialize( NULL ); hr = CoCreateInstance( CLSID_DataLinks, NULL, CLSCTX_INPROC_SERVER, IID_IDataSourceLocator, (void**)&dlPrompt ); if( FAILED( hr ) ) { piTmpConnection->Release(); dlPrompt->Release( ); return connstr; } .... }

PVS-Studio: V522 Dereferencing of the null pointer 'piTmpConnection' might take place. adodatalinks.cxx 84

CoCreateInstance() , 'piTmpConnection' NULL.


. , .

, . , . . , .

, PVS-Studio .


, , , . , . , , , , .



Example:
 string getexe(string exename, bool maybeempty) { char* cmdbuf; size_t cmdlen; _dupenv_s(&cmdbuf, &cmdlen, exename.c_str()); if(!cmdbuf) { if (maybeempty) { return string(); } cout << "Error " << exename << " not defined. " "Did you forget to source the environment?" << endl; exit(1); } string command(cmdbuf); free(cmdbuf); return command; }

'exename' . : V813 Decreased performance. The 'exename' argument should probably be rendered as a constant reference. wrapper.cxx 18

:
 string getexe(const string &exename, bool maybeempty)

«». , , 20 « const » :

. « C++. 55 » — .: , 2006. — 300 .: . ISBN 5-94074-304-8

V801 . , 465 , : LibreOffice-V801-V813.txt .


. « 6. » :

. ++. 35 : . from English — .: , 2000. — 304 .: . ( « »). ISBN 5-94074-033-2. 32.973.26-018.1.

, 'A++' '++A' . , , ( ).

Code example:
 typename InterfaceMap::iterator find(const key &rKey) const { typename InterfaceMap::iterator iter = m_pMap->begin(); typename InterfaceMap::iterator end = m_pMap->end(); while( iter != end ) { equalImpl equal; if( equal( iter->first, rKey ) ) break; iter++; } return iter; }

PVS_Studio: V803 Decreased performance. In case 'iter' is iterator it's more effective to use prefix form of increment. Replace iterator++ with ++iterator. interfacecontainer.h 405

«iter++» "++iter". , . , , 257 , : LibreOffice-V803.txt .

,


, , . :
 BOOL GetMsiProp(....) { .... char* buff = reinterpret_cast<char*>( malloc( nbytes ) ); .... return ( strlen(buff) > 0 ); }

PVS-Studio warning : V805 Decreased performance. It is not necessary to identify the string by using 'strlen (str)> 0' construct. A more efficient way is to check: str [0]! = '\ 0'. sellang.cxx 49 The

inefficiency is that you need to loop through all the characters in the string until a terminal zero is encountered . But it suffices to check only one byte:
 return buff[0] != '\0';

This code is not very beautiful, so it would be better to have a special function:
 inline bool IsEmptyStr(const char *s) { return s == nullptr || s[0] == '\0'; }

Here there was an extra check for pointer 0 equality. I do not really like it and you can think about other options. But still, this function will work faster than strlen ().

Other ineffective checks: LibreOffice-V805.txt .

Other


There are some more warnings of the analyzer that may seem interesting: LibreOffice-V804_V811.txt .

Number of false positives


In the article I mentioned 240 warnings that seemed interesting to me. In total, the analyzer issued about 1500 general level ( GA ) 1 and 2 level warnings . Does this mean that the analyzer generates a lot of false positives? Not.Most warnings are quite relevant, but there’s no point in talking about them in the article.

From time to time, we receive positive feedback from our users, in which they say: “The PVS-Studio analyzer produces few false positives, which is very convenient.” We also believe that there are few false positives. But how so? The article tells only about 16% of messages. What's the rest of it? Is it a false positive?

Of course, there are false positives. From this you can not get anywhere. To suppress them, there are a number of mechanisms . However, most of the warnings didn’t reveal an error, but pointed out the code that smells bad. I will try to explain with examples.

The analyzer issued 206 warnings. V690 , , . :
 class RegistryTypeReader { public: .... inline RegistryTypeReader(const RegistryTypeReader& toCopy); .... }; inline RegistryTypeReader::RegistryTypeReader(const RegistryTypeReader& toCopy) : m_pApi(toCopy.m_pApi) , m_hImpl(toCopy.m_hImpl) { m_pApi->acquire(m_hImpl); }

. , operator = 206 . ?

.

, , . , V690, 206 .

. :
 if( pInit ) AddMap( pInit ); if( pInit ) AddMap( pInit2 );

V581. , , V581 - . , 70 . . , :
 static bool lcl_parseDate(....) { bool bSuccess = true; .... if (bSuccess) { ++nPos; } if (bSuccess) { bSuccess = readDateTimeComponent(string, nPos, nDay, 2, true); .... }

'bSuccess'. ?

70 . , - , . V581 70 .

, - . :
 static bool lcl_parseDate(....) { bool bSuccess = true; .... if (bSuccess) { ++nPos; bSuccess = readDateTimeComponent(string, nPos, nDay, 2, true); .... }

, . , , . , , , , .

Note. , . . , , . .

Conclusion


, , LibreOffice . Coverity . .

? . . PVS-Studio .



. , . LibreOffice . I apologize. .

This article is in English.


, : Andrey Karpov. LibreOffice Project's Check .

Read the article and have a question?
Often our articles are asked the same questions. We collected the answers to them here: Answers to questions from readers of articles about PVS-Studio and CppCat, version 2014 . Please review the list.

Source: https://habr.com/ru/post/251817/

More articles:


All Articles