Geekly Articles each Day
📜 ⬆️ ⬇️

How using PVS-Studio can improve Visual C ++ 2017 Libraries

The title of the article hints to Visual Studio developers that they can benefit from using the PVS-Studio static code analyzer. The article presents the results of the analysis of the libraries that are part of the recently released version of Visual C ++ 2017, and gives recommendations for improving and eliminating errors. I invite readers to find out how the developers of Visual C ++ Libraries shoot their feet: it will be interesting and informative.

A bit of background


This is not my first experiment in checking libraries included in Visual C ++. You can read about previous checks here:


After these publications there was a long break, and I did not write an article regarding VS2015. There were many interesting projects that were waiting for verification. Although, to be honest, I just forgot to write an article on this topic. Fortunately, a tweet from one of the Visual C ++ developers ( @MalwareMinigun ) reminded me about VS2017:

You must be able to find out the standard library headers.
')

Picture 1


Indeed, I did not tell the world about the bugs in the libraries of Visual Studio 2017! Well, the challenge is accepted!

As you can see, a month has passed since the tweet (March 31). I admit that I delayed the answer, but now I will correct it.

What and how was checked


For verification, I used the latest version of the PVS-Studio analyzer (6.15) available to me.

I checked the C ++ libraries that are part of the recently released version of Visual Studio 2017. I checked the version of the libraries that I had on my computer 04/12/2017. However, the version is not so important, since this text is not a bug report, but an article popularizing the static analysis methodology in general, and the PVS-Studio analyzer in particular.

I admit, I did not bother to complete the library check, as it is difficult for me.

First, I had to copy all the libraries to another folder, otherwise I would not be able to get warnings for them. The analyzer does not display messages to the system libraries. By copying files to another folder, I deceive PVS-Studio, and I get the warnings I need.

By the way, here's the answer to the tweet above. For this reason, Visual C ++ users do not write about the PVS-Studio warnings regarding libraries. It makes no sense to give them out, as they will only distract people. At the same time, this speeds up the analysis a bit, since it is not necessary to fully parse and analyze the bodies of inline functions.

Secondly, I did not try to honestly collect projects. I just created a new solution and added files from libraries there. As a result, some of the files I could not verify. But, from the point of view of writing the article, this is not essential. Material for a full text and so it turned out quite enough. A more thorough and correct check should be done by Visual C ++ developers themselves, and I am ready to help them with this.

False alarms


This time, unfortunately, I cannot quote specific numbers and percentages.

I can say that:


However, these numbers can not be interpreted in any way, and no conclusions can be made on their basis!

Let me remind you that I checked only part of the files, but also in a strange way. Plus there is an unusual moment with libraries. The analyzer generates many absolutely correct messages that are absolutely false. Now I will explain the reason for this paradox.

It is harmful and dangerous to independently declare system data types. An example of a bad idea:

typedef unsigned long DWORD;

The PVS-Studio analyzer will display a warning: V677 Custom declaration of a standard 'DWORD' type. The system header file should be used: #include <WinDef.h>.

The analyzer is absolutely right. You should not declare the type "manually", but connect the corresponding header file.

As you understand, such diagnostics are not applicable to Visual C ++ libraries. After all there, just, such types also appear. And the analyzer issued more than 250 such messages.

Or here is another interesting example. The PVS-Studio analyzer is absolutely right when it criticizes the code in which the this pointer is checked for NULL equality. According to the current C ++ standard, the this pointer cannot be NULL .

Visual C ++ has big problems with this. Apparently, in this regard, it will never meet the standard, well, or it will happen very soon. The fact is that libraries (for example, MFC) are architecturally constructed so that this equal to NULL is a typical situation.

The library code contains a large number of functions that check the this pointer. Here are a couple of such features:

 _AFXWIN_INLINE CDC::operator HDC() const { return this == NULL ? NULL : m_hDC; } _AFXWIN_INLINE HDC CDC::GetSafeHdc() const { return this == NULL ? NULL : m_hDC; }

The PVS-Studio analyzer, of course, will issue warnings for them:


There are more than 40 such messages, while there is no sense, of course. In the coming years, they can be considered false alarms.

As we can see from the example of messages V677 and V704 , not all diagnostics are applicable to Visual C ++ libraries. Of course there is no problem: these and other warnings can be simply turned off and, thus, immediately reduce the number of messages by 300 pieces.

