Geekly articles weekly

Contando errores en la calculadora de Windows


Hace unos días, Microsoft puso a disposición del público el código fuente de su Calculadora de Windows. Calculator es una aplicación que tradicionalmente se incluye con cada versión de Windows. Varios proyectos de Microsoft pasaron al código abierto en los últimos años, pero esta vez las noticias fueron cubiertas incluso por medios que no son de TI el primer día. Bueno, es un programa popular pero pequeño en C ++. A pesar de su tamaño, todavía logramos encontrar una serie de fragmentos sospechosos en su código utilizando el analizador estático PVS-Studio.

Introduccion


No creo que necesitemos presentar Calculadora, ya que difícilmente encontrarías un usuario de Windows que no sepa qué es. Ahora cualquiera puede descargar el código fuente de la aplicación desde GitHub y sugerir sus mejoras.

La siguiente función, por ejemplo, ya atrajo la atención de la comunidad:

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

Esta función registra el texto del portapapeles y aparentemente lo envía a los servidores de Microsoft. Sin embargo, esta publicación no trata sobre esa función, pero seguro que verá muchos fragmentos sospechosos.

Utilizamos el analizador estático PVS-Studio para verificar el código fuente de la Calculadora. Como no está escrito en C ++ estándar, muchos de nuestros lectores habituales dudaban de que tal verificación fuera posible, pero lo hicimos. El analizador admite C ++ / CLI y C ++ / CX, y aunque algunos diagnósticos produjeron algunos falsos positivos, no encontramos ningún problema crítico que obstaculizara el trabajo de PVS-Studio.

Solo como recordatorio, en caso de que se haya perdido la noticia sobre otras capacidades de nuestra herramienta, PVS-Studio no solo admite C y C ++, sino también C # y Java.

Comparación incorrecta de cadenas


V547 La expresión 'm_resolvedName == L "en-US"' siempre es falsa. Para comparar cadenas debe usar la función 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()); } .... }

Al ver los informes del analizador, clasifico las advertencias por código de diagnóstico en orden ascendente, y este, que es un ejemplo bastante vívido, resultó ser el primero en la lista.

Verá, el ejemplo anterior muestra una comparación incorrecta de cadenas. De hecho, el programador está comparando punteros en lugar de valores de cadena al comparar la dirección de una matriz de caracteres con la de un literal de cadena. Estos punteros nunca son iguales, por lo que la condición siempre es falsa también. Para una correcta comparación de cadenas, uno debería usar la función wcscmp , por ejemplo.

Por cierto, mientras escribía este artículo, la matriz de caracteres m_resolvedName se arregló en el archivo de encabezado y se convirtió en una cadena completa de tipo std :: wstring , por lo que la comparación se puede hacer correctamente ahora. En el momento en que leerá este artículo, muchos otros errores probablemente también se solucionarán gracias a los entusiastas y las críticas como esta.

Pérdida de memoria en código nativo


V773 Se salió de la función sin soltar el puntero 'temp'. Una pérdida de memoria es posible. 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; }

El puntero temporal se refiere a una matriz de 100 elementos asignada dinámicamente. Desafortunadamente, la memoria se libera solo en una parte de la función, mientras que el resto termina con una pérdida de memoria. No es tan malo, pero todavía se considera un error en el código C ++.

Excepción evasiva


Las clases V702 siempre deben derivarse de std :: exception (y similares) como 'público' (no se especificó ninguna palabra clave, por lo que el compilador lo predetermina a 'privado'). CalcManager CalcException.h 4

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

El analizador ha detectado una clase derivada de la clase std :: exception utilizando el modificador privado (que es el predeterminado si no se especifican otros modificadores). El problema con este código es que el controlador ignorará la excepción de tipo CalcException cuando intente detectar una excepción genérica std :: ya que la herencia privada prohíbe la conversión de tipo implícita.

Día perdido


V719 La instrucción switch no cubre todos los valores de la enumeración '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(); }

Es sospechoso que la declaración de cambio no tenga el caso DateUnit :: Day . Debido a eso, el valor del día no se agregará al calendario (la variable m_calendar ), aunque el calendario tiene el método AddDays .

Otros casos sospechosos con otra enumeración:

  • V719 La instrucción switch no cubre todos los valores de la enumeración 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 109
  • V719 La instrucción switch no cubre todos los valores de la enumeración 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 204
  • V719 La instrucción switch no cubre todos los valores de la enumeración 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 276

Comparación sospechosa de números reales


V550 Una comparación precisa impar: relación == umbral. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B) <Epsilon. Calculadora 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); }

El analizador señaló la relación de expresión sospechosa == umbral . Estas variables son de tipo doble y, por lo tanto, difícilmente podrían compararse con precisión utilizando el operador regular igual. Además, el valor de la variable de relación es el resultado de una operación de división.

