Eine schlampige Überprüfung der Visual C++ 2013-Bibliothek (Update 3)

Eine schlampige Überprüfung der Visual C++ 2013-Bibliothek (Update 3)

Jemand hat mir kürzlich vorgeschlagen, die Bibliotheken von Visual Studio 2013 zu überprüfen. Ich habe nichts Interessantes gefunden, nur ein paar kleine Fehler und Ausrutscher. Sie würden keinen interessanten, attraktiven Artikel abgeben, aber ich habe mich dennoch entschlossen, all diese Mängel zu beschreiben. Ich hoffe nur, dass es dazu beiträgt, die Bibliotheken ein wenig besser zu machen und die Autoren dazu anregt, eine gründlichere Analyse durchzuführen. Ich habe nicht die Projektdateien, die zum Erstellen der Bibliotheken erforderlich sind, daher musste meine Analyse oberflächlich sein und ich hätte viel übersehen können.

Dies ist der zweite Artikel über die Analyse der Visual C++-Bibliotheken. Die Ergebnisse der vorherigen Prüfung finden Sie im Artikel In den Bibliotheken von Visual C++ 2012 erkannte Fehler.

Ich kann die Bibliotheken nicht vollständig analysieren und was ich gemacht habe, war ein ziemlich schlampiger Check:Ich habe alle Dateien aus den Ordnern "crt\src" und "atlmfc\src" in ein neues Projekt eingefügt und auch eine neue Datei test.cpp erstellt fügen Sie alle Header-Dateien ein, die sich auf die Standardbibliothek beziehen (Vektor, Karte, Satz usw.) in.

Danach habe ich ein bisschen mit den Projekteinstellungen herumgespielt und es schließlich geschafft, etwa 80% der Dateien zu kompilieren. Ich denke, das reicht völlig aus. Selbst wenn eine Datei nicht kompiliert werden kann, kann PVS-Studio sie normalerweise trotzdem überprüfen, wenn auch nur teilweise.

Ich denke, wenn die Entwickler der Bibliotheken diesen Artikel nützlich und interessant finden, werden sie eine gründlichere Analyse durchführen. Selbst der exotische Bauprozess ist kein Problem mehr, da Sie das Compiler-Überwachungssystem verwenden können.

Ich habe PVS-Studio 5.19 verwendet, um die Analyse durchzuführen. Ich habe die Quellcodes der in Visual Studio 2013 (Update 3) enthaltenen C/C++-Bibliotheken überprüft.

Analyseergebnisse

Ich habe ein paar Fehler gefunden, die auch in der vorherigen Version, Visual Studio 2012, gefunden wurden. Zum Beispiel ist die Funktion proj() immer noch auf ziemlich seltsame Weise implementiert; Der Destruktor ~single_link_registry() ist auf die gleiche gefährliche Weise geschrieben. Aber es ist nicht interessant, dieselbe Geschichte zu erzählen. Versuchen wir, etwas Neues zu finden.

Falsche Indexprüfung

void _Initialize_order_node(...., size_t _Index, ....)
{
  if (_Index < 0)
  {
    throw std::invalid_argument("_Index");
  }
  ....
}

Diagnosemeldung von PVS-Studio:V547 Ausdruck '_Index <0' ist immer falsch. Der Wert des vorzeichenlosen Typs ist nie <0. agents.h 8442

Das Argument „_Index“ ist vorzeichenlos. Aus diesem Grund macht die Prüfung keinen Sinn, da niemals eine Ausnahme generiert wird. Es sieht eher nach überflüssigem Code als nach einem Fehler aus.

Falsches Format

int _tfpecode; /* float point exception code */

void __cdecl _print_tiddata1 (
  _ptiddata ptd
)
{
  ....
  printf("\t_gmtimebuf      = %p\n",   ptd->_gmtimebuf);
  printf("\t_initaddr       = %p\n",   ptd->_initaddr);
  printf("\t_initarg        = %p\n",   ptd->_initarg);
  printf("\t_pxcptacttab    = %p\n",   ptd->_pxcptacttab);
  printf("\t_tpxcptinfoptrs = %p\n",   ptd->_tpxcptinfoptrs);
  printf("\t_tfpecode       = %p\n\n", ptd->_tfpecode);
  ....
}

