⬆️ ⬇️

Errors found in Visual C ++ 2012 libraries

One of the methods for detecting errors in programs is static code analysis. We are pleased that this methodology is becoming increasingly popular. In many ways, this contributes to Visual Studio, in which static analysis is one of many features. This functionality is easy to try and start using regularly. When a person understands that he likes static code analysis, we are pleased to offer him a professional analyzer PVS-Studio for C / C ++ / C ++ 11 languages.



Introduction



Visual Studio development environment allows static code analysis. This analysis is extremely useful and easy to use. However, it should be understood that Visual Studio performs a huge number of functions. This means that a single function always loses to specialized tools. The functions of refactoring and coloring code lose compared to Visual Assist. The function for inline image editing is naturally worse than Adobe Photoshop or CorelDRAW. The situation is similar with the static code analysis function.



We will not theorize. Let's go to practice. Let's see what we managed to notice interesting by the PVS-Studio analyzer in the folders of Visual Studio 2012.



In fact, we did not plan to check the source files included in Visual Studio. Everything happened by chance. In Visual Studio 2012, due to the support of the new C ++ standard, many header files have been modified. The task was to make sure that the PVS-Studio analyzer is able to process the header files included in it.

')

Unexpectedly, we noticed several errors in the header * .h files. We decided to study the files included in Visual Studio 2012 in more detail. Namely, such folders as:



There was no complete validation, as we have no projects or make-files for building libraries. So, it was possible to check only the modest part of the library code. Despite the incompleteness of the test, the results are very interesting.



Let's see what the PVS-Studio analyzer found inside the libraries for Visual C ++. As you can see, all these errors leaked past the analyzer built into Visual C ++ itself.



Some of the suspicious sites found



We will not claim that all the code snippets below contain errors. We simply chose from the list of places suggested by the PVS-Studio analyzer that seem to be the most suspicious.



Strange cycle



This suspicious code was found first. He served as the impetus for the continuation of the study.

template <class T> class ATL_NO_VTABLE CUtlProps : public CUtlPropsBase { .... HRESULT GetIndexOfPropertyInSet(....) { .... for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++) { if( dwPropertyId == pUPropInfo[ul].dwPropId ) *piCurPropId = ul; return S_OK; } return S_FALSE; } .... }; 


V612 An unconditional 'return' within a loop. atldb.h 4829



The body of the loop is executed only once. Error in the explanation does not need. Most likely, the operator 'return' should be called when the desired value is found. In this case, the code should look like this:

 for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++) { if( dwPropertyId == pUPropInfo[ul].dwPropId ) { *piCurPropId = ul; return S_OK; } } 


Suspicious projection



I apologize for the hard to read example. Pay attention to the condition in the ternary operator.

 // TEMPLATE FUNCTION proj _TMPLT(_Ty) inline _CMPLX(_Ty) proj(const _CMPLX(_Ty)& _Left) { // return complex projection return (_CMPLX(_Ty)( _CTR(_Ty)::_Isinf(real(_Left)) || _CTR(_Ty)::_Isinf(real(_Left)) ? _CTR(_Ty)::_Infv(real(_Left)) : real(_Left), imag(_Left) < 0 ? -(_Ty)0 : (_Ty)0)); } 


V501 There are identical sub-expressions '_Ctraits <_Ty> :: _ Isinf (real (_Left))' to the left | operator. xcomplex 780



The condition repeats the expression "_CTR (_Ty) :: _ Isinf (real (_Left))" twice. It is difficult to say if there is a mistake and how to correct the code. However, this feature clearly deserves attention.



