Geekly articles weekly

Zählen von Fehlern im Windows-Rechner


Vor einigen Tagen hat Microsoft den Quellcode des Windows-Rechners öffentlich zugänglich gemacht. Calculator ist eine Anwendung, die traditionell mit jeder Windows-Version ausgeliefert wird. Eine Reihe von Microsoft-Projekten wurde in den letzten Jahren als Open Source-Projekt veröffentlicht. Diesmal wurden die Nachrichten jedoch bereits am ersten Tag von Nicht-IT-Medien berichtet. Nun, es ist ein beliebtes, aber winziges Programm in C ++. Trotz seiner Größe konnten wir mit dem statischen Analysegerät PVS-Studio immer noch eine Reihe verdächtiger Fragmente in seinem Code finden.

Einführung


Ich glaube nicht, dass wir Calculator einführen müssen, da Sie kaum einen Windows-Benutzer finden würden, der nicht weiß, was es ist. Jetzt kann jeder den Quellcode der App von GitHub herunterladen und Verbesserungen vorschlagen.

Die folgende Funktion hat beispielsweise bereits die Aufmerksamkeit der Community auf sich gezogen:

void TraceLogger::LogInvalidInputPasted(....) { if (!GetTraceLoggingProviderEnabled()) return; 

  LoggingFields fields{}; fields.AddString(L"Mode", NavCategory::GetFriendlyName(mode)->Data()); fields.AddString(L"Reason", reason); fields.AddString(L"PastedExpression", pastedExpression); fields.AddString(L"ProgrammerNumberBase", GetProgrammerType(...).c_str()); fields.AddString(L"BitLengthType", GetProgrammerType(bitLengthType).c_str()); LogTelemetryEvent(EVENT_NAME_INVALID_INPUT_PASTED, fields); }

Diese Funktion protokolliert Text aus der Zwischenablage und sendet ihn anscheinend an Microsoft-Server. In diesem Beitrag geht es jedoch nicht um diese Funktion, aber Sie werden mit Sicherheit viele verdächtige Schnipsel sehen.

Wir haben den statischen Analysator PVS-Studio verwendet, um den Quellcode des Rechners zu überprüfen. Da es nicht in Standard-C ++ geschrieben ist, bezweifelten viele unserer regelmäßigen Leser, dass eine solche Überprüfung möglich wäre, aber wir haben es getan. Der Analysator unterstützt C ++ / CLI und C ++ / CX, und obwohl einige Diagnosen einige Fehlalarme ergaben, sind keine kritischen Probleme aufgetreten, die die Arbeit von PVS-Studio behindern würden.

Nur zur Erinnerung, falls Sie die Neuigkeiten über andere Funktionen unseres Tools verpasst haben, unterstützt PVS-Studio nicht nur C und C ++, sondern auch C # und Java.

Falscher Zeichenfolgenvergleich


V547 Der Ausdruck 'm_resolvedName == L "en-US"' ist immer falsch. Um Zeichenfolgen zu vergleichen, sollten Sie die Funktion wcscmp () verwenden. Rechner LocalizationSettings.h 180

 wchar_t m_resolvedName[LOCALE_NAME_MAX_LENGTH]; Platform::String^ GetEnglishValueFromLocalizedDigits(....) const { if (m_resolvedName == L"en-US") { return ref new Platform::String(localizedString.c_str()); } .... }

Beim Anzeigen von Analysatorberichten sortiere ich Warnungen nach Diagnosecode in aufsteigender Reihenfolge, und diese, die ein anschauliches Beispiel darstellt, war zufällig die erste auf der Liste.

Sie sehen, das obige Beispiel zeigt einen falschen Vergleich von Zeichenfolgen. Der Programmierer vergleicht tatsächlich Zeiger anstelle von Zeichenfolgenwerten, indem er die Adresse eines Zeichenarrays mit der eines Zeichenfolgenliteral vergleicht. Diese Zeiger sind niemals gleich, daher ist auch die Bedingung immer falsch. Für den korrekten Vergleich von Strings sollte man zum Beispiel die Funktion wcscmp verwenden .

Während ich diesen Artikel schrieb, wurde das Zeichenarray m_resolvedName übrigens in der Header-Datei fixiert und zu einer vollständigen Zeichenfolge vom Typ std :: wstring , sodass der Vergleich jetzt ordnungsgemäß durchgeführt werden kann. In dem Moment, in dem Sie diesen Artikel lesen, werden wahrscheinlich auch viele andere Fehler behoben, dank der Enthusiasten und Bewertungen wie dieser.