Diagnosemeldung von PVS-Studio:V576 Falsches Format. Erwägen Sie, das zweite tatsächliche Argument der Funktion „printf“ zu überprüfen. Als Argument wird der Zeiger erwartet. tidprint.c 133

Wir haben es hier mit dem Last-Line-Effekt zu tun. Am Ende eines Blocks ähnlich aussehender Zeilen ist ein Fehler. In jeder Zeile sollte ein Zeigerwert ausgegeben werden, aber in der letzten Zeile ist die Variable „_tfpecode“ nur ein ganzzahliger Wert, kein Zeiger. Was stattdessen hätte geschrieben werden sollen, ist Folgendes:

printf("\t_tfpecode       = %i\n\n", ptd->_tfpecode);

Seltsame sich wiederholende Berechnungen

unsigned int SchedulerProxy::AdjustAllocationIncrease(....) const
{
  ....
  unsigned int remainingConcurrency = 
                         m_maxConcurrency - m_currentConcurrency;
  remainingConcurrency = m_maxConcurrency - m_currentConcurrency;
  ....
}

Diagnosemeldung von PVS-Studio:V519 Die Variable 'remainingConcurrency' wird zweimal hintereinander mit Werten belegt. Vielleicht ist dies ein Fehler. Überprüfen Sie die Zeilen:1136, 1137. schedulerproxy.cpp 1137

Der Variablen wird zweimal das Ergebnis ein und desselben Ausdrucks zugewiesen. Dieser Code ist überflüssig und resultiert höchstwahrscheinlich aus einem schlechten Refactoring.

Tippfehler vermutet

double HillClimbing::CalculateThroughputSlope(....)
{
  ....
  MeasuredHistory * lastHistory = GetHistory(fromSetting);
  MeasuredHistory * currentHistory = GetHistory(toSetting);
  ....
  double varianceOfcurrentHistory = currentHistory->VarianceMean();
  double varianceOflastHistory = currentHistory->VarianceMean();
  ....
}

Diagnosemeldung von PVS-Studio:V656 Variablen 'varianceOfcurrentHistory', 'varianceOflastHistory' werden durch den Aufruf derselben Funktion initialisiert. Es ist wahrscheinlich ein Fehler oder nicht optimierter Code. Erwägen Sie, den Ausdruck „currentHistory->VarianceMean()“ zu untersuchen. Überprüfen Sie die Zeilen:412, 413. hillclimbing.cpp 413

Verdächtig ist, dass den Variablen varianceOfcurrentHistory und varianceOflastHistory ein und derselbe Wert zugewiesen wird. Es wäre logischer, die Variable varianceOflastHistory folgendermaßen zu initialisieren:

double varianceOflastHistory = varianceOfcurrentHistory;

Außerdem gibt es noch den 'lastHistory'-Zeiger. Meine Vermutung ist, dass dieser Code einen Tippfehler enthält und höchstwahrscheinlich so aussehen sollte:

double varianceOfcurrentHistory = currentHistory->VarianceMean();
double varianceOflastHistory = lastHistory->VarianceMean();

Echter Tippfehler

BOOL CPropertySheet::SetActivePage(CPropertyPage* pPage)
{
  ASSERT_VALID(this);
  ENSURE_VALID(pPage);
  ASSERT_KINDOF(CPropertyPage, pPage);

  int nPage = GetPageIndex(pPage);
  ASSERT(pPage >= 0);

  return SetActivePage(nPage);
}

Diagnosemeldung von PVS-Studio:V503 Unsinniger Vergleich:Zeiger>=0. dlgprop.cpp 1206

Es ist seltsam, einen Zeigerwert darauf zu prüfen, ob er größer oder gleich Null ist. Das ist offensichtlich ein Tippfehler und der Programmierer wollte eigentlich die 'nPage'-Variable überprüfen:

int nPage = GetPageIndex(pPage);
ASSERT(nPage >= 0);

Das ist natürlich nur ein ASSERT und der Fehler wird keine ernsthaften Probleme verursachen, aber es ist immer noch ein Fehler.