Unnecessary check



 template<typename BaseType, bool t_bMFCDLL = false> class CSimpleStringT { .... void Append(_In_reads_(nLength) PCXSTR pszSrc, _In_ int nLength) { .... UINT nOldLength = GetLength(); if (nOldLength < 0) { // protects from underflow nOldLength = 0; } .... }; 


V547 Expression 'nOldLength <0' is always false. Unsigned type value is never <0. atlsimpstr.h 420



There is no error here. Judging by the code, the length of the string cannot become negative. In the class CSimpleStringT there are relevant checks. The fact that the nOldLength variable has an unsigned type does not spoil anything. Anyway, the length is always positive. This is just an extra code.



Wrong string formation



 template <class T> class CHtmlEditCtrlBase { .... HRESULT SetDefaultComposeSettings( LPCSTR szFontName=NULL, .....) const { CString strBuffer; .... strBuffer.Format(_T("%d,%d,%d,%d,%s,%s,%s"), bBold ? 1 : 0, bItalic ? 1 : 0, bUnderline ? 1 : 0, nFontSize, szFontColor, szBgColor, szFontName); .... } }; 


V576 Incorrect format. Consider checking the eighth actual argument of the Format. Wchar_t type symbols is expected. afxhtml.h 826



This code forms the wrong message in UNICODE programs. The function 'Format ()' expects the eighth argument to be of type LPCTSTR. But the variable 'szFontName' is always of type LPCSTR.



Port with a negative number



 typedef WORD ATL_URL_PORT; class CUrl { ATL_URL_PORT m_nPortNumber; .... inline BOOL Parse(_In_z_ LPCTSTR lpszUrl) { .... //get the port number m_nPortNumber = (ATL_URL_PORT) _ttoi(tmpBuf); if (m_nPortNumber < 0) goto error; .... }; 


V547 Expression 'm_nPortNumber <0' is always false. Unsigned type value is never <0. atlutil.h 2775



Checking that the port number is less than zero will not work. The variable 'm_nPortNumber' has the unsigned type 'WORD'. The type 'WORD' is 'unsigned short'.



Undefined behavior



The following macro is in the Visual C ++ header files.

 #define DXVABitMask(__n) (~((~0) << __n)) 


Wherever it is used, behavior arises indefinitely. Of course, Visual C ++ developers know better if this construction is safe or not. Perhaps there is confidence that Visual C ++ will always treat the shift of negative numbers in the same way. Formally, a negative number shift leads to Undefined behavior. More details on this topic are covered in the article " Without knowing the ford, do not climb into the water. Part three ".



Incorrect work in 64-bit mode



This pattern of 64-bit error is described in detail in the lessons we have drawn up on developing 64-bit C / C ++ applications. To understand what the error is, we suggest that you familiarize yourself with the lesson number 12 .

 class CWnd : public CCmdTarget { .... virtual void WinHelp(DWORD_PTR dwData, UINT nCmd = HELP_CONTEXT); .... }; class CFrameWnd : public CWnd { .... }; class CFrameWndEx : public CFrameWnd { .... virtual void WinHelp(DWORD dwData, UINT nCmd = HELP_CONTEXT); .... }; 


V301 Unexpected function overloading behavior. WinHelpW 'in derived class' CFrameWndEx' and base class' CFrameWnd '. afxframewndex.h 154



The 'WinHelp' function is declared incorrectly in the 'CFrameWndEx' class. The type of the first argument must be 'DWORD_PTR'. A similar error can be seen in some other classes:



The pointer at the beginning is used and then compared to NULL.



Such places have been found quite a lot. To analyze whether each case is dangerous or not is very tiring. The creators of libraries will do much better with this. We will give only a couple of examples.

 BOOL CDockablePane::PreTranslateMessage(MSG* pMsg) { .... CBaseTabbedPane* pParentBar = GetParentTabbedPane(); CPaneFrameWnd* pParentMiniFrame = pParentBar->GetParentMiniFrame(); if (pParentBar != NULL && (pParentBar->IsTracked() || pParentMiniFrame != NULL && pParentMiniFrame->IsCaptured() ) ) .... } 


V595 The 'pParentBar' pointer was used before it was verified against nullptr. Check lines: 2840, 2841. afxdockablepane.cpp 2840



See, at the beginning, the 'pParentBar' pointer is used to call the GetParentMiniFrame () function. The Zetas are suddenly suspected that this pointer may be NULL and the corresponding check is being performed.

 AFX_CS_STATUS CDockingManager::DeterminePaneAndStatus(....) { .... CDockablePane* pDockingBar = DYNAMIC_DOWNCAST(CDockablePane, *ppTargetBar); if (!pDockingBar->IsFloating() && (pDockingBar->GetCurrentAlignment() & dwEnabledAlignment) == 0) { return CS_NOTHING; } if (pDockingBar != NULL) { return pDockingBar->GetDockingStatus( pt, nSensitivity); } .... } 


V595 The 'pDockingBar' pointer was used before it was verified against nullptr. Check lines: 582, 587. afxdockingmanager.cpp 582



At the beginning, the 'pDockingBar' pointer is actively used, and then suddenly compared to NULL.



And one more example at last:

 void CFrameImpl::AddDefaultButtonsToCustomizePane(....) { .... for (POSITION posCurr = lstOrigButtons.GetHeadPosition(); posCurr != NULL; i++) { CMFCToolBarButton* pButtonCurr = (CMFCToolBarButton*)lstOrigButtons.GetNext(posCurr); UINT uiID = pButtonCurr->m_nID; if ((pButtonCurr == NULL) || (pButtonCurr->m_nStyle & TBBS_SEPARATOR) || (....) { continue; } .... } 


V595 The 'pButtonCurr' pointer was used before it was verified against nullptr. Check lines: 1412, 1414. afxframeimpl.cpp 1412



Feel free to contact the member of the class 'm_nID'. And then from the condition it is clear that the 'pButtonCurr' pointer can be equal to 0.



Using a destroyed object



 CString m_strBrowseFolderTitle; void CMFCEditBrowseCtrl::OnBrowse() { .... LPCTSTR lpszTitle = m_strBrowseFolderTitle != _T("") ? m_strBrowseFolderTitle : (LPCTSTR)NULL; .... } 


V623 Consider inspecting the '?:' Operator. A temporary object is being created and subsequently destroyed. afxeditbrowsectrl.cpp 308



The ternary operator cannot return values ​​of different types. Therefore, an object of type CString is implicitly created from "(LPCTSTR) NULL". Then from this empty string will be implicitly taken a pointer to its buffer. The trouble is that a temporary object of type CString will be destroyed. As a result, the value of the 'lpszTitle' pointer becomes not valid and it is impossible to work with it. Here you can read a more detailed description of this error pattern.



Incorrect work with time



 UINT CMFCPopupMenuBar::m_uiPopupTimerDelay = (UINT) -1; .... void CMFCPopupMenuBar::OnChangeHot(int iHot) { .... SetTimer(AFX_TIMER_ID_MENUBAR_REMOVE, max(0, m_uiPopupTimerDelay - 1), NULL); .... } 


V547 Expression '(0)> (m_uiPopupTimerDelay - 1)' is always false. Unsigned type value is never <0. afxpopupmenubar.cpp 968



The value '-1' is used as a special flag. Using the macro 'max', programmers wanted to protect themselves against negative values ​​in the m_uiPopupTimerDelay variable. This will not work, since the variable is of unsigned type. It is always greater than or equal to zero. The correct code should look something like this:

 SetTimer(AFX_TIMER_ID_MENUBAR_REMOVE, m_uiPopupTimerDelay == (UINT)-1 ? 0 : m_uiPopupTimerDelay - 1, NULL); 


A similar error is found here:



Pointless string



 BOOL CMFCTasksPaneTask::SetACCData(CWnd* pParent, CAccessibilityData& data) { .... data.m_nAccHit = 1; data.m_strAccDefAction = _T("Press"); data.m_rectAccLocation = m_rect; pParent->ClientToScreen(&data.m_rectAccLocation); data.m_ptAccHit; return TRUE; } 


V607 Ownerless expression 'data.m_ptAccHit'. afxtaskspane.cpp 96



What is "data.m_ptAccHit;"? Perhaps you forgot to assign a value to this variable?



Maybe you need another one 0?



 BOOL CMFCTasksPane::GetMRUFileName(....) { .... const int MAX_NAME_LEN = 512; TCHAR lpcszBuffer [MAX_NAME_LEN + 1]; memset(lpcszBuffer, 0, MAX_NAME_LEN * sizeof(TCHAR)); if (GetFileTitle((*pRecentFileList)[nIndex], lpcszBuffer, MAX_NAME_LEN) == 0) { strName = lpcszBuffer; return TRUE; } .... } 


V512 A call of the memset function will lead to underflow of the buffer lpcszBuffer. afxtaskspane.cpp 2626



There is a suspicion that this code may return a string that will not terminate with a terminal zero. Most likely, the last element of the array should also be reset:

 memset(lpcszBuffer, 0, (MAX_NAME_LEN + 1) * sizeof(TCHAR)); 


Strange 'if'



 void CMFCVisualManagerOfficeXP::OnDrawBarGripper(....) { .... if (bHorz) { rectFill.DeflateRect(4, 0); } else { rectFill.DeflateRect(4, 0); } .... } 


V523 The 'then' statement is equivalent to the 'else' statement. afxvisualmanagerofficexp.cpp 264



Dangerous class single_link_registry



If you use the 'single_link_registry' class, then your application may terminate unexpectedly, even if you correctly handle all exceptions. Let's look at the destructor of the 'single_link_registry' class:

 virtual ~single_link_registry() { // It is an error to delete link registry with links // still present if (count() != 0) { throw invalid_operation( "Deleting link registry before removing all the links"); } } 


V509 The 'throw' operator inside the destructor should be placed within the try..catch block. Raising exception inside the destructor is illegal. agents.h 759



This destructor may throw an exception. It is a bad idea. If an exception occurs in the program, the destruction of objects begins by calling destructors. If an error occurs in the 'single_link_registry' destructor, another exception will be raised. It is not processed in the destructor. As a result, the C ++ library immediately crashes the program by calling the terminate () function.



Similar bad destructors:



Another weird cycle



 void CPreviewView::OnPreviewClose() { .... while (m_pToolBar && m_pToolBar->m_pInPlaceOwner) { COleIPFrameWnd *pInPlaceFrame = DYNAMIC_DOWNCAST(COleIPFrameWnd, pParent); if (!pInPlaceFrame) break; CDocument *pViewDoc = GetDocument(); if (!pViewDoc) break; // in place items must have a server document. COleServerDoc *pDoc = DYNAMIC_DOWNCAST(COleServerDoc, pViewDoc); if (!pDoc) break; // destroy our toolbar m_pToolBar->DestroyWindow(); m_pToolBar = NULL; pInPlaceFrame->SetPreviewMode(FALSE); // restore toolbars pDoc->OnDocWindowActivate(TRUE); break; } .... } 


V612 An unconditional 'break' within a loop. viewprev.cpp 476



There is no 'continue' statement in the loop. At the end of the cycle is a 'break'. It is very strange. The loop is always executed only once. Here is either an error or better to replace 'while' with 'if'



Strange constant



There are other irrelevant notes on the code that are not interesting to list. We give only one example, so that it is clear what is at stake.



In the file afxdrawmanager.cpp, for some reason, a constant is entered for Pi:

 const double AFX_PI = 3.1415926; 


V624 The constant 3.1415926 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. afxdrawmanager.cpp 22



This is certainly not an error and the accuracy of the constant is sufficient. But it is not clear why not to use the constant M_PI, which is much more accurate:

 #define M_PI 3.14159265358979323846 


Contacting Visual C ++ Developers



Unfortunately, we do not have a project and makefiles for building libraries included in Visual C ++. Therefore, our analysis is very superficial. We just found something and wrote about it. Do not think that there are no other suspicious places :).



We are sure that it will be much more convenient for you to use PVS-Studio to check the libraries. If necessary, we are ready to prompt and help with integration into make-files. It will also be easier to understand what is a mistake and what is not.



findings



See, in Visual Studio 2012 there is a static analysis of C / C ++ code. However, this does not mean that it is enough. This is only the first step. This is only an opportunity to easily and cheaply try a new technology to improve the quality of the code. And when you like it, come to us and purchase PVS-Studio. This tool is much more intense struggling with defects. He is under this sharpened. We earn money on it, which means we are actively developing it.



We found errors in the Visual C ++ libraries, although there is a static analysis. We found errors in the Clang compiler, although it has static analysis. Acquire us and we will regularly find errors in your project. We are well interned in Visual Studio 2005, 2008, 2010, 2012 and are able to look for errors in the background.

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



All Articles