Geekly articles weekly

Compter les bogues dans la calculatrice Windows


Il y a quelques jours, Microsoft a rendu public le code source de leur calculatrice Windows. Calculatrice est une application qui est traditionnellement livrée avec toutes les versions de Windows. Un certain nombre de projets Microsoft sont devenus open source au cours des dernières années, mais cette fois, l'actualité a été couverte même par des médias non informatiques dès le premier jour. Eh bien, c'est un programme populaire mais minuscule en C ++. Malgré sa taille, nous avons quand même réussi à trouver un certain nombre de fragments suspects dans son code en utilisant l'analyseur statique PVS-Studio.

Présentation


Je ne pense pas que nous ayons besoin d'introduire Calculator car vous ne trouveriez guère un utilisateur Windows qui ne sait pas ce que c'est. Désormais, n'importe qui peut télécharger le code source de l'application depuis GitHub et suggérer leurs améliorations.

La fonction suivante, par exemple, a déjà attiré l'attention de la communauté:

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); }

Cette fonction enregistre le texte du presse-papiers et l'envoie apparemment aux serveurs Microsoft. Ce post, cependant, ne concerne pas cette fonction, mais vous verrez certainement de nombreux extraits suspects.

Nous avons utilisé l'analyseur statique PVS-Studio pour vérifier le code source de Calculator. Comme il n'est pas écrit en C ++ standard, beaucoup de nos lecteurs réguliers doutaient qu'une telle vérification soit possible, mais nous l'avons fait. L'analyseur prend en charge C ++ / CLI et C ++ / CX, et même si certains diagnostics ont produit quelques faux positifs, nous n'avons rencontré aucun problème critique qui gênerait le travail de PVS-Studio.

Pour rappel, au cas où vous auriez manqué les informations sur les autres fonctionnalités de notre outil, PVS-Studio prend en charge non seulement C et C ++ mais aussi C # et Java.

Comparaison de chaînes incorrecte


V547 L' expression 'm_resolvedName == L "en-US"' est toujours fausse. Pour comparer des chaînes, vous devez utiliser la fonction wcscmp (). Calculator 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()); } .... }

Lors de l'affichage des rapports de l'analyseur, je trie les avertissements par code de diagnostic dans l'ordre croissant, et celui-ci, qui constitue un exemple assez vivant, s'est avéré être le premier de la liste.

Vous voyez, l'exemple ci-dessus montre une comparaison incorrecte des chaînes. Le programmeur compare en fait des pointeurs au lieu de valeurs de chaîne en comparant l'adresse d'un tableau de caractères à celle d'un littéral de chaîne. Ces pointeurs ne sont jamais égaux, la condition est donc toujours fausse également. Pour une comparaison correcte des chaînes, on devrait utiliser la fonction wcscmp , par exemple.

À propos, pendant que j'écrivais cet article, le tableau de caractères m_resolvedName a été corrigé dans le fichier d'en-tête et est devenu une chaîne complète de type std :: wstring , de sorte que la comparaison peut être effectuée correctement maintenant. Au moment où vous lirez cet article, de nombreux autres bugs seront probablement corrigés aussi grâce aux passionnés et aux critiques comme celle-ci.

Fuite de mémoire en code natif


V773 La fonction a été fermée sans relâcher le pointeur 'temp'. Une fuite de mémoire est possible. 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; }

Le pointeur temporaire fait référence à un tableau alloué dynamiquement de 100 éléments. Malheureusement, la mémoire n'est libérée que dans une partie de la fonction, tandis que tous les autres se retrouvent avec une fuite de mémoire. Ce n'est pas trop mal, mais il est toujours considéré comme un bogue dans le code C ++.

Exception insaisissable


Les classes V702 doivent toujours être dérivées de std :: exception (et similaires) en tant que «public» (aucun mot-clé n'a été spécifié, donc le compilateur le définit par défaut sur «privé»). CalcManager CalcException.h 4

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

L'analyseur a détecté une classe dérivée de la classe std :: exception à l'aide du modificateur private (qui est par défaut si aucun autre modificateur n'est spécifié). Le problème avec ce code est que le gestionnaire ignorera l'exception de type CalcException lorsqu'il tentera d'attraper une exception std :: générique car l'héritage privé interdit la conversion de type implicite.

Jour manqué


V719 L'instruction switch ne couvre pas toutes les valeurs de l'énumération 'DateUnit': Day. 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(); }

Il est suspect que l'instruction switch n'ait pas de cas DateUnit :: Day . Pour cette raison, la valeur du jour ne sera pas ajoutée au calendrier (la variable m_calendar ), bien que le calendrier ait la méthode AddDays .

Autres cas suspects avec une autre énumération:

  • V719 L'instruction switch ne couvre pas toutes les valeurs de l'énumération 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 109
  • V719 L'instruction switch ne couvre pas toutes les valeurs de l'énumération 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 204
  • V719 L'instruction switch ne couvre pas toutes les valeurs de l'énumération 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 276

Comparaison suspecte de nombres réels


V550 Une comparaison étrange et précise: ratio == seuil. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B) <Epsilon. Calculatrice 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); }