Ausführen derselben Aktionen unabhängig von der Bedingung

void CMFCVisualManager::OnDrawTasksGroupCaption(....)
{
  ....
  if (pGroup->m_bIsSpecial)
  {
    if (!pGroup->m_bIsCollapsed)
    {
      CMenuImages::Draw(pDC, CMenuImages::IdArrowUp,
                        rectButton.TopLeft());
    }
    else
    {
      CMenuImages::Draw(pDC, CMenuImages::IdArrowDown,
                        rectButton.TopLeft());
    }
  }
  else
  {
    if (!pGroup->m_bIsCollapsed)
    {
      CMenuImages::Draw(pDC, CMenuImages::IdArrowUp,
                        rectButton.TopLeft());
    }
    else
    {
      CMenuImages::Draw(pDC, CMenuImages::IdArrowDown,
                        rectButton.TopLeft());
    }
  }
  ....
}

Diagnosemeldung von PVS-Studio:V523 Die 'then'-Anweisung entspricht der 'else'-Anweisung. afxvisualmanager.cpp 2118

Unabhängig von der Bedingung (pGroup->m_bIsSpecial) werden dieselben Aktionen ausgeführt. Das ist seltsam.

Falsche Überprüfung der Portnummer

typedef WORD ATL_URL_PORT;
ATL_URL_PORT m_nPortNumber;

inline BOOL Parse(_In_z_ LPCTSTR lpszUrl)
{
  ....
  m_nPortNumber = (ATL_URL_PORT) _ttoi(tmpBuf);
  if (m_nPortNumber < 0)
    goto error;
  ....
}

Diagnosemeldung von PVS-Studio:V547 Ausdruck 'm_nPortNumber <0' ist immer falsch. Der Wert des vorzeichenlosen Typs ist nie <0. atlutil.h 2773

Die Variable 'm_nPortNumber' ist vom Typ unsigned WORD.

Virtueller Destruktor fehlt

class CDataSourceControl
{
  ....
  ~CDataSourceControl();
  ....
  virtual IUnknown* GetCursor();
  virtual void BindProp(....);
  virtual void BindProp(....);
  ....
}

CDataSourceControl* m_pDataSourceControl;

COleControlSite::~COleControlSite()
{
  ....
  delete m_pDataSourceControl;
  ....
}

Diagnosemeldung von PVS-Studio:V599 Der Destruktor wurde nicht als virtuell deklariert, obwohl die Klasse 'CDataSourceControl' virtuelle Funktionen enthält. occsite.cpp 77

Die CDataSourceControl-Klasse enthält virtuelle Methoden, aber der Destruktor ist nicht virtuell. Das ist gefährlich:Wenn eine X-Klasse von der CDataSourceControl-Klasse geerbt wird, können Sie keine Objekte des X-Typs zerstören, indem Sie einen Zeiger auf die Basisklasse verwenden.

Unvollständiger Code

BOOL CMFCWindowsManagerDialog::OnHelpInfo(HELPINFO* pHelpInfo)
{
  pHelpInfo->iCtrlId;
  CWnd* pParentFrame = AfxGetMainWnd();
  pParentFrame->SendMessage(AFX_WM_WINDOW_HELP, 0,
                            (LPARAM) this);
  return FALSE;
}

Diagnosemeldung von PVS-Studio:V607 Besitzerloser Ausdruck 'pHelpInfo->iCtrlId'. afxwindowsmanagerdialog.cpp 472

Was ist "pHelpInfo->iCtrlId;"? Was bedeutet das?

Verdächtige doppelte Initialisierung

CMFCStatusBar::CMFCStatusBar()
{
  m_hFont = NULL;

  // setup correct margins
  m_cxRightBorder = m_cxDefaultGap;  // <=
  m_cxSizeBox = 0;

  m_cxLeftBorder = 4;
  m_cyTopBorder = 2;
  m_cyBottomBorder = 0;
  m_cxRightBorder = 0;               // <=
  ....
}

Diagnosemeldung von PVS-Studio:V519 Die Variable 'm_cxRightBorder' wird zweimal hintereinander mit Werten belegt. Vielleicht ist dies ein Fehler. Überprüfen Sie die Zeilen:74, 80. afxstatusbar.cpp 80