Speicherverlust im nativen Code


V773 Die Funktion wurde beendet, ohne den Zeiger 'temp' loszulassen . Ein Speicherverlust ist möglich. CalcViewModel StandardCalculatorViewModel.cpp 529

 void StandardCalculatorViewModel::HandleUpdatedOperandData(Command cmdenum) { .... wchar_t* temp = new wchar_t[100]; .... if (commandIndex == 0) { delete [] temp; return; } .... length = m_selectedExpressionLastData->Length() + 1; if (length > 50) { return; } .... String^ updatedData = ref new String(temp); UpdateOperand(m_tokenPosition, updatedData); displayExpressionToken->Token = updatedData; IsOperandUpdatedUsingViewModel = true; displayExpressionToken->CommandIndex = commandIndex; }

Der temporäre Zeiger bezieht sich auf ein dynamisch zugewiesenes Array von 100 Elementen. Leider wird der Speicher nur in einem Teil der Funktion freigegeben, während der Rest zu einem Speicherverlust führt. Es ist nicht schlecht, aber es wird immer noch als Fehler im C ++ - Code angesehen.

Schwer fassbare Ausnahme


V702- Klassen sollten immer von std :: exception (und ähnlichem) als 'public' abgeleitet werden (es wurde kein Schlüsselwort angegeben, daher ist der Compiler standardmäßig 'private'). CalcManager CalcException.h 4

 class CalcException : std::exception { public: CalcException(HRESULT hr) { m_hr = hr; } HRESULT GetException() { return m_hr; } private: HRESULT m_hr; };

Der Analysator hat eine von der Klasse std :: exception abgeleitete Klasse mithilfe des privaten Modifikators erkannt (dies ist die Standardeinstellung, wenn keine anderen Modifikatoren angegeben sind). Das Problem mit diesem Code besteht darin, dass der Handler die Ausnahme vom Typ CalcException ignoriert, wenn er versucht, eine generische std :: -Ausnahme abzufangen, da die private Vererbung die implizite Typkonvertierung verbietet.

Verpasster Tag


V719 Die switch-Anweisung deckt nicht alle Werte der Enum 'DateUnit': Day ab. CalcViewModel DateCalculator.cpp 279

 public enum class _Enum_is_bitflag_ DateUnit { Year = 0x01, Month = 0x02, Week = 0x04, Day = 0x08 }; Windows::Globalization::Calendar^ m_calendar; DateTime DateCalculationEngine::AdjustCalendarDate(Windows::Foundation::DateTime date, DateUnit dateUnit, int difference) { m_calendar→SetDateTime(date); switch (dateUnit) { case DateUnit::Year: { .... m_calendar->AddYears(difference); m_calendar->ChangeCalendarSystem(currentCalendarSystem); break; } case DateUnit::Month: m_calendar->AddMonths(difference); break; case DateUnit::Week: m_calendar->AddWeeks(difference); break; } return m_calendar->GetDateTime(); }

Es ist verdächtig, dass die switch-Anweisung keinen DateUnit :: Day- Fall enthält. Aus diesem Grund wird der Tageswert nicht zum Kalender hinzugefügt (die Variable m_calendar ), obwohl der Kalender über die AddDays- Methode verfügt.

Andere verdächtige Fälle mit einer anderen Aufzählung:

  • V719 Die switch-Anweisung deckt nicht alle Werte der Enumeration 'eANGLE_TYPE' ab: ANGLE_RAD. CalcManager trans.cpp 109
  • V719 Die switch-Anweisung deckt nicht alle Werte der Enumeration 'eANGLE_TYPE' ab: ANGLE_RAD. CalcManager trans.cpp 204
  • V719 Die switch-Anweisung deckt nicht alle Werte der Enumeration 'eANGLE_TYPE' ab: ANGLE_RAD. CalcManager trans.cpp 276

Verdächtiger Vergleich reeller Zahlen


