The title of this article is a hint for the Visual Studio developers that they could benefit from the use of PVS-Studio static code analyzer. The article discusses the analysis results of the libraries in the recent Visual C++ 2017 release and gives advice on how to improve them and eliminate the bugs found. Read on to find out how the developers of Visual C++ Libraries shoot themselves in the foot: it's going to be interesting and informative.
A bit of history
This is not my first experiment on checking Visual C++ libraries. To see the results of the previous checks, see the following articles:
- Errors detected in the Visual C++ 2012 libraries
- A Slipshod Check of the Visual C++ 2013 Library (update 3)
There was a long pause after these checks, and I never wrote an article about checking VS2015, as there were lots of interesting projects waiting to be checked. Well, frankly, I just forgot to write that article. Luckily, I remembered about VS2017 thanks to the tweet by one of the Visual C++ developers (@MalwareMinigun):
I'm surprised we don't have people yelling at us all the time for stuff you folks find in standard library headers.
Indeed, I didn't tell the world about the bugs in Visual Studio 2017 libraries! Well then, challenge accepted!
As you can see, it's been a month since the tweet (March 31-st), so I admit I've been dragging my feet over the reply, but now I'm going to mend that.
What was checked and how it was checked
I did this check using the latest currently available version of PVS-Studio analyzer (6.15).
What I was checking were the C++ libraries that come with the recently released Visual Studio 2017 version. The version of the libraries that I had on my computer at the time was dated 12.04.2017. However, the version number doesn't matter that much because it's an article popularizing static analysis in general and PVS-Studio analyzer in particular, rather than a bug report.
I confess that I didn't bother to perform proper, full-fledged analysis, as it's a complicated task for me.
Firstly, I had to make copies of all the libraries and put them in another folder; otherwise, the analyzer wouldn't have been able to produce diagnostic messages for them because it doesn't do that for system libraries. By copying files into another folder I trick PVS-Studio into generating the warnings I need.
By the way, this also explains the absence of feedback from Visual C++ users on warnings in the libraries as mentioned in the tweet quoted above. There's no point in generating such warnings by default, as they would only distract people. Besides, it helps us speed up the analysis process a little, as the analyzer doesn't have to perform full-fledged parsing and analysis of inline functions' bodies.
Secondly, I didn't try to build the projects in an honest way. I just started a new solution and added the files from the libraries to it. Because of that, PVS-Studio failed to check some of the files, but it doesn't matter much from the viewpoint of my goal, which was to write the article. I got enough material anyway. A more thorough and correct check is something that the Visual C++ developers should do themselves, and I'm ready to help them with that.
False positives
I'm afraid I can't give you concrete figures on false positives this time.
I just can tell you that there were:
- 433 general-analysis (GA) warnings of High certainty level.
- 743 general-analysis (GA) warnings of Medium certainty level.
However, you can't use these figures to make any interpretations or draw any conclusions!
Remember, I checked only part of the files, and did it in an unconventional way. Besides, there is one peculiar thing about the libraries. You see, the analyzer issued lots of warnings that were totally correct yet completely false. There is an explanation to this paradox.
It's bad and dangerous to declare system data types manually. For example:
typedef unsigned long DWORD;
PVS-Studio analyzer issues the following warning for this: V677 Custom declaration of a standard 'DWORD' type. The system header file should be used: #include <WinDef.h>.
The analyzer is totally correct in saying so. You should include the corresponding header rather than declare the type manually.
As you understand, this diagnostic doesn't apply to Visual C++ libraries, for they are just the place to contain the declarations of such types. There were more than 250 warnings like that.
Here's another interesting example. PVS-Studio analyzer is right in criticizing code that checks the this pointer for NULL. As specified by the modern C++ standard, this cannot equal NULL.
Yet Visual C++ has big problems with that. It seems it's never going to meet the standard as far as this matter is concerned, or at least not any time soon. The reason is that the architecture of the libraries (for example MFC) is such that this being equal to NULL is an ordinary thing there.
There are many functions in the libraries' code that check the this pointer. Here are two examples:
_AFXWIN_INLINE CDC::operator HDC() const
{ return this == NULL ? NULL : m_hDC; }
_AFXWIN_INLINE HDC CDC::GetSafeHdc() const
{ return this == NULL ? NULL : m_hDC; }
Naturally, these functions trigger the following PVS-Studio analyzer warnings:
- V704 'this == 0' expression should be avoided - this expression is always false on newer compilers, because 'this' pointer can never be NULL. afxwin1.inl 314
- V704 'this == 0' expression should be avoided - this expression is always false on newer compilers, because 'this' pointer can never be NULL. afxwin1.inl 316
There are over 40 warnings like that and, of course, they are all useless. You can treat them as false positives in the next few years.
As you can see from the examples with the messages V677 and V704, not all diagnostics apply to the Visual C++ libraries. It's not a problem, of course - you can simply turn them off and thus reduce the number of warnings by 300 at once.
I'm writing all this to show you just one more time that discussing the false positives rate makes no sense without prior customization of the analyzer.
So, no rate this time, sorry. If you want to know my personal opinion, there aren't many false positives.
Interesting findings
I'm going to move from harmless to horrible. We'll start with recommendations on minor fixes, then move to mild errors, and finally discuss what I think to be the "creepy" ones. In other words, I'll be raising the tension. Here we go, saving the software world from bugs!
Microoptimizations
The analyzer suggested applying a number of microoptimizations. It means every example in this section is code with a potential for small improvements rather than a bug.
We'll start with the V808 warning, which warns us about an object being created but never used. We'll examine this situation in two functions as examples.
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 diagnostic message: V808 'rectEdit' object of 'CRect' type was created but was not utilized. afxtoolbarcomboboxbutton.cpp 607
After the rectEdit object has been created and initialized, it's not used anywhere. It's just an extra object and can be removed without any hesitation. This will make the code a bit shorter.
The other example:
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 was created but was not utilized. afxtoolbarfontcombobox.cpp 138
An object of type CString is created and initialized but not used anywhere. I don't know if the compiler is smart enough to throw away the unnecessary code that performs string creation and initialization, but it might well fail to do so since CStirng is a complex class. It doesn't matter, though; the strName object should be removed anyway to make the code shorter.
There are just tons of unnecessary objects like that. In addition to what we have already discussed, the analyzer issued 50 more messages. In order not to clutter up the text, I've made a separate list: vs2017_V808.txt.
Now it's time for unnecessary checks.
TaskStack::~TaskStack()
{
if (m_pStack)
delete [] m_pStack;
}
PVS-Studio diagnostic message: V809 Verifying that a pointer value is not NULL is not required. The 'if (m_pStack)' check can be removed. taskcollection.cpp 29
You can safely use nullptr as input for the delete operator, so the check is unnecessary and the code can be simplified:
TaskStack::~TaskStack()
{
delete [] m_pStack;
}
Checks like that are also numerous. All 68 messages can be found in the file vs2017_V809.txt.
The next small improvement is about replacing iterators' postfix increments with prefix ones. For 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 diagnostic message: V803 Decreased performance. In case '_It' is iterator it's more effective to use prefix form of increment. Replace iterator++ with ++iterator. internal_concurrent_hash.h 509
The code would become a bit better if you wrote:
for (;_It != end() && !this->_M_comparator(....); ++_It)
The question if there is any use in doing such refactoring was discussed in the article "Is it reasonable to use the prefix increment operator ++it instead of postfix operator it++ for iterators?". In brief, the answer is yes, though not much.
Should the library developers decide these fixes are worth applying, here's the file with the other 26 warnings of this type: vs2017_V803.txt.
One more microoptimization. It is better to clear a string by calling to str.Empty() rather than assigning it the value _T(""). The class doesn't have prior knowledge about how much memory to allocate for a string, so it starts wasting time computing the string length, which is just an unnecessary operation.
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 diagnostic message: V815 Decreased performance. Consider replacing the expression 'm_strRegSection = L""' with 'm_strRegSection.Empty()'. afxfullscreenimpl.cpp 52
In this code, it's better to replace the line
m_strRegSection = _T("");
with
m_strRegSection.Empty();
It's just a small improvement, but it would surely please a perfectionist.
Note. In general, this string can be removed, as this code is in the constructor and the string is empty anyway.
The other 27 warnings of this kind: vs2017_V815.txt.
One more example:
HRESULT GetPropertyInfo(....)
{
....
for(ul=0; ul<m_cPropSetDex; ul++)
{
....
for(ULONG ulProp=0; ....)
{
....
pDescBuffer += (wcslen(L"UNKNOWN") + 1);
....
}
PVS-Studio diagnostic message: V814 Decreased performance. The 'wcslen' function was called multiple times inside the body of a loop. atldb.h 2374
Note that the wcslen function will be called multiple times, as it is written inside nested loops. A more logical solution would be to compute the length of the L"UNKNOWN" string in advance and remember it.
The last message in this section: V814 Decreased performance. The 'wcslen' function was called multiple times inside the body of a loop. atldb.h 2438
We've finished with microoptimizations. Let's move on to more interesting things.
Small and medium bugs
Compiler warnings are turned off in header files in an incorrect way. Here's one example of this error:
#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 diagnostic message: 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 know it's not easy to figure out what the error is actually about, so here are the key lines:
#pragma warning(disable:4200)
....
#pragma warning(default:4200)
Compiler warning 4200 is turned off and then its state is set to default. You can't do it that way. Suppose some user has the 4200 diagnostic turned off completely for one of their files and writes the following line in that file, unaware of the harm it could do:
#include <mmreg.h>
As a result, this line will enable the warning again to be triggered by the user's code.
The correct solution is to save the current state and then return the previous one:
#pragma warning(push)
#pragma warning(disable:4200)
....
#pragma warning(pop)
Here's a list of other cases of incorrect use of pragma warnings in headers:
- 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: 586, 601. workstealingqueue.h 601
- 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: 1669, 1697. usbioctl.h 1697
- 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: 1631, 1646. usbioctl.h 1646
- 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: 1490, 1518. usbioctl.h 1518
- 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: 986, 1002. usbioctl.h 1002
- 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: 960, 978. usbioctl.h 978
- 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: 913, 925. usbioctl.h 925
- 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: 861, 876. usbioctl.h 876
- 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: 860, 875. usbioctl.h 875
Errors of this kind are found in *.cpp files as well, but I didn't write them down as they don't pose any threat to the code of Visual C++ users. It is desirable, however, to fix those tooNevertheless,.
Now let's talk about the new operator.
inline HRESULT CreatePhraseFromWordArray(....)
{
....
SPPHRASEELEMENT *pPhraseElement = new SPPHRASEELEMENT[cWords];
if(pPhraseElement == NULL)
{
::CoTaskMemFree(pStringPtrArray);
return E_OUTOFMEMORY;
}
memset(pPhraseElement, 0, sizeof(SPPHRASEELEMENT) * cWords);
....
}
PVS-Studio diagnostic message: V668 There is no sense in testing the 'pPhraseElement' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. sphelper.h 2973
This code is technically faulty. If a memory allocation error occurs, the new operator must throw an exception, thus preventing the flow of execution from getting into the body of the if statement and calling the CoTaskMemFree function. The program's behavior will no longer follow the author's design.
I'm not sure this is a real error, though. This project might be linked with nothrownew.obj, in which case the new operator will not throw the exception. It is a common practice to use this feature among driver developers, for example. To learn more, see: new and delete operators. So, if these are false warnings, you can simply turn off the V668 warning.
However, another scenario is also possible: the code in question dates back to the ancient times when the new operator used to return the value of NULL in case of an error. If that is the case, then it's really bad, since I got 112 warnings of that kind: vs2017_V668.txt.
Let's move on. The analyzer issued multiple V730 warnings that tell us about some of constructor members left uninitialized. Here are two examples to illustrate this.
We'll examine class CMFCScanliner first. It has the following declared 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's nothing to look at here. We should go to 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 diagnostic message: V730 It is possible that not all members of a class are initialized inside the constructor. Consider inspecting: m_channels. afxtoolbarimages.cpp 510
Every member except m_channels has been initialized. It looks strange, doesn't it? There is nothing special about this member. So, it really looks like an error, though I can't say for sure since I don't know how exactly this class should work.
Now let's examine structure AFX_EVENT.
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 diagnostic message: V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_bPropChanged. afxpriv2.h 104
This time, it is the variable m_bPropChanged that was left uninitialized.
In both cases, I can't say for sure if these variables must be initialized. I leave it to the developers to examine this and other suspicious cases pointed out by PVS-Studio analyzer. The file vs2017_V730.txt contains 183 more warnings of that type. I'm sure some of them will prove to be genuine. Had I been sure that those members must be initialized, I'd have included them all into the next section instead. Uninitialized variables are very tricky because they lead to rare and irregular errors.
The next warnings deal with pointless checks: such checks should either be removed or replaced with appropriate ones.
HRESULT
SetDpiCompensatedEffectInput(....)
{
....
hr = deviceContext->CreateEffect(CLSID_D2D1DpiCompensation,
&dpiCompensationEffect);
if (SUCCEEDED(hr))
{
if (SUCCEEDED(hr))
{
....
}
PVS-Studio diagnostic message: 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. It's either duplicate code or some typo, in which case the second condition needs to be changed.
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 diagnostic message: V547 Expression 'nOldLength < 0' is always false. Unsigned type value is never < 0. atlsimpstr.h 392
The nOldLength variable is of type unsigned UINT and therefore can't be less than zero.
Now let's talk about the function FreeLibrary.
extern "C"
BOOL WINAPI DllMain(HINSTANCE hInstance, DWORD dwReason, LPVOID)
{
....
::FreeLibrary(pState->m_appLangDLL);
....
}
PVS-Studio diagnostic message: V718 The 'FreeLibrary' function should not be called from 'DllMain' function. dllinit.cpp 639
This is what MSDN says about this function: It is not safe to call FreeLibrary from DllMain. For more information, see the Remarks section in DllMain.
It's pure luck this code works as intended, but it's still bad and must be reviewed.
As the last example in this section, I'd like you to look at the following template function:
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 diagnostic message: V530 The return value of function 'tolower' is required to be utilized. regex 319
It's the first time I see this code, and I'm not sure how to treat it. I don't know if the analyzer is right in pointing out the call of the tolower function. The return value of the tolower function typically must be used, but I don't know what version of it exactly is called here. So, I'm just pointing this code out to the developers for them to check it.
Hardcore
This is where, I believe, the most interesting things start.
_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 diagnostic message: V779 Unreachable code detected. It is possible that an error is present. afxcmn2.inl 111
This is an obvious bug: the last line of the function never executes.
The next fragment contains a highly suspicious call of the return statement inside a 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 diagnostic message: V612 An unconditional 'return' within a loop. atldb.h 4837
Why implement a loop if it can't iterate more than once anyway? The code looks like it could be simplified, but I suspect it needs some bug fixing instead. It seems that there are braces missing here and the function should actually look as follows:
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;
}
Besides that loop discussed above, there are a couple of break statements breaking loops all the time:
- V612 An unconditional 'break' within a loop. viewprev.cpp 476
- V612 An unconditional 'break' within a loop. iomanip 489
Now let's talk about Copy-Paste. One can't write a big software project without making a heap of mistakes dealing with text copy-and-pasting.
Try to spot the bug in the example below by yourself, without reading my comment.
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);
}
}
}
Have you found it?
I bet many of you gave up and scrolled on. It's a nice example of why static analyzers are so important and needed: they never get lazy or tired.
PVS-Studio diagnostic message: V778 Two similar code fragments were found. Perhaps, this is a typo and 'm_lstSliders' variable should be used instead of 'm_lstControlBars'. afxpanecontainermanager.cpp 1645
However, finding the bug hasn't become much easier even after reading the analyzer's warning. Here's an abridged version with only the key lines left:
for (... m_lstControlBars ...)
{
CBasePane* pWnd = ... m_lstControlBars ...
m_lstControlBars.RemoveAt();
}
for (... m_lstSliders ...)
{
CBasePane* pWnd = ... m_lstControlBars ...
m_lstSliders.RemoveAt();
}
Container m_lstControlBars is handled in the first loop, and container m_lstSliders, in the second.
There's almost no doubt that the second loop was written using the Copy-Paste technique: the programmer took the first loop, copied it, and then changed all the instances of the name m_lstControlBars to m_lstSliders. All but one!
The mistake is here: CBasePane* pWnd = ... m_lstControlBars ...
It's a nice bug but the next one is just as cool. Let's check how 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 diagnostic message: V524 It is odd that the body of '--' function is fully equivalent to the body of '++' function. afxtoolbarimages.cpp 656
Note how the very last operator is implemented: the programmer forgot to change += to -=. It's a classic! It's the "last line effect" in all its glory!
The analyzer found three spots where leaks might occur. This 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 diagnostic message: V773 The function was exited without releasing the 'pWndSpin' pointer. A memory leak is possible. afxpropertygridctrl.cpp 1490
If an error occurs while executing the Create function, the object the pointer to which is stored in the pWndSpin variable won't be deleted.
The other cases:
- V773 The function was exited without releasing the 'pList' pointer. A memory leak is possible. afxribboncombobox.cpp 461
- V773 The function was exited without releasing the 'pButton' pointer. A memory leak is possible. afxvslistbox.cpp 222
As specified by the C++ standard, calling the delete operator on a pointer of type void* is undefined behavior. As you have already guessed, that's what 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; // <= void *m_pSD;
free(m_pOwner);
free(m_pGroup);
free(m_pDACL);
free(m_pSACL);
}
PVS-Studio diagnostic message: V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1039
Other defects of this kind:
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1048
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1070
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1667
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. afxstate.cpp 265
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. dbcore.cpp 1240
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. dbcore.cpp 1250
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. doccore.cpp 1654
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. dockstat.cpp 343
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. filefind.cpp 43
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. filefind.cpp 49
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. sockcore.cpp 541
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. winfrm.cpp 145
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. winfrm.cpp 465
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. mapiunicodehelp.h 168
The CMFCReBar::CalcFixedLayout function gets the bStretch parameter but doesn't use it. To be more exact, 1 is explicitly written to bStretch before using it for the first time. To prove that I haven't misunderstood or missed anything, here's this function in full.
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 diagnostic message: V763 Parameter 'bStretch' is always rewritten in function body before being used. afxrebar.cpp 209
The line "bStretch = 1;" looks like someone added it for debugging purposes and forgot to delete it when it was no longer needed. Perhaps this is exactly what happened. The authors should check this strange code.
Look at the declaration of the AdjustDockingLayout function in the classes CBasePane and CDockSite.
class CBasePane : public CWnd
{
....
virtual void AdjustDockingLayout(HDWP hdwp = NULL);
....
};
class CDockSite : public CBasePane
{
....
virtual void AdjustDockingLayout();
....
};
PVS-Studio diagnostic message: 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
It looks like the programmer added the argument hdwp to the function declaration in the base class at some point but forgot to do the same in the derived class. As a result, these two are different functions now.
Similar cases:
- V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'CopyState' in derived class 'CPane' and base class 'CBasePane'. afxpane.h 96
- V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'CopyState' in derived class 'CDockablePane' and base class 'CPane'. afxdockablepane.h 184
- V762 It is possible a virtual function was overridden incorrectly. See second argument of function 'SizeToContent' in derived class 'CMFCLinkCtrl' and base class 'CMFCButton'. afxlinkctrl.h 50
- V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'RecalcLayout' in derived class 'CMFCTasksPane' and base class 'CPane'. afxtaskspane.h 287
Since we started talking about functions in classes, let's talk about virtual destructors, or more exactly, missing virtual destructors. We'll start with the CAccessToken class:
class CAccessToken
{
....
mutable CRevert *m_pRevert;
};
inline bool
CAccessToken::ImpersonateLoggedOnUser() const throw(...)
{
....
delete m_pRevert;
m_pRevert = _ATL_NEW CRevertToSelf;
....
}
The analyzer issues the following warning: V599 The virtual destructor is not present, although the 'CRevert' class contains virtual functions. atlsecurity.h 5252
Let's figure out why it does that. We are interested in the m_pRevert member, which is a pointer to an object of type CRevert. The class is used polymorphically judging by the following code line:
m_pRevert = _ATL_NEW CRevertToSelf;
The CRevertToSelf class is derived from CRevert. Now let's look into these classes:
class CRevert
{
public:
virtual bool Revert() throw() = 0;
};
class CRevertToSelf : public CRevert
{
public:
bool Revert() throw()
{
return 0 != ::RevertToSelf();
}
};
What is missing in this CRevert class? A virtual destructor.
We do not only add new diagnostics to PVS-Studio analyzer but also improve existing ones. For example, the V611 diagnostic has recently learned how to detect memory release issues in the cases when memory allocation and freeing are performed in different functions. This is how it works in practice.
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 diagnostic message: 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
Memory is allocated in the function BindFinished using the new [] operator:
m_phRow = _ATL_NEW HROW[m_nRows];
and released in the function SetRows using the delete operator:
delete m_phRow;
The result is undefined behavior.
Now, here's a very suspicious call to the memset function. However, before we examine the faulty code, let's see how a call to that function is used in correct code.
Normal code:
void CToolTipCtrl::FillInToolInfo(TOOLINFO& ti, ....) const
{
memset(&ti, 0, sizeof(AFX_OLDTOOLINFO));
ti.cbSize = sizeof(AFX_OLDTOOLINFO);
....
}
It's a typical situation. All the structure members are cleared (filled with zeroes) by calling the memset function. After that, the size of the structure is written to it. It's a usual practice for WinAPI - this is how functions figure out which version (format) of the structure they are dealing with.
The code above is logical. The size of the structure AFX_OLDTOOLINFO is computed. Then this size is used to call the memset function, and the same size is written to the structure.
Now, here's abnormal code:
BOOL CControlBar::PreTranslateMessage(MSG* pMsg)
{
....
TOOLINFO ti; memset(&ti, 0, sizeof(AFX_OLDTOOLINFO));
ti.cbSize = sizeof(TOOLINFO);
....
}
PVS-Studio diagnostic message: V512 A call of the 'memset' function will lead to underflow of the buffer '& ti'. barcore.cpp 384
The structure is of type TOOLINFO, and it is the size of the TOOLINFO structure that gets written to it: ti.cbSize = sizeof(TOOLINFO);.
However, the structure is cleared only partially since the number of bytes to be cleared is computed according to the sizeof(AFX_OLDTOOLINFO) expression.
As a result, some members of the structure remain uninitialized.
There is one more case of a structure incompletely filled by 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 diagnostic message: V512 A call of the 'memset' function will lead to underflow of the buffer '& m_Id'. traceloggingactivity.h 656
Computing the pointer size instead of the structure size is a classic bug, which leads to clearing only the first 4 or 8 bytes depending on whether the application is compiled as 32-bit or 64-bit, while the GUID structure's size is 16 bytes (128 bits).
Fixed version:
ZeroMemory(&m_Id, sizeof(m_Id));
There were a few V595 warnings too, which is not surprising since this diagnostic detects one of the most widespread bugs in C and C++ programs. However, those written in C# are not perfect either.
This error is about a pointer being dereferenced before the check.
Look at the following code fragment.
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 diagnostic message: V595 The 'pszHelp' pointer was utilized before it was verified against nullptr. Check lines: 1324, 1328. afxbasepane.cpp 1324
If you call the function in the following way:
VARIANT foo;
foo.vt = VT_I4;
foo.lVal = CHILDID_SELF;
get_accHelp(foo, NULL);
it must return the E_INVALIDARG status, but a null pointer deference will occur instead.
This is the way of the analyzer's "thought". "The pointer is dereferenced, but it is checked for NULL later. Since there is such a check, the pointer might be null. If it really is, that's bad. Aha, I should warn about this!"
As I have already said, this error is found in many applications, and the Visual C++ libraries are no exception. Here are 17 more fragments that need refactoring: vs2017_V595.txt.
The last bug I'd like to discuss deals with mixing up the constants FALSE and 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 diagnostic message: V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. afxribbonpanel.cpp 4107
The function returns the type BOOL. For the case when HWND can't be obtained from the parent window, the programmer wanted the function to return the value FALSE but made a typo and wrote S_FALSE instead, which drastically changes the statement's meaning.
This is how the S_FALSE constant is declared:
#define S_FALSE ((HRESULT)1L)
You've probably guessed already what happens, but I shall explain just in case.
Writing "return S_FALSE;" is the same thing as writing "return TRUE;". Epic fail.
This error is not alone, it has a few friends:
- V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. afxribbonbar.cpp 5623
- V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. afxribbonbar.cpp 5627
- V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. ctlnownd.cpp 349
- V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. olecli2.cpp 548
Note
As I already said in the beginning, not all of the files were checked. More than that, I could have missed something among those warnings the analyzer did issue. So, I'm asking the developers not to view this paper as a manual on fixing some of the bugs. It would be much better if you checked the libraries yourselves and closely examined the analyzer warnings.
Conclusion
This is just one more time that I managed to demonstrate how useful static analysis tools can be.
Please beware of one mistake. I hear every now and then that some programmers run static analyzers on their code before releasing. If you know someone who does that and finds it normal, know that they are very, very wrong, so please set them back on the right track. It's the most erroneous way of using static analyzers. It's just like keeping all compiler warnings disabled while working on a project and enabling them just before the release.
You are all welcome to download PVS-Studio demo version and try it on your projects.
PVS-Studio page: https://www.viva64.com/en/pvs-studio/
Supported languages and compilers:
- Windows. Visual Studio 2017 C, C++, C++/CLI, C++/CX (WinRT), C#
- Windows. Visual Studio 2015 C, C++, C++/CLI, C++/CX (WinRT), C#
- Windows. Visual Studio 2013 C, C++, C++/CLI, C++/CX (WinRT), C#
- Windows. Visual Studio 2012 C, C++, C++/CLI, C++/CX (WinRT), C#
- Windows. Visual Studio 2010 C, C++, C++/CLI, C#
- Windows. MinGW C, C++
- Windows/Linux. Clang C, C++
- Linux. GCC C, C++
Thanks for reading; follow me on Twitter: @Code_Analysis.
No comments:
Post a Comment