Zunächst wird ein Wert einer anderen Variablen in die Variable 'm_cxRightBorder' geschrieben. Und dann wird es plötzlich auf Null gesetzt.

Verdächtige Statusprüfung

#define S_OK  ((HRESULT)0L)
#define E_NOINTERFACE  _HRESULT_TYPEDEF_(0x80004002L)

HRESULT GetDocument(IHTMLDocument2** ppDoc) const
{  
  const T* pT = static_cast<const T*>(this);
  return pT->GetDHtmlDocument(ppDoc) ? S_OK : E_NOINTERFACE;
}

HRESULT GetEvent(IHTMLEventObj **ppEventObj) const
{
  ....
  if (GetDocument(&sphtmlDoc))
  ....
}

Diagnosemeldung von PVS-Studio:V545 Dieser bedingte Ausdruck des Operators „if“ ist für den HRESULT-Typwert „GetDocument(&sphtmlDoc)“ falsch. Stattdessen sollte das Makro SUCCEEDED oder FAILED verwendet werden. afxhtml.h 593

Die Codeformatierung scheint nicht der Codeausführungslogik zu entsprechen. Zunächst denken Sie vielleicht, dass Sie das Dokument erhalten haben, wenn die Bedingung „GetDocument(...)“ wahr ist. Aber eigentlich ist alles ganz anders. Die Funktion GetDocument() gibt einen Wert vom Typ HRESULT zurück. Und bei diesem Typ ist alles anders. Beispielsweise wird der S_OK-Status als 0 und der E_NOINTERFACE-Status als 0x80004002L codiert. Um Werte vom Typ HRESULT zu überprüfen, sollten spezielle Makros verwendet werden:SUCCEEDED, FAILED.

Ich weiß nicht genau, ob hier ein Fehler vorliegt, aber dieser Code ist trotzdem verwirrend und muss überprüft werden.

Falsches Argument für das MAKE_HRESULT-Makro

#define MAKE_HRESULT(sev,fac,code) \
  ((HRESULT) \
   (((unsigned long)(sev)<<31) | \
    ((unsigned long)(fac)<<16) | \
    ((unsigned long)(code))) )

ATLINLINE ATLAPI AtlSetErrorInfo(....)
{
  ....
  hRes = MAKE_HRESULT(3, FACILITY_ITF, nID);
  ....
}

Diagnosemeldung von PVS-Studio:V673 Der Ausdruck „(unsigned long)(3) <<31“ ergibt 6442450944. 33 Bits sind erforderlich, um den Wert zu speichern, aber der Ausdruck ergibt den Typ „unsigned“, der nur „32“ enthalten kann 'Bits. atlcom.h 6650

Der Code wird so funktionieren, wie er sollte, aber es ist immer noch ein Fehler darin. Lesen Sie weiter für die Erklärung.

Die Funktion muss innerhalb einer Variablen vom Typ HRESULT eine Fehlermeldung bilden. Dazu wird das Makro MAKE_HRESULT verwendet. Trotzdem wird es falsch verwendet. Der Programmierer ging davon aus, dass der erste Parameter 'severity' Werte im Bereich zwischen 0 und 3 annimmt. Er muss ihn mit der Art und Weise verwechselt haben, wie Fehlercodes bei der Arbeit mit den Funktionen GetLastError()/SetLastError() gebildet werden.

Das MAKE_HRESULT-Makro kann nur 0 (Erfolg) oder 1 (Fehler) als erstes Argument annehmen. Einzelheiten zu diesem Thema finden Sie im Forum auf der CodeGuru-Website:Warnung! Makro MAKE_HRESULT funktioniert nicht.

Da die Zahl 3 als erstes tatsächliches Argument verwendet wird, tritt ein Überlauf auf. Die Zahl 3 wird zu 1 "verwandelt". Dieser glückliche Zufall verhindert, dass der Fehler die Programmausführung beeinträchtigt.

ASSERTs mit immer wahren Bedingungen