V550 Ein merkwürdig präziser Vergleich: Verhältnis == Schwelle. Es ist wahrscheinlich besser, einen Vergleich mit definierter Genauigkeit zu verwenden: fabs (A - B) <Epsilon. Rechner AspectRatioTrigger.cpp 80

 void AspectRatioTrigger::UpdateIsActive(Size sourceSize) { double numerator, denominator; .... bool isActive = false; if (denominator > 0) { double ratio = numerator / denominator; double threshold = abs(Threshold); isActive = ((ratio > threshold) || (ActiveIfEqual && (ratio == threshold))); } SetActive(isActive); }

Der Analysator wies auf den Schwellenwert für das verdächtige Expressionsverhältnis == hin . Diese Variablen sind vom Typ double und können daher mit dem regulären Gleichheitsoperator kaum genau verglichen werden. Außerdem ist der Wert der Verhältnisvariablen das Ergebnis einer Divisionsoperation.

Code wie dieser sieht in einer Anwendung wie Calculator besonders seltsam aus. Ich füge eine vollständige Liste der Warnungen dieses Typs hinzu, nur für den Fall:

  • V550 Ein merkwürdig präziser Vergleich. Es ist wahrscheinlich besser, einen Vergleich mit definierter Genauigkeit zu verwenden: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 752
  • V550 Ein merkwürdig präziser Vergleich: stod (roundString)! = 0.0. Es ist wahrscheinlich besser, einen Vergleich mit definierter Präzision zu verwenden: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 778
  • V550 Ein merkwürdig präziser Vergleich. Es ist wahrscheinlich besser, einen Vergleich mit definierter Genauigkeit zu verwenden: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 790
  • V550 Ein merkwürdig präziser Vergleich: stod (roundString)! = 0.0. Es ist wahrscheinlich besser, einen Vergleich mit definierter Präzision zu verwenden: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 820
  • V550 Ein merkwürdiger genauer Vergleich: convertTable [m_toType] .ratio == 1.0. Es ist wahrscheinlich besser, einen Vergleich mit definierter Genauigkeit zu verwenden: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 980
  • V550 Ein merkwürdiger genauer Vergleich: convertTable [m_toType] .offset == 0.0. Es ist wahrscheinlich besser, einen Vergleich mit definierter Genauigkeit zu verwenden: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 980
  • V550 Ein merkwürdig präziser Vergleich: returnValue! = 0. Es ist wahrscheinlich besser, einen Vergleich mit definierter Genauigkeit zu verwenden: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 1000
  • V550 Ein merkwürdig präziser Vergleich: sizeToUse! = 0.0. Es ist wahrscheinlich besser, einen Vergleich mit definierter Präzision zu verwenden: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 270
  • V550 Ein merkwürdig präziser Vergleich: sizeToUse! = 0.0. Es ist wahrscheinlich besser, einen Vergleich mit definierter Präzision zu verwenden: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 289
  • V550 Ein merkwürdig präziser Vergleich: sizeToUse! = 0.0. Es ist wahrscheinlich besser, einen Vergleich mit definierter Präzision zu verwenden: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 308
  • V550 Ein merkwürdig präziser Vergleich: sizeToUse! = 0.0. Es ist wahrscheinlich besser, einen Vergleich mit definierter Präzision zu verwenden: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 327
  • V550 Ein merkwürdig präziser Vergleich: stod (stringToLocalize) == 0. Es ist wahrscheinlich besser, einen Vergleich mit definierter Genauigkeit zu verwenden: fabs (A - B) <Epsilon. CalcViewModel UnitConverterViewModel.cpp 388

Verdächtige Funktionsfolge