L'analyseur a souligné le taux d' expression suspecte == seuil . Ces variables sont de type double et, par conséquent, pourraient difficilement être comparées avec précision à l'aide de l'opérateur égal régulier. Par ailleurs, la valeur de la variable ratio est le résultat d'une opération de division.

Un code comme celui-ci semble particulièrement étrange dans une application comme Calculatrice. J'inclus une liste complète des avertissements de ce type au cas où:

  • V550 Une comparaison précise étrange. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 752
  • V550 Une comparaison étrange et précise: stod (roundString)! = 0.0. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 778
  • V550 Une comparaison précise étrange. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 790
  • V550 Une comparaison étrange et précise: stod (roundString)! = 0.0. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 820
  • V550 Une comparaison étrange et précise: table de conversion [m_toType] .ratio == 1.0. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 980
  • V550 Une comparaison étrange et précise: conversionTable [m_toType] .offset == 0.0. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 980
  • V550 Une comparaison étrange et précise: returnValue! = 0. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 1000
  • V550 Une comparaison étrange et précise: sizeToUse! = 0.0. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 270
  • V550 Une comparaison étrange et précise: sizeToUse! = 0.0. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 289
  • V550 Une comparaison étrange et précise: sizeToUse! = 0.0. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 308
  • V550 Une comparaison étrange et précise: sizeToUse! = 0.0. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 327
  • V550 Une comparaison précise étrange: stod (stringToLocalize) == 0. Il est probablement préférable d'utiliser une comparaison avec une précision définie: fabs (A - B) <Epsilon. CalcViewModel UnitConverterViewModel.cpp 388

Séquence de fonctions suspectes


V1020 La fonction s'est terminée sans appeler la fonction 'TraceLogger :: GetInstance (). LogNewWindowCreationEnd'. Vérifier les lignes: 396, 375. Calculatrice 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 inspecte les blocs de code et recherche les branches avec un appel de fonction manquant à l'aide de l'heuristique.

L'extrait ci-dessus contient un bloc avec les appels aux fonctions LogNewWindowCreationBegin et LogNewWindowCreationEnd . Ceci est suivi par un autre bloc où la fonction LogNewWindowCreationEnd est appelée uniquement si certaines conditions sont remplies, ce qui semble très suspect.

Tests peu fiables


V621 Envisagez d'inspecter l'opérateur «pour». Il est possible que la boucle soit mal exécutée ou ne soit pas exécutée du tout. 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); } .... }

L'analyseur a détecté une boucle for qui ne s'exécute pas du tout, ce qui signifie que les tests ne s'exécutent pas non plus. La valeur initiale du bouton de compteur de boucle (93) est supérieure à la valeur finale (0) dès le début.

V760 Deux blocs de texte identiques ont été trouvés. Le deuxième bloc commence à la ligne 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); }

Un autre test suspect. L'analyseur a détecté deux fragments de code identiques s'exécutant immédiatement l'un après l'autre. Il semble que ce code ait été écrit en utilisant la technique du copier-coller et le programmeur a oublié de modifier les copies.

V601 La valeur 'false' est implicitement convertie en type entier. Inspectez le deuxième argument. CalculatorUnitTests CalcInputTest.cpp 352

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

La fonction ToRational est appelée avec la valeur booléenne false , tandis que le paramètre correspondant est de type int32_t et est appelé précision .

J'ai décidé de suivre la valeur dans le code et j'ai vu qu'elle était ensuite passée à la fonction StringToRat :

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

puis à StringToNumber :

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

Voici le corps de la fonction cible:

 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; } .... }

La variable de précision s'appelle désormais starting et participe à l'expression cdigits> starting , ce qui est très suspect car false a été transmis comme valeur d'origine.

Redondance


V560 Une partie de l'expression conditionnelle est toujours vraie: 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) { .... } .... } .... }

La variable op a déjà été comparée à la valeur NumbersAndOperatorsEnum :: None , de sorte que la vérification en double peut être supprimée.

V728 Un contrôle excessif peut être simplifié. Le '(A && B) || (! A &&! B) 'est équivalente à l'expression' bool (A) == bool (B) '. Calculatrice 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); } } }

Cette énorme expression conditionnelle comptait à l'origine 218 caractères, mais je l'ai divisée en plusieurs lignes à des fins de démonstration. Il peut être réécrit dans une version beaucoup plus courte et, surtout, plus claire:

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

V524 Il est étrange que le corps de la fonction 'ConvertBack' soit entièrement équivalent au corps de la fonction 'Convert'. Calculator 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; }

L'analyseur a détecté deux fonctions implémentées de manière identique. Comme leurs noms, Convert et ConvertBack , le suggèrent, ils étaient censés faire des choses différentes, mais les développeurs devraient en savoir plus.

Conclusion


Je suppose que chaque projet Microsoft qui a été rendu open-source nous a donné l'occasion de montrer l'importance de l'analyse statique - même sur des projets aussi petits que Calculator. Les grandes entreprises, telles que Microsoft, Google, Amazon, etc., emploient beaucoup de développeurs talentueux, mais ce sont toujours des humains qui font des erreurs. Les outils d'analyse statique sont l'un des meilleurs moyens d'aider toute équipe de développeurs à améliorer la qualité de leurs produits.

Bienvenue pour télécharger PVS-Studio et l'essayer sur votre propre «calculatrice». :-)

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

More articles:


All Articles