All this I am writing to once again demonstrate that it is very often pointless to discuss the percentage of false positives if you do not pre-configure the analyzer.

In general, this time without interest, I apologize. My subjective opinion is that there are few false positives.

What the analyzer found interesting


I decided to go from harmless to scary. In the beginning I will consider recommendations for minor improvements. Then I move on to horror mistakes, and finish the story with what, in my opinion, can be called “horror”. In general, I will gradually increase the degree. So, go ahead, save the world of programs from bugs!

Picture 5



Micro-optimizations


The analyzer offers to perform a number of micro-optimizations. That is, everything that will be considered in this chapter is not an error, but the code can be slightly improved.

Let's start with the warning V808 , which says that a certain object is created, but not used. Consider this situation on the example of two functions.

 void CMFCToolBarComboBoxButton::AdjustRect() { .... if (m_pWndEdit != NULL) { CRect rectEdit = m_rect; const int iBorderOffset = 3; m_pWndEdit->SetWindowPos( NULL, m_rect.left + nHorzMargin + iBorderOffset, m_rect.top + iBorderOffset, m_rect.Width() - 2 * nHorzMargin - m_rectButton.Width() - iBorderOffset - 3, m_rectCombo.Height() - 2 * iBorderOffset, SWP_NOZORDER | SWP_NOACTIVATE); } .... }

PVS-Studio warning : V808 'rectEdit' object of 'CRect' type created. afxtoolbarcomboboxbutton.cpp 607

The rectEdit object is not used anywhere after creation and initialization. It is just superfluous and it can be safely removed. The code will be slightly shorter.

Another case:

 BOOL CALLBACK AFX_EXPORT CMFCToolBarFontComboBox::EnumFamPrinterCallBackEx(....) { .... CString strName = pelf->elfLogFont.lfFaceName; pCombo->AddFont((ENUMLOGFONT*)pelf, FontType, CString(pelf->elfScript)); return 1; }

V808 'strName' object of 'CStringT' type created but not used. afxtoolbarfontcombobox.cpp 138

An object of type CString is created and initialized, but not used anywhere. I don’t know if the compiler will guess to throw out extra code to create and copy a string. Perhaps, he will not guess, since CStirng is a complex class. However, it does not matter, in any case, the strName object should be removed and, thus, make the code easier.

And there are a lot of such superfluous objects. In addition to those already reviewed, the analyzer issued another 50 messages. In order not to clutter the text of the article, I wrote these messages in a separate file: vs2017_V808.txt .

Now it's time for extra checks.

 TaskStack::~TaskStack() { if (m_pStack) delete [] m_pStack; }

PVS-Studio warning : V809 Verifying that a pointer value is not NULL is not required. The 'if (m_pStack)' check can be removed. taskcollection.cpp 29

The delete operator is completely safe to give nullptr as input. Thus, verification is superfluous and the code can be simplified:

 TaskStack::~TaskStack() { delete [] m_pStack; }

There are too many such extra checks. I wrote 68 messages to the file vs2017_V809.txt .

The next trifle that can be changed is to replace the postfix increment of iterators with the prefix one. Example:

 size_type count(const key_type& _Keyval) const { size_type _Count = 0; const_iterator _It = _Find(_Keyval); for (;_It != end() && !this->_M_comparator(....); _It++) { _Count++; } return _Count; }

PVS-Studio warning: V803 Decreased performance. In case of an '_It' is an iterator, it is more useful to use the prefix form of increment. Replace iterator ++ with ++ iterator. internal_concurrent_hash.h 509

It will be a little better if you write:

 for (;_It != end() && !this->_M_comparator(....); ++_It)

The question of whether this refactoring is useful, I considered in the article: " Is there a practical sense to use the prefix increment operator ++ it for iterators instead of postfix it ++ ". The answer is, though not great.

If the authors of the libraries decide that the fix is ​​worth making, then here are 26 more similar warnings: vs2017_V803.txt .

Next micro optimization. It is better to clear the string by calling the function str.Empty () , rather than assigning it the value _T ("") . The class does not know in advance how much memory should be allocated for the line. Therefore, it begins to waste the length of the string. In general, extra actions.

 CString m_strRegSection; CFullScreenImpl::CFullScreenImpl(CFrameImpl* pFrameImpl) { m_pImpl = pFrameImpl; m_pwndFullScreenBar = NULL; m_bFullScreen = FALSE; m_bShowMenu = TRUE; m_bTabsArea = TRUE; m_uiFullScreenID = (UINT)-1; m_strRegSection = _T(""); }