V1020 Die Funktion wurde beendet, ohne die Funktion 'TraceLogger :: GetInstance (). LogNewWindowCreationEnd' aufzurufen. Überprüfen Sie die Zeilen: 396, 375. Rechner App.xaml.cpp 396

 void App::OnAppLaunch(IActivatedEventArgs^ args, String^ argument) { .... if (!m_preLaunched) { auto newCoreAppView = CoreApplication::CreateNewView(); newCoreAppView->Dispatcher->RunAsync(....([....]() { TraceLogger::GetInstance().LogNewWindowCreationBegin(....); // <= Begin .... TraceLogger::GetInstance().LogNewWindowCreationEnd(....); // <= End })); } else { TraceLogger::GetInstance().LogNewWindowCreationBegin(....); // <= Begin ActivationViewSwitcher^ activationViewSwitcher; auto activateEventArgs = dynamic_cast<IViewSwitcherProvider^>(args); if (activateEventArgs != nullptr) { activationViewSwitcher = activateEventArgs->ViewSwitcher; } if (activationViewSwitcher != nullptr) { activationViewSwitcher->ShowAsStandaloneAsync(....); TraceLogger::GetInstance().LogNewWindowCreationEnd(....); // <= End TraceLogger::GetInstance().LogPrelaunchedAppActivatedByUser(); } else { TraceLogger::GetInstance().LogError(L"Null_ActivationViewSwitcher"); } } m_preLaunched = false; .... }

Diagnostic V1020 überprüft Codeblöcke und sucht mithilfe von Heuristiken nach Zweigen mit einem fehlenden Funktionsaufruf.

Das obige Snippet enthält einen Block mit den Aufrufen der Funktionen LogNewWindowCreationBegin und LogNewWindowCreationEnd . Darauf folgt ein weiterer Block, in dem die Funktion LogNewWindowCreationEnd nur aufgerufen wird, wenn bestimmte Bedingungen erfüllt sind, was sehr verdächtig aussieht.

Unzuverlässige Tests


V621 Überprüfen Sie den ' for' -Operator. Es ist möglich, dass die Schleife falsch oder gar nicht ausgeführt wird. CalculatorUnitTests UnitConverterViewModelUnitTests.cpp 500

 public enum class NumbersAndOperatorsEnum { .... Add = (int) CM::Command::CommandADD, // 93 .... None = (int) CM::Command::CommandNULL, // 0 .... }; TEST_METHOD(TestButtonCommandFiresModelCommands) { .... for (NumbersAndOperatorsEnum button = NumbersAndOperatorsEnum::Add; button <= NumbersAndOperatorsEnum::None; button++) { if (button == NumbersAndOperatorsEnum::Decimal || button == NumbersAndOperatorsEnum::Negate || button == NumbersAndOperatorsEnum::Backspace) { continue; } vm.ButtonPressed->Execute(button); VERIFY_ARE_EQUAL(++callCount, mock->m_sendCommandCallCount); VERIFY_IS_TRUE(UCM::Command::None == mock->m_lastCommand); } .... }

Der Analysator hat eine for- Schleife erkannt, die überhaupt nicht ausgeführt wird, was bedeutet, dass die Tests auch nicht ausgeführt werden. Der Anfangswert der Schleifenzählertaste (93) ist von Anfang an größer als der Endwert (0).

V760 Es wurden zwei identische Textblöcke gefunden. Der zweite Block beginnt in Zeile 688. CalculatorUnitTests UnitConverterViewModelUnitTests.cpp 683

 TEST_METHOD(TestSwitchAndReselectCurrentlyActiveValueDoesNothing) { shared_ptr<UnitConverterMock> mock = make_shared<UnitConverterMock>(); VM::UnitConverterViewModel vm(mock); const WCHAR * vFrom = L"1", *vTo = L"234"; vm.UpdateDisplay(vFrom, vTo); vm.Value2Active = true; // Establish base condition VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount); VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount); VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount); vm.Value2Active = true; VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount); VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount); VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount); }

Ein weiterer verdächtiger Test. Der Analysator hat zwei identische Codefragmente erkannt, die unmittelbar nacheinander ausgeführt werden. Es sieht so aus, als ob dieser Code mit der Copy-Paste-Technik geschrieben wurde und der Programmierer vergessen hat, die Kopien zu ändern.

V601 Der Wert 'false' wird implizit in den Integer-Typ umgewandelt. Untersuchen Sie das zweite Argument. CalculatorUnitTests CalcInputTest.cpp 352

 Rational CalcInput::ToRational(uint32_t radix, int32_t precision) { .... } TEST_METHOD(ToRational) { .... auto rat = m_calcInput.ToRational(10, false); .... }

Die ToRational- Funktion wird mit dem Booleschen Wert false aufgerufen, während der entsprechende Parameter vom Typ int32_t ist und als Genauigkeit bezeichnet wird .

Ich entschied mich, den Wert im Code aufzuspüren und sah, dass er dann an die StringToRat- Funktion übergeben wurde:

 PRAT StringToRat(...., int32_t precision) { .... }

und dann zu StringToNumber :

 PNUMBER StringToNumber(...., int32_t precision) { .... stripzeroesnum(pnumret, precision); .... }

Hier ist der Körper der Zielfunktion:

 bool stripzeroesnum(_Inout_ PNUMBER pnum, long starting) { MANTTYPE *pmant; long cdigits; bool fstrip = false; pmant=pnum->mant; cdigits=pnum->cdigit; if ( cdigits > starting ) // <= { pmant += cdigits - starting; cdigits = starting; } .... }

Die Präzisionsvariable heißt jetzt start und nimmt am Ausdruck cdigits> launch teil , was sehr verdächtig ist, da false als ursprünglicher Wert übergeben wurde.

Redundanz


V560 Ein Teil des bedingten Ausdrucks ist immer wahr: NumbersAndOperatorsEnum :: None! = Op. CalcViewModel UnitConverterViewModel.cpp 991

 void UnitConverterViewModel::OnPaste(String^ stringToPaste, ViewMode mode) { .... NumbersAndOperatorsEnum op = MapCharacterToButtonId(*it, canSendNegate); if (NumbersAndOperatorsEnum::None != op) // <= { .... if (NumbersAndOperatorsEnum::None != op && // <= NumbersAndOperatorsEnum::Negate != op) { .... } .... } .... }

Die op- Variable wurde bereits mit dem Wert NumbersAndOperatorsEnum :: None verglichen, sodass die doppelte Prüfung entfernt werden kann.

V728 Eine übermäßige Überprüfung kann vereinfacht werden. Die '(A && B) || (! A &&! B) 'Ausdruck entspricht dem Ausdruck' bool (A) == bool (B) '. Rechner Calculator.xaml.cpp 239

 void Calculator::AnimateCalculator(bool resultAnimate) { if (App::IsAnimationEnabled()) { m_doAnimate = true; m_resultAnimate = resultAnimate; if (((m_isLastAnimatedInScientific && IsScientific) || (!m_isLastAnimatedInScientific && !IsScientific)) && ((m_isLastAnimatedInProgrammer && IsProgrammer) || (!m_isLastAnimatedInProgrammer && !IsProgrammer))) { this->OnStoryboardCompleted(nullptr, nullptr); } } }

Dieser riesige bedingte Ausdruck war ursprünglich 218 Zeichen lang, aber ich habe ihn zu Demonstrationszwecken in mehrere Zeilen aufgeteilt. Es kann in eine viel kürzere und vor allem klarere Version umgeschrieben werden:

 if ( m_isLastAnimatedInScientific == IsScientific && m_isLastAnimatedInProgrammer == IsProgrammer) { this->OnStoryboardCompleted(nullptr, nullptr); }

V524 Es ist seltsam, dass der Hauptteil der Funktion 'ConvertBack' dem Hauptteil der Funktion 'Convert' vollständig entspricht. Rechner BooleanNegationConverter.cpp 24

 Object^ BooleanNegationConverter::Convert(....) { (void) targetType; // Unused parameter (void) parameter; // Unused parameter (void) language; // Unused parameter auto boxedBool = dynamic_cast<Box<bool>^>(value); auto boolValue = (boxedBool != nullptr && boxedBool->Value); return !boolValue; } Object^ BooleanNegationConverter::ConvertBack(....) { (void) targetType; // Unused parameter (void) parameter; // Unused parameter (void) language; // Unused parameter auto boxedBool = dynamic_cast<Box<bool>^>(value); auto boolValue = (boxedBool != nullptr && boxedBool->Value); return !boolValue; }

Der Analysator hat zwei identisch implementierte Funktionen erkannt. Wie ihre Namen Convert und ConvertBack andeuten, sollten sie verschiedene Dinge tun, aber die Entwickler sollten es besser wissen.

Fazit


Ich denke, jedes Microsoft-Projekt, das Open Source gemacht wurde, gab uns die Gelegenheit, die Bedeutung der statischen Analyse zu demonstrieren - selbst bei Projekten, die so klein wie Calculator sind. Große Unternehmen wie Microsoft, Google, Amazon usw. beschäftigen viele talentierte Entwickler, aber sie sind immer noch Menschen, die Fehler machen. Statische Analysetools sind eines der besten Mittel, um Entwicklerteams bei der Verbesserung der Qualität ihrer Produkte zu unterstützen.

Willkommen zum Herunterladen von PVS-Studio und zum Ausprobieren auf Ihrem eigenen „Rechner“. :-)

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

More articles:


All Articles