Un código así se ve particularmente extraño en una aplicación como Calculadora. Incluyo una lista completa de las advertencias de este tipo por si acaso:

  • V550 Una comparación precisa extraña. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 752
  • V550 Una comparación extraña y precisa: stod (roundedString)! = 0.0. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 778
  • V550 Una comparación precisa extraña. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 790
  • V550 Una comparación extraña y precisa: stod (roundedString)! = 0.0. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 820
  • V550 Una comparación extraña y precisa: conversionTable [m_toType] .ratio == 1.0. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 980
  • V550 Una comparación extraña y precisa: conversionTable [m_toType] .offset == 0.0. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 980
  • V550 Una comparación precisa extraña: returnValue! = 0. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 1000
  • V550 Una comparación extraña y precisa: sizeToUse! = 0.0. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 270
  • V550 Una comparación extraña y precisa: sizeToUse! = 0.0. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 289
  • V550 Una comparación extraña y precisa: sizeToUse! = 0.0. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 308
  • V550 Una comparación extraña y precisa: sizeToUse! = 0.0. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 327
  • V550 Una comparación precisa impar: stod (stringToLocalize) == 0. Probablemente sea mejor usar una comparación con precisión definida: fabs (A - B) <Epsilon. CalcViewModel UnitConverterViewModel.cpp 388

Secuencia de funciones sospechosas


V1020 La función salió sin llamar a la función 'TraceLogger :: GetInstance (). LogNewWindowCreationEnd'. Líneas de verificación: 396, 375. Calculadora 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; .... }

El diagnóstico V1020 inspecciona los bloques de código y busca ramas con una llamada de función faltante mediante heurística.

El fragmento anterior contiene un bloque con las llamadas a las funciones LogNewWindowCreationBegin y LogNewWindowCreationEnd . A esto le sigue otro bloque donde se llama a la función LogNewWindowCreationEnd solo si se cumplen ciertas condiciones, lo que parece muy sospechoso.

Pruebas poco confiables


V621 Considere inspeccionar el operador 'para'. Es posible que el bucle se ejecute incorrectamente o no se ejecute en absoluto. 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); } .... }

El analizador ha detectado un bucle for que no se ejecuta en absoluto, lo que significa que las pruebas tampoco se ejecutan. El valor inicial del botón del contador de bucle (93) es mayor que el valor final (0) desde el principio.

V760 Se encontraron dos bloques de texto idénticos. El segundo bloque comienza desde la línea 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); }

Otra prueba sospechosa. El analizador ha detectado dos fragmentos de código idénticos ejecutándose inmediatamente uno después del otro. Parece que este código fue escrito usando la técnica de copiar y pegar y el programador olvidó modificar las copias.

V601 El valor 'falso' se convierte implícitamente en el tipo entero. Inspeccione el segundo argumento. CalculatorUnitTests CalcInputTest.cpp 352

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

La función ToRational se llama con el valor booleano falso , mientras que el parámetro correspondiente es de tipo int32_t y se llama precisión .

Decidí rastrear el valor por el código y vi que luego se pasó a la función StringToRat :

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

y luego a StringToNumber :

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

Aquí está el cuerpo de la función objetivo:

 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 precisión ahora se llama comenzar y participa en la expresión cdigits> comenzar , lo cual es muy sospechoso porque se pasó falso como el valor original.

Redundancia


V560 Una parte de la expresión condicional siempre es verdadera: 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 ya se comparó con el valor NumbersAndOperatorsEnum :: None , por lo que se puede eliminar la verificación duplicada.

V728 Se puede simplificar una verificación excesiva. El '(A y B) || (! A &&! B) 'expresión es equivalente a la expresión' bool (A) == bool (B) '. Calculadora Calculadora.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); } } }

Esta enorme expresión condicional tenía originalmente 218 caracteres de longitud, pero la dividí en varias líneas con fines de demostración. Se puede reescribir en una versión mucho más corta y, lo más importante, más clara:

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

V524 Es extraño que el cuerpo de la función 'ConvertBack' sea completamente equivalente al cuerpo de la función 'Convert'. Calculadora 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; }

El analizador ha detectado dos funciones implementadas idénticamente. Como sus nombres, Convert y ConvertBack , sugieren, estaban destinados a hacer cosas diferentes, pero los desarrolladores deberían saberlo mejor.

Conclusión


Supongo que cada proyecto de Microsoft que se hizo de código abierto nos dio la oportunidad de mostrar la importancia del análisis estático, incluso en proyectos tan pequeños como Calculator. Las grandes empresas, como Microsoft, Google, Amazon, etc., emplean a muchos desarrolladores talentosos, pero siguen siendo humanos cometiendo errores. Las herramientas de análisis estático son uno de los mejores medios para ayudar a cualquier equipo de desarrolladores a mejorar la calidad de sus productos.

Bienvenido a descargar PVS-Studio y probarlo en su propia "Calculadora". :-)

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

More articles:


All Articles