Es gibt ziemlich viele Fragmente, in denen eine ASSERT-Bedingung im Muster (X>=0) implementiert ist. Gleichzeitig wird eine X-Variable als unsigned Integer-Typ deklariert. Die Bedingung ist also immer wahr.

In einigen Fällen ist die Verwendung solcher ASSERTs gültig - z. wenn eine Variable aufgrund von Refactoring signiert werden kann und der Algorithmus nicht bereit ist, negative Zahlen zu verarbeiten. In diesem Code scheint es jedoch keinen Sinn zu machen, einige davon zu verwenden. Sie sollten aus dem Code entfernt oder durch andere nützliche Überprüfungen ersetzt werden. Deshalb habe ich beschlossen, sie in dem Artikel zu erwähnen.

Überprüfen Sie dieses Beispiel:

DWORD m_oversubscribeCount; 

void ExternalContextBase::Oversubscribe(....)
{
  if (beginOversubscription)
  {
    ASSERT(m_oversubscribeCount >= 0);
    ++m_oversubscribeCount;
  }
  ....
}

Diagnosemeldung von PVS-Studio:V547 Ausdruck 'm_oversubscribeCount>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. externalcontextbase.cpp 204

Und hier ist die Liste aller anderen Ausgaben dieser Art:

  • V547 Ausdruck 'm_oversubscribeCount>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. internalcontextbase.cpp 506
  • V547 Ausdruck 'pGlobalNode->m_idleCores>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 3764
  • V547 Ausdruck 'pGlobalNode->m_availableCores>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 3769
  • V547 Ausdruck 'pReceivingProxyData->m_allocation>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 4100
  • V547 Ausdruck 'pReceivingProxyData->m_allocation>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 4360
  • V547 Ausdruck 'exclusiveCoresAvailable>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 4657
  • V547 Ausdruck 'coresNeeded>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 4657
  • V547 Ausdruck ' previousGlobal>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 4781
  • V547 Ausdruck 'currentGlobal>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 4782
  • V547 Ausdruck 'm_minConcurrency>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. schedulerproxy.cpp 63
  • V547 Ausdruck 'm_minimumHardwareThreads>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. schedulerproxy.cpp 125
  • V547 Ausdruck 'm_oversubscribeCount>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. umsthreadinternalcontext.cpp 308
  • V547 Ausdruck 'j>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 1922
  • V547 Ausdruck 'pMaxNode->m_availableCores>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 2542
  • V547 Ausdruck 'vorherigerOrt>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 4793
  • V547 Ausdruck 'currentLocal>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. resourcemanager.cpp 4794
  • V547 Ausdruck ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. schedulerpolicybase.cpp 285
  • V547 Ausdruck 'Wert>=0' ist immer wahr. Der Wert des vorzeichenlosen Typs ist immer>=0. schedulerpolicybase.cpp 345

Überflüssige Typkonvertierungen

Ich habe ein paar explizite Typumwandlungen gefunden, die nicht nur überflüssig sind, sondern auch Werte verderben können.

Beispiel eins:

size_t __cdecl strnlen(const char *str, size_t maxsize);
size_t __cdecl _mbstrnlen_l(const char *s,
                            size_t sizeInBytes,
                            _locale_t plocinfo)
{
  ....
  if ( _loc_update.GetLocaleT()->locinfo->mb_cur_max == 1 )
      /* handle single byte character sets */
      return (int)strnlen(s, sizeInBytes);
  ....
}

Diagnosemeldung von PVS-Studio:V220 Verdächtige Folge von Typumwandlungen:Speichergröße -> 32-Bit-Ganzzahl -> Speichergröße. Der umzuwandelnde Wert:„strnlen(s, sizeInBytes)“. _mbslen_s.c 67

Die Funktion strnlen() gibt einen Wert vom Typ 'size_t' zurück. Dann wird es plötzlich explizit in den Typ „int“ umgewandelt. Danach wird der Wert implizit wieder auf den Typ size_t erweitert.

Dieser Code enthält ein potenzielles 64-Bit-Problem. Versucht man in einer 64-Bit-Programmversion, die Anzahl der Zeichen in einem sehr langen String mit der Funktion _mbstrnlen_l() zu berechnen, erhält man ein falsches Ergebnis.