PVS-Studio warning: V815 Decreased performance. Consider replacing the expression 'm_strRegSection = L ""' with 'm_strRegSection.Empty ()'. afxfullscreenimpl.cpp 52

Here it is better to replace the line

 m_strRegSection = _T("");

on

 m_strRegSection.Empty();

A trifle, but a perfectionist would be happy for such an improvement in the code.

Note. In fact, this line can be removed altogether, since this code is in the constructor, and the string is empty.

Here are 27 more warnings on this topic: vs2017_V815.txt .

And finally, here is the code:

 HRESULT GetPropertyInfo(....) { .... for(ul=0; ul<m_cPropSetDex; ul++) { .... for(ULONG ulProp=0; ....) { .... pDescBuffer += (wcslen(L"UNKNOWN") + 1); .... }

PVS-Studio warning : V814 Decreased performance. Wreck of the body of a loop. atldb.h 2374

Note that the wcslen function will be called multiple times, as it is inside nested loops. It would be more logical to calculate in advance and remember the length of the string L "UNKNOWN" .

Another post: V814 Decreased performance. Wreck of the body of a loop. atldb.h 2438

Everything finished with recommendations. Let's move on to something more interesting.

Errors of small and medium caliber


Compiler warnings are suppressed incorrectly in header files. Consider the wrong way to temporarily disable alerts:

 #ifdef _MSC_VER #pragma warning(disable:4200) #endif typedef struct adpcmwaveformat_tag { WAVEFORMATEX wfx; WORD wSamplesPerBlock; WORD wNumCoef; #if defined( _MSC_VER ) ADPCMCOEFSET aCoef[]; #else ADPCMCOEFSET aCoef[1]; #endif } ADPCMWAVEFORMAT; typedef ADPCMWAVEFORMAT *PADPCMWAVEFORMAT; typedef ADPCMWAVEFORMAT NEAR *NPADPCMWAVEFORMAT; typedef ADPCMWAVEFORMAT FAR *LPADPCMWAVEFORMAT; #ifdef _MSC_VER #pragma warning(default:4200) #endif

PVS-Studio warning : V665 Possibly, the usage of #pragma warning (default: X) 'is incorrect in this context. The '#pragma warning (push / pop)' should be used instead. Check lines: 2610, 2628. mmreg.h 2628

I understand that it’s not easy to see what the error itself is, so I’ll highlight the point.

 #pragma warning(disable:4200) .... #pragma warning(default:4200)

First, the compiler warning is disabled. Next, alert status 4200 is set to its default value. So you can not do. Suppose the user has disabled the 4200 diagnostics for one of his files altogether. To his misfortune, he wrote in the file:

 #include <mmreg.h>

As a result, the warning will turn on again and will be issued for the user code.

The correct solution is to save the state, and then return the previous one:

 #pragma warning(push) #pragma warning(disable:4200) .... #pragma warning(pop)

This is where pragma warning is used incorrectly in header files:


There are such errors in the * .cpp files, but I did not write them out, as they do not pose any danger to the code of Visual C ++ users. Although, of course, they too will be useful to fix.

Now let's talk about the operator new .

 inline HRESULT CreatePhraseFromWordArray(....) { .... SPPHRASEELEMENT *pPhraseElement = new SPPHRASEELEMENT[cWords]; if(pPhraseElement == NULL) { ::CoTaskMemFree(pStringPtrArray); return E_OUTOFMEMORY; } memset(pPhraseElement, 0, sizeof(SPPHRASEELEMENT) * cWords); .... }

PVS-Studio warning : V668 pphraseElement The exception will be generated in the case of memory allocation error. sphelper.h 2973

Formally, this code is incorrect. In case of a memory allocation error, the operator new should throw an exception. As a result, we will not get inside the body of the if operator and the CoTaskMemFree function will not be called. And, in general, the program will not work as expected by the programmer.

I'm not sure this is a real mistake. There is a possibility that the project is linked with nothrownew.obj . In this case, the new operator will not generate an exception. This feature, for example, is used by driver developers. Read more: new and delete operators . So, if these are false positives, then you can simply turn off the V668 warning.

But another scenario is possible. This code has remained since ancient times, when the new operator returned a null value in case of an error. Then everything is bad, since the analyzer issued 112 such warnings: vs2017_V668.txt .

We continue. The analyzer generates many warnings V730 , which says that not all members are initialized in the constructor. Let me explain the essence of the warning on two examples.

In the beginning, we consider the CMFCScanliner class. It announces the following members:

 class CMFCScanliner { .... private: LPBYTE m_line; LPBYTE m_line_begin; LPBYTE m_line_end; size_t m_pitch; DWORD m_start_row; DWORD m_start_col; DWORD m_rows; DWORD m_cols; long m_offset; BYTE m_channels; size_t m_height; };

Now look at the constructor:

 CMFCScanliner() { empty(); }

Actually, there is nothing to look at in the constructor. We should proceed to the study of the empty function:

 void empty() { m_line = NULL; m_pitch = 0; m_start_row = 0; m_start_col = 0; m_rows = 0; m_cols = 0; m_offset = 0; m_height = 0; m_line_begin = NULL; m_line_end = NULL; }

PVS-Studio Warning: V730 It is possible that all of the elements are initialized inside the constructor. Consider inspecting: m_channels. afxtoolbarimages.cpp 510

Initialized all members except m_channels . Agree, this is strange, since this member does not stand out among the rest. This is very similar to an error, although I cannot be sure to the end, since I am not familiar with the principles of the class in question.

Now consider the AFX_EVENT structure.

 struct AFX_EVENT { enum { event, propRequest, propChanged, propDSCNotify }; AFX_EVENT(int eventKind); AFX_EVENT(int eventKind, DISPID dispid, ....); int m_eventKind; DISPID m_dispid; DISPPARAMS* m_pDispParams; EXCEPINFO* m_pExcepInfo; UINT* m_puArgError; BOOL m_bPropChanged; HRESULT m_hResult; DSCSTATE m_nDSCState; DSCREASON m_nDSCReason; }; AFX_INLINE AFX_EVENT::AFX_EVENT(int eventKind) { m_eventKind = eventKind; m_dispid = DISPID_UNKNOWN; m_pDispParams = NULL; m_pExcepInfo = NULL; m_puArgError = NULL; m_hResult = NOERROR; m_nDSCState = dscNoState; m_nDSCReason = dscNoReason; }

PVS-Studio warning : V730 class are initialized inside the constructor. Consider inspecting: m_bPropChanged. afxpriv2.h 104

This time, the m_bPropChanged variable remained uninitialized.

In both cases, I do not presume to judge whether it is necessary to initialize these variables. Therefore, I suggest the developers to study these and other suspicious cases that the PVS-Studio analyzer pays attention to. I wrote another 183 analyzer warnings to the vs2017_V730.txt file. Surely among these messages there are several that indicate these serious errors. If I were sure that the members should be precisely initialized, I would refer all these warnings to the next chapter of this article. Uninitialized variables are extremely insidious, since errors can occur rarely and irregularly.

The following analyzer warnings concern meaningless checks. Such checks should either be removed or replaced by some others.

 HRESULT SetDpiCompensatedEffectInput(....) { .... hr = deviceContext->CreateEffect(CLSID_D2D1DpiCompensation, &dpiCompensationEffect); if (SUCCEEDED(hr)) { if (SUCCEEDED(hr)) { .... }

PVS-Studio warning : V571 Recurring check. The 'if (((HRESULT) (hr))> = 0)' condition was already verified in line 881. d2d1_1helper.h 883

The value of the variable hr is checked twice in a row. We deal either with a superfluous code, or this is some kind of typo and we need to change the second condition.

 void Append(_In_reads_(nLength) PCXSTR pszSrc, _In_ int nLength) { // See comment in SetString() about why we do this UINT_PTR nOffset = pszSrc-GetString(); UINT nOldLength = GetLength(); if (nOldLength < 0) { // protects from underflow nOldLength = 0; } .... }

PVS-Studio warning : V547 Expression 'nOldLength <0' is always false. Unsigned type value is never <0. atlsimpstr.h 392

The variable nOldLength is of unsigned type UINT , which means it cannot be less than zero.

Now let's talk about the FreeLibrary function.

 extern "C" BOOL WINAPI DllMain(HINSTANCE hInstance, DWORD dwReason, LPVOID) { .... ::FreeLibrary(pState->m_appLangDLL); .... }

PVS-Studio warning : V718 The 'FreeLibrary' function should not be called from the 'DllMain' function. dllinit.cpp 639

Here is what MSDN says about the FreeLibrary function: It is not safe to call FreeLibrary from DllMain . For more information, see the Remarks section in DllMain .

Thanks to luck, the code can work. However, the code is still bad and you should pay attention to it.

In conclusion of this chapter I want to draw attention to this template function here:

 template<class _FwdIt> string_type transform_primary(_FwdIt _First, _FwdIt _Last) const { // apply locale-specific case-insensitive transformation string_type _Res; if (_First != _Last) { // non-empty string, transform it vector<_Elem> _Temp(_First, _Last); _Getctype()->tolower(&*_Temp.begin(), &*_Temp.begin() + _Temp.size()); _Res = _Getcoll()->transform(&*_Temp.begin(), &*_Temp.begin() + _Temp.size()); } return (_Res); }

PVS-Studio warning : V530 The return value of the function 'tolower' is required to be utilized. regex 319

I see this code for the first time and it's hard for me to navigate Therefore, I don’t know if the analyzer is right, indicating a call to the tolower function. Usually, the result of the tolower function must be used, but I do not know which particular tolower function is called here. Therefore, I simply pay attention of the library developers to this code and ask to check it.

Hardcore


Here begins, in my opinion, the most interesting.

Picture 7
 _AFXCMN_INLINE int CToolBarCtrl::GetString( _In_ int nString, _Out_writes_to_(cchMaxLen, return + 1) LPTSTR lpstrString, _In_ size_t cchMaxLen) const { ASSERT(::IsWindow(m_hWnd)); return (int) ::SendMessage(m_hWnd, ...., (LPARAM)lpstrString); lpstrString[cchMaxLen]=_T('\0'); }

PVS-Studio warning : V779 Unreachable code detected. It is possible that an error is present. afxcmn2.inl 111

Clear error: the last line of the function is never executed.

The following code snippet contains a highly suspicious call to the return statement inside the loop body:

 HRESULT GetIndexOfPropertyInSet( _In_ const GUID* pPropSet, _In_ DBPROPID dwPropertyId, _Out_ ULONG* piCurPropId, _Out_ ULONG* piCurSet) { HRESULT hr = GetIndexofPropSet(pPropSet, piCurSet); if (hr == S_FALSE) return hr; UPROPINFO* pUPropInfo = m_pUPropSet[*piCurSet].pUPropInfo; for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++) { if( dwPropertyId == pUPropInfo[ul].dwPropId ) *piCurPropId = ul; return S_OK; } return S_FALSE; }

PVS-Studio warning : V612 An unconditional 'return' within a loop. atldb.h 4837

It is not clear why there was a cycle to be done, if, all the same, this cycle will not be able to perform more than one iteration. The code can be simplified. However, it seems to me that here it is necessary not to simplify the code, but to correct the error. I have a suspicion that the curly brackets are forgotten and, in fact, the function should look like this:

 HRESULT GetIndexOfPropertyInSet( _In_ const GUID* pPropSet, _In_ DBPROPID dwPropertyId, _Out_ ULONG* piCurPropId, _Out_ ULONG* piCurSet) { HRESULT hr = GetIndexofPropSet(pPropSet, piCurSet); if (hr == S_FALSE) return hr; UPROPINFO* pUPropInfo = m_pUPropSet[*piCurSet].pUPropInfo; for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++) { if( dwPropertyId == pUPropInfo[ul].dwPropId ) { *piCurPropId = ul; return S_OK; } } return S_FALSE; }

In addition to the considered cycle, you should pay attention to a couple of break statements , which always interrupt the cycles:


Now let's talk about Copy-Paste. You can not write a big project and not make mistakes associated with copying and editing blocks of text.

By the way, I suggest at the beginning to try to find the error yourself, without reading the article further.

 void CPaneContainerManager::RemoveAllPanesAndPaneDividers() { ASSERT_VALID(this); POSITION pos = NULL; for (pos = m_lstControlBars.GetHeadPosition(); pos != NULL;) { POSITION posSave = pos; CBasePane* pWnd = DYNAMIC_DOWNCAST( CBasePane, m_lstControlBars.GetNext(pos)); ASSERT_VALID(pWnd); if (pWnd->IsPaneVisible()) { m_lstControlBars.RemoveAt(posSave); } } for (pos = m_lstSliders.GetHeadPosition(); pos != NULL;) { POSITION posSave = pos; CBasePane* pWnd = DYNAMIC_DOWNCAST( CBasePane, m_lstControlBars.GetNext(pos)); ASSERT_VALID(pWnd); if (pWnd->IsPaneVisible()) { m_lstSliders.RemoveAt(posSave); } } }

Picture 3


See what's wrong?

I bet that many have surrendered and decided to read the article further. This is a good example of why static analyzers are important and necessary: ​​they are not lazy and not tired.

PVS-Studio warning : V778 Two code fragments were found. Perhaps this is a typo and 'm_lstSliders' variable should not be used instead of 'm_lstControlBars'. afxpanecontainermanager.cpp 1645

However, even after reading the analyzer warning, it’s not easy to find an error in the code. Therefore, I will cut it, leaving important for us:

 for (... m_lstControlBars ...) { CBasePane* pWnd = ... m_lstControlBars ... m_lstControlBars.RemoveAt(); } for (... m_lstSliders ...) { CBasePane* pWnd = ... m_lstControlBars ... m_lstSliders.RemoveAt(); }

In the first cycle, the m_lstControlBars container is processed , and in the second, the m_lstSliders container.

With a probability of 99%, the second cycle was written using Copy-Paste technology: the first cycle was taken, copied, and then it replaced the name m_lstControlBars with m_lstSliders . But in one place you forgot to replace the name!

Error here: CBasePane * pWnd = ... m_lstControlBars ...

It was a good mistake, but the next one is not inferior to her in beauty. Consider how the increment / decrement operators are implemented in the CMFCScanliner class:

 class CMFCScanliner { .... inline const CMFCScanliner& operator ++ () { m_line += m_offset; return *this; } inline const CMFCScanliner& operator ++ (int) { m_line += m_offset; return *this; } inline const CMFCScanliner& operator -- () { m_line -= m_offset; return *this; } inline const CMFCScanliner& operator -- (int) { m_line += m_offset; return *this; } .... };

PVS-Studio warning : V524 It is odd that the body of the '-' function is fully equivalent to the body of the '++' function. afxtoolbarimages.cpp 656

Pay attention to the implementation of the latest operator. Forgot to change + = to - = . This is a classic! This is the “ last line effect ” in action!

There are three places where leaks can occur. Here is one of them:

 CSpinButtonCtrl* CMFCPropertyGridProperty::CreateSpinControl( CRect rectSpin) { ASSERT_VALID(this); ASSERT_VALID(m_pWndList); CSpinButtonCtrl* pWndSpin = new CMFCSpinButtonCtrl; if (!pWndSpin->Create(WS_CHILD | WS_VISIBLE | UDS_ARROWKEYS | UDS_SETBUDDYINT | UDS_NOTHOUSANDS, rectSpin, m_pWndList, AFX_PROPLIST_ID_INPLACE)) { return NULL; } .... }

PVS-Studio warning : V773 The pWndSpin pointer. A memory leak is possible. afxpropertygridctrl.cpp 1490

If the Create function is executed with an error, the object to which the pointer is stored in the pWndSpin variable will not be deleted.

Similarly:


According to the C ++ standard, calling the delete operator on a void * pointer results in undefined behavior. And, as the reader has already guessed, this happens in the Visual C ++ libraries:

 typedef void *PVOID; typedef PVOID PSECURITY_DESCRIPTOR; class CSecurityDescriptor { .... PSECURITY_DESCRIPTOR m_pSD; .... }; inline CSecurityDescriptor::~CSecurityDescriptor() { delete m_pSD; // <=  m_pSD   void * free(m_pOwner); free(m_pGroup); free(m_pDACL); free(m_pSACL); }

PVS-Studio warning : V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1039

Similar problems can be found here:


CMFCReBar::CalcFixedLayout bStretch , . , , bStretch 1. , , .

 CSize CMFCReBar::CalcFixedLayout(BOOL bStretch, BOOL bHorz) { ASSERT_VALID(this); ENSURE(::IsWindow(m_hWnd)); // the union of the band rectangles is the total bounding rect int nCount = (int) DefWindowProc(RB_GETBANDCOUNT, 0, 0); REBARBANDINFO rbBand; rbBand.cbSize = m_nReBarBandInfoSize; int nTemp; // sync up hidden state of the bands for (nTemp = nCount; nTemp--; ) { rbBand.fMask = RBBIM_CHILD|RBBIM_STYLE; VERIFY(DefWindowProc(RB_GETBANDINFO, nTemp, (LPARAM)&rbBand)); CPane* pBar = DYNAMIC_DOWNCAST( CPane, CWnd::FromHandlePermanent(rbBand.hwndChild)); BOOL bWindowVisible; if (pBar != NULL) bWindowVisible = pBar->IsVisible(); else bWindowVisible = (::GetWindowLong( rbBand.hwndChild, GWL_STYLE) & WS_VISIBLE) != 0; BOOL bBandVisible = (rbBand.fStyle & RBBS_HIDDEN) == 0; if (bWindowVisible != bBandVisible) VERIFY(DefWindowProc(RB_SHOWBAND, nTemp, bWindowVisible)); } // determine bounding rect of all visible bands CRect rectBound; rectBound.SetRectEmpty(); for (nTemp = nCount; nTemp--; ) { rbBand.fMask = RBBIM_STYLE; VERIFY(DefWindowProc(RB_GETBANDINFO, nTemp, (LPARAM)&rbBand)); if ((rbBand.fStyle & RBBS_HIDDEN) == 0) { CRect rect; VERIFY(DefWindowProc(RB_GETRECT, nTemp, (LPARAM)&rect)); rectBound |= rect; } } // add borders as part of bounding rect if (!rectBound.IsRectEmpty()) { CRect rect; rect.SetRectEmpty(); CalcInsideRect(rect, bHorz); rectBound.right -= rect.Width(); rectBound.bottom -= rect.Height(); } bStretch = 1; return CSize((bHorz && bStretch) ? 32767 : rectBound.Width(), (!bHorz && bStretch) ? 32767 : rectBound.Height()); }

PVS-Studio: V763 Parameter 'bStretch' is always rewritten in function body before being used. afxrebar.cpp 209

«bStretch = 1;» , - , . , . .

, AdjustDockingLayout CBasePane CDockSite .

 class CBasePane : public CWnd { .... virtual void AdjustDockingLayout(HDWP hdwp = NULL); .... }; class CDockSite : public CBasePane { .... virtual void AdjustDockingLayout(); .... };

PVS-Studio: V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'AdjustDockingLayout' in derived class 'CDockSite' and base class 'CBasePane'. afxdocksite.h 94

, - hdwp , - . , .

:


, , , . CAccessToken :

 class CAccessToken { .... mutable CRevert *m_pRevert; }; inline bool CAccessToken::ImpersonateLoggedOnUser() const throw(...) { .... delete m_pRevert; m_pRevert = _ATL_NEW CRevertToSelf; .... }

The analyzer issues a warning: V599 The virtual destructor is not present, although the 'CRevert' class contains virtual functions. atlsecurity.h 5252

Let's see why he does it. We are interested in the member m_pRevert , which is a pointer to an object of type CRevert . The class is used polymorphically. This conclusion can be reached by looking at this line of code:

 m_pRevert = _ATL_NEW CRevertToSelf;

The class CRevertToSelf is inherited from the class CRevert . Let's now get acquainted with these classes:

 class CRevert { public: virtual bool Revert() throw() = 0; }; class CRevertToSelf : public CRevert { public: bool Revert() throw() { return 0 != ::RevertToSelf(); } };

CRevert ? .

PVS-Studio , . , V611 , . , .

 template <class TAccessor> class CBulkRowset : public CRowset<TAccessor> { .... void SetRows(_In_ DBROWCOUNT nRows) throw() { if (nRows == 0) nRows = 10; if (nRows != m_nRows) { delete m_phRow; m_phRow = NULL; m_nRows = nRows; } } HRESULT BindFinished() throw() { m_nCurrentRows = 0; m_nCurrentRow = 0; m_hr = S_OK; if (m_phRow == NULL) { m_phRow = _ATL_NEW HROW[m_nRows]; if (m_phRow == NULL) return E_OUTOFMEMORY; } return S_OK; } .... HROW* m_phRow; .... }

PVS-Studio: V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] m_phRow;'. atldbcli.h 5689

BindFinished new [] :

 m_phRow = _ATL_NEW HROW[m_nRows];

Memory is freed in the SetRows function using the delete operator :

 delete m_phRow;

Result: undefined behavior.

Now let's talk about one very suspicious function call memset . However, before looking at the incorrect code, take a look at the code where everything is fine.

The correct code is:

 void CToolTipCtrl::FillInToolInfo(TOOLINFO& ti, ....) const { memset(&ti, 0, sizeof(AFX_OLDTOOLINFO)); ti.cbSize = sizeof(AFX_OLDTOOLINFO); .... }

Before us is a typical situation. All members of the structure are cleared (filled with zeros) by calling the memset function . Then the structure is written its size. This is standard practice for WinAPI. So many functions will find out with which version (format) of the structure they are working.

In the above code, everything is logical. The size of the AFX_OLDTOOLINFO structure is calculated . Further this structure size is used to call the memset function and the same size is written inside the structure.

Now consider the anomalous code:

 BOOL CControlBar::PreTranslateMessage(MSG* pMsg) { .... TOOLINFO ti; memset(&ti, 0, sizeof(AFX_OLDTOOLINFO)); ti.cbSize = sizeof(TOOLINFO); .... }

PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer '& ti'. barcore.cpp 384

TOOLINFO . TOOLINFO : ti.cbSize = sizeof(TOOLINFO); .

, , , sizeof(AFX_OLDTOOLINFO) .

, .

, memset .

 GUID m_Id; void zInternalStart() { .... // Zero the activity id in case we end up logging the stop. ZeroMemory(&m_Id, sizeof(&m_Id)); .... }

PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer '& m_Id'. traceloggingactivity.h 656

, . , 4 8 , , 32- 64- . GUID 16 (128 ).

:

 ZeroMemory(&m_Id, sizeof(m_Id));

Not without warnings V595 . This is not surprising, since this diagnostics reveals one of the most common errors in C and C ++ programs. However, in C #, everything is also not perfect .

The essence of the error lies in the fact that the pointer dereference occurs before it is checked.

Consider the code snippet identified using this diagnostic.

 HRESULT CBasePane::get_accHelp(VARIANT varChild, BSTR *pszHelp) { if ((varChild.vt == VT_I4) && (varChild.lVal == CHILDID_SELF)) { *pszHelp = SysAllocString(L"ControlPane"); return S_OK; } if (((varChild.vt != VT_I4) && (varChild.lVal != CHILDID_SELF)) || (NULL == pszHelp)) { return E_INVALIDARG; } .... }

PVS-Studio warning : V595 The 'pszHelp' pointer was used against nullptr. Check lines: 1324, 1328. afxbasepane.cpp 1324

If the function is called like this:

 VARIANT foo; foo.vt = VT_I4; foo.lVal = CHILDID_SELF; get_accHelp(foo, NULL);

E_INVALIDARG , .

. . , NULL . , . , . , !

, . Visual C++ . 17 , : vs2017_V595.txt .

, FALSE S_FALSE.

 BOOL CMFCRibbonPanel::OnSetAccData (long lVal) { .... if (nIndex < 0 || nIndex >= arElements.GetSize()) { return FALSE; } if (GetParentWnd()->GetSafeHwnd() == NULL) { return S_FALSE; } ASSERT_VALID(arElements[nIndex]); return arElements[nIndex]->SetACCData(GetParentWnd(), m_AccData); }

PVS-Studio warning : V716 Suspicious type conversion in HR statement, but the function actually returns BOOL. afxribbonpanel.cpp 4107.

The function returns the type BOOL . If it was not possible to get the HWND from the parent window, the programmer wanted to return FALSE values ​​from the function . But it was sealed and wrote S_FALSE that radically changes everything.

Here is how the constant S_FALSE is declared:

 #define S_FALSE ((HRESULT)1L)

I think you already understand what is happening, but I will explain just in case.

Write “return S_FALSE;” is the same as writing “return TRUE;”. Epic fail.

Error is not alone, she has friends:


Note


As mentioned at the beginning of the article, I have not checked all the files. In addition, I could miss something among the warnings issued by the analyzer. Therefore, please do not consider the developers of this document as a guide to correct some errors. It is much better if the developers themselves fully check the libraries and carefully examine the warnings of the analyzer.

Conclusion


Picture 4



, .

. , . - , , , . . , , , .

PVS-Studio .

PVS-Studio: https://www.viva64.com/ru/pvs-studio/

Supported languages ​​and compilers:


Thank you for your attention and follow @Code_Analysis on twitter .



If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. How to Improve Visual C ++ 2017 Libraries Using PVS-Studio

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

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

More articles:


All Articles