Ich schätze, diese explizite Typumwandlung wurde versehentlich im Code belassen und muss einfach entfernt werden.

Beispiel zwei:

WINBASEAPI SIZE_T WINAPI GlobalSize (_In_ HGLOBAL hMem);

inline void __cdecl memcpy_s(
  _Out_writes_bytes_to_(_S1max,_N)  void *_S1,
  _In_ size_t _S1max,
  _In_reads_bytes_(_N) const void *_S2,
  _In_ size_t _N);

AFX_STATIC HGLOBAL AFXAPI _AfxCopyGlobalMemory(....)
{
  ULONG_PTR nSize = ::GlobalSize(hSource);
  ....
  Checked::memcpy_s(lpDest, (ULONG)::GlobalSize(hDest),
                    lpSource, (ULONG)nSize);
  ....
}

Diagnosemeldung von PVS-Studio:V220 Verdächtige Folge von Typumwandlungen:Speichergröße -> 32-Bit-Ganzzahl -> Speichergröße. Der umzuwandelnde Wert:„nSize“. olemisc.cpp 684.

Die Funktion GlobalSize() gibt den Typ SIZE_T zurück. Die Argumente der Funktion memcpy_s() haben ebenfalls den Typ size_t.

Wozu dient dann die explizite Typumwandlung "(ULONG)::GlobalSize(hDest)"?

Wenn wir beginnen, mit einem Puffer zu arbeiten, der größer als 4 GB ist, kopiert die Funktion memcpy_s() nur einen Teil des Arrays.

Es gibt noch einige andere verdächtige Typumwandlungen:

  • V220 Verdächtige Folge von Typumwandlungen:Speichergröße -> 32-Bit-Ganzzahl -> Speichergröße. Der gecastete Wert:'wcslen(* vp ++)'. cenvarg.c 236
  • V220 Verdächtige Folge von Typumwandlungen:Speichergröße -> 32-Bit-Ganzzahl -> Speichergröße. Der umzuwandelnde Wert:'::GlobalSize(m_hGlobalMemory)'. fileshrd.cpp 48
  • V220 Verdächtige Folge von Typumwandlungen:Speichergröße -> 32-Bit-Ganzzahl -> Speichergröße. Der gecastete Wert:'wcslen(lpsz)'. dumpcont.cpp 31
  • V220 Verdächtige Folge von Typumwandlungen:Speichergröße -> 32-Bit-Ganzzahl -> Speichergröße. Der gecastete Wert:'wcslen(lpsz)'. dumpcont.cpp 82
  • V220 Verdächtige Folge von Typumwandlungen:Speichergröße -> 32-Bit-Ganzzahl -> Speichergröße. Der umzuwandelnde Wert:'(cElems * sizeof (CLSID))'. ctlcore.cpp 1975
  • V220 Verdächtige Folge von Typumwandlungen:Speichergröße -> 32-Bit-Ganzzahl -> Speichergröße. Der umzuwandelnde Wert:„wParam“. afxtoolbarslistcheckbox.cpp 94
  • V220 Verdächtige Folge von Typumwandlungen:Speichergröße -> 32-Bit-Ganzzahl -> Speichergröße. Der umzuwandelnde Wert:'nChars * sizeof (TCHAR)'. statreg.h 270

Zeiger vor einer Prüfung verwenden

CMFCPopupMenu* CMFCCustomizeButton::CreatePopupMenu()
{
  ....
  if (m_pWndParentToolbar->IsLocked())
  {
    pMenu->GetMenuBar()->m_pRelatedToolbar = m_pWndParentToolbar;
  }

  pMenu->m_bRightAlign = m_bMenuRightAlign &&
    (m_pWndParentToolbar->GetExStyle() & WS_EX_LAYOUTRTL) == 0;

  BOOL bIsLocked = (m_pWndParentToolbar == NULL ||
                    m_pWndParentToolbar->IsLocked());
  ....
}

Diagnosemeldung von PVS-Studio:V595 Der Zeiger 'm_pWndParentToolbar' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen:192, 199. afxcustomizebutton.cpp 192

Der 'm_pWndParentToolbar'-Zeiger wird zuerst im 'm_pWndParentToolbar->IsLocked()'-Ausdruck dereferenziert und dann auf Null geprüft:'m_pWndParentToolbar ==NULL'.

Es ist gefährlicher Code und ich glaube nicht, dass ich erklären sollte, warum.

Ein anderer Fall wie dieser:

void COleControlSite::BindDefaultProperty(....)
{
  ....
  if (pDSCWnd != NULL)
  {
    ....
    m_pDSCSite = pDSCWnd->m_pCtrlSite;
    ....
    m_pDSCSite->m_pDataSourceControl->BindProp(this, TRUE);
    if (m_pDSCSite != NULL)
      m_pDSCSite->m_pDataSourceControl->BindColumns();
  }
  ....
}

Diagnosemeldung von PVS-Studio:V595 Der Zeiger 'm_pDSCSite' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen:1528, 1529. occsite.cpp 1528

Überflüssige Variablen

Überflüssige Variablen sind keine Fehler. Aber da sie überflüssig sind, will man sie trotzdem nicht im Code haben und sollte sie loswerden. Zum Beispiel:

int GetImageCount() const
{
  CRect rectImage(m_Params.m_rectImage);
  if (m_Bitmap.GetCount() == 1)
  {
    HBITMAP hBmp = m_Bitmap.GetImageWell();
    BITMAP bmp;

    if (::GetObject(hBmp, sizeof(BITMAP), &bmp) ==
        sizeof(BITMAP))
    {
      return bmp.bmHeight / m_Params.m_rectImage.Height();
    }

    return 0;
  }

  return m_Bitmap.GetCount();
}

Diagnosemeldung von PVS-Studio:V808 Objekt „rectImage“ vom Typ „CRect“ wurde erstellt, aber nicht verwendet. afxcontrolrenderer.h 89

Das 'rectImage'-Rechteck wird erstellt, aber danach nicht mehr verwendet. Daher haben wir eine zusätzliche Zeile im Programm und ein paar zusätzliche Prozessortaktzyklen, die ausgeführt werden müssen, wenn wir mit der Debug-Version arbeiten.

Hier ist eine Datei mit einer Liste aller überflüssigen Variablen:vs2003_V808.txt

Verschiedenes

Ziemlich viele Warnungen von PVS-Studio weisen eher auf einen schlechten Programmierstil als auf Fehler hin. Meiner Meinung nach sollten die Quellcodes der Visual C++-Bibliotheken als Vorbild für andere Programmierer dienen und es bringt nichts, ihnen schlechte Dinge beizubringen.

Einige Fragmente, die verbessert werden können, sind unten aufgeführt.

Gefährliche Vergleiche mit TRUE

_PHNDLR __cdecl signal(int signum, _PHNDLR sigact)
{
  ....
  if ( SetConsoleCtrlHandler(ctrlevent_capture, TRUE)
       == TRUE )
  ....
}

Diagnosemeldung von PVS-Studio:V676 Es ist falsch, die Variable vom Typ BOOL mit TRUE zu vergleichen. winsig.c 255

Jede Quelle, einschließlich MSDN, sagt uns, dass es eine schlechte Praxis ist, irgendetwas mit TRUE zu vergleichen. Die Funktion kann jeden anderen Wert als 0 zurückgeben, der als TRUE zählt. Aber TRUE ist 1. Die korrekte Berechnung eines solchen Vergleichs ist also Foo() !=FALSE.

Andere ähnliche Vergleiche:

  • V676 Es ist falsch, die Variable vom Typ BOOL mit TRUE zu vergleichen. event.cpp 448
  • V676 Es ist falsch, die Variable vom Typ BOOL mit TRUE zu vergleichen. Richtiger Ausdruck ist:'retVal !=FALSE'. resourcemanager.cpp 1437
  • V676 Es ist falsch, die Variable vom Typ BOOL mit TRUE zu vergleichen. Richtiger Ausdruck ist:'retVal !=FALSE'. resourcemanager.cpp 5027

Erhöhen

void _To_array(
  ::Concurrency::details::_Dynamic_array<_EType>& _Array)
{
  _LockHolder _Lock(_M_lock);
  _M_iteratorCount++;

  for(_LinkRegistry::iterator _Link = _M_links.begin();
      *_Link != NULL; _Link++)
  {
    _Array._Push_back(*_Link);
  }
}

Diagnosemeldung von PVS-Studio:V803 Verringerte Leistung. Falls '_Link' ein Iterator ist, ist es effektiver, die Präfixform des Inkrements zu verwenden. Ersetzen Sie Iterator++ durch ++Iterator. agenten.h 1713

Es ist nur eine so subtile Nuance, aber alle Quellen empfehlen die Verwendung von ++ iterator. Wo immer möglich, ist es besser, einen Präfix-Operator als guten Codierungsstil zu verwenden, den andere lernen können.

Notiz. Ein paar Beiträge zum Thema:

  • Ist es sinnvoll, den Präfix-Inkrementoperator ++it anstelle des Postfix-Operators it++ für Iteratoren zu verwenden?.
  • Pre- vs. Post-Increment-Operator - Benchmark.

Wenn die Autoren der Bibliotheken entscheiden, dass sie an diesen Inkrementen arbeiten sollten, hier ist die Liste aller Fragmente, die ich gefunden habe:vs2003_V803.txt.

Falsche Wiederherstellung des Warnstatus

#pragma warning (disable : 4311)
SetClassLongPtr(m_hWnd,
  GCLP_HBRBACKGROUND,
  PtrToLong(reinterpret_cast<void*>(
    ::GetSysColorBrush(COLOR_BTNFACE))));
#pragma warning (default : 4311)

Die V665-Diagnosemeldung:Möglicherweise ist die Verwendung von '#pragma warning(default:X)' in diesem Zusammenhang falsch. Stattdessen sollte die '#pragma warning(push/pop)' verwendet werden. Überprüfen Sie die Zeilen:165, 167. afxbasepane.cpp 167

Ein korrekter Weg, um den vorherigen Warnstatus wiederherzustellen, ist die Verwendung von "#pragma warning(push[ ,n ])" und "#pragma warning(pop)".

Andere ähnliche Fragmente:vs2003_V665.txt.

Die Prüfung (dies ==NULL)

Das ist ein Klassiker des Genres:

_AFXWIN_INLINE CWnd::operator HWND() const
  { return this == NULL ? NULL : m_hWnd; }

Die Diagnosemeldung von PVS-Studio:V704 'this ==0'-Ausdruck sollte vermieden werden - dieser Ausdruck ist auf neueren Compilern immer falsch, da 'this'-Zeiger niemals NULL sein kann. afxwin2.inl 19

Leider ist das ein sehr häufiges Muster – insbesondere in MFC. Aber Programmierer sollten nach und nach lernen, auf solche Konstrukte zu verzichten und stattdessen anderen ein gutes Beispiel zu geben.

Diejenigen, die noch nicht wissen, warum es schlecht ist, finden eine detaillierte Erklärung in der Dokumentation zur V704-Diagnose.

Ich verstehe, dass der Operator HWND() wirklich nicht behoben werden kann:Die Abwärtskompatibilität ist wichtiger. Aber warum nicht überall dort, wo es ohne schmerzliche Folgen möglich ist? Hier ist die Liste aller Prüfungen dieser Art:vs2003_V704.txt

Schlussfolgerung

Wie Sie sehen können, fällt der Artikel ziemlich groß aus. Aber eigentlich gibt es in den Bibliotheken nichts allzu Interessantes oder Entscheidendes; Ihr Code ist definitiv von hoher Qualität und gut debuggt.

Ich freue mich, wenn dieser Artikel dazu beiträgt, die Visual C++-Bibliotheken in Zukunft ein wenig besser zu machen. Lassen Sie mich noch einmal darauf hinweisen, dass das, was ich getan habe, eine unvollständige Analyse war. Die Entwickler der Visual C++-Bibliotheken können eine viel bessere und gründlichere Ausführung durchführen, da sie über Skripte/Projekte zum Erstellen der Bibliotheken verfügen. Wenn Sie auf Probleme stoßen, helfe ich Ihnen gerne bei deren Lösung – wenden Sie sich an unseren Support-Service.