Geekly articles weekly

Contando erros na calculadora do Windows


Alguns dias atrás, a Microsoft disponibilizou publicamente o código-fonte da sua Calculadora Windows. O Calculator é um aplicativo que é fornecido tradicionalmente com todas as versões do Windows. Vários projetos da Microsoft foram de código aberto nos últimos anos, mas desta vez as notícias foram cobertas até pela mídia que não era de TI no primeiro dia. Bem, é um programa popular e minúsculo em C ++. Apesar de seu tamanho, ainda conseguimos encontrar vários fragmentos suspeitos em seu código usando o analisador estático PVS-Studio.

1. Introdução


Acho que não precisamos apresentar o Calculator, pois você dificilmente encontrará um usuário do Windows que não saiba o que é. Agora qualquer pessoa pode baixar o código-fonte do aplicativo no GitHub e sugerir suas melhorias.

A seguinte função, por exemplo, já atraiu a atenção da comunidade:

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

Essa função registra o texto da área de transferência e aparentemente o envia para os servidores da Microsoft. Esta postagem, no entanto, não é sobre essa função, mas você verá muitos trechos suspeitos com certeza.

Utilizamos o analisador estático PVS-Studio para verificar o código-fonte da Calculadora. Como não está escrito em C ++ padrão, muitos de nossos leitores regulares duvidavam que tal verificação fosse possível, mas nós o fizemos. O analisador suporta C ++ / CLI e C ++ / CX e, embora alguns diagnósticos tenham produzido alguns falsos positivos, não encontramos problemas críticos que dificultariam o trabalho do PVS-Studio.

Apenas como lembrete, caso você tenha perdido as notícias sobre outros recursos de nossa ferramenta, o PVS-Studio suporta não apenas C e C ++, mas também C # e Java.

Comparação incorreta de strings


A expressão V547 'm_resolvedName == L "en-US"' é sempre falsa. Para comparar strings, você deve usar a função wcscmp (). Localização da calculadoraSettings.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()); } .... }

Ao visualizar os relatórios do analisador, ordeno os avisos por código de diagnóstico em ordem crescente, e este, que faz um exemplo bastante vívido, passou a ser o primeiro da lista.

Você vê, o exemplo acima mostra uma comparação incorreta de strings. De fato, o programador está comparando ponteiros em vez de valores de string, comparando o endereço de uma matriz de caracteres com o de um literal de string. Esses ponteiros nunca são iguais, portanto a condição também é sempre falsa. Para uma comparação correta de strings, deve-se usar a função wcscmp , por exemplo.

A propósito, enquanto eu escrevia este artigo, a matriz de caracteres m_resolvedName foi corrigida no arquivo de cabeçalho e se tornou uma sequência completa do tipo std :: wstring , para que a comparação possa ser feita corretamente agora. No momento em que você ler este artigo, muitos outros erros provavelmente serão corrigidos também graças aos entusiastas e críticas como essa.

Vazamento de memória no código nativo


V773 A função foi encerrada sem liberar o ponteiro 'temp'. É possível um vazamento de memória. 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; }

O ponteiro temp refere-se a uma matriz alocada dinamicamente de 100 elementos. Infelizmente, a memória é liberada apenas em uma parte da função, enquanto todo o restante acaba com um vazamento de memória. Não é tão ruim, mas ainda é considerado um bug no código C ++.

Exceção indescritível


As classes V702 sempre devem ser derivadas de std :: exception (e similares) como 'public' (nenhuma palavra-chave foi especificada, então o compilador o padroniza como '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; };

O analisador detectou uma classe derivada da classe std :: exception usando o modificador privado (que é o padrão se nenhum outro modificador for especificado). O problema com esse código é que o manipulador ignorará a exceção do tipo CalcException ao tentar capturar uma std :: exception genérica, pois a herança privada proíbe a conversão implícita de tipos.

Dia perdido


V719 A instrução switch não cobre todos os valores da enumeração '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(); }

É suspeito que a instrução switch não tenha caso DateUnit :: Day . Por esse motivo , o valor do dia não será adicionado ao calendário (a variável m_calendar ), embora o calendário tenha o método AddDays .

Outros casos suspeitos com outra enumeração:

  • V719 A instrução switch não cobre todos os valores da enumeração 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 109
  • V719 A instrução switch não cobre todos os valores da enumeração 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 204
  • V719 A instrução switch não cobre todos os valores da enumeração 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 276

Comparação suspeita de números reais


V550 Uma comparação precisa e ímpar: razão == limite. Provavelmente é melhor usar uma comparação com precisão 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); }

O analisador apontou a razão de expressão suspeita == limiar . Essas variáveis ​​são do tipo double e, portanto, dificilmente poderiam ser comparadas com precisão usando o operador regular igual. Além disso, o valor da variável ratio é o resultado de uma operação de divisão.

Um código como esse parece particularmente estranho em um aplicativo como o Calculator. Estou incluindo uma lista completa dos avisos desse tipo, apenas no caso de:

  • V550 Uma comparação estranha e precisa. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 752
  • V550 Uma comparação estranha e precisa: stod (roundedString)! = 0.0. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 778
  • V550 Uma comparação estranha e precisa. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 790
  • V550 Uma comparação estranha e precisa: stod (roundedString)! = 0.0. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 820
  • V550 Uma comparação precisa e ímpar: conversionTable [m_toType] .ratio == 1.0. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 980
  • V550 Uma comparação estranha e precisa: conversionTable [m_toType] .offset == 0.0. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 980
  • V550 Uma comparação estranha e precisa: returnValue! = 0. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 1000
  • V550 Uma comparação precisa e estranha: sizeToUse! = 0.0. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 270
  • V550 Uma comparação precisa e estranha: sizeToUse! = 0.0. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 289
  • V550 Uma comparação precisa e estranha: sizeToUse! = 0.0. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 308
  • V550 Uma comparação precisa e estranha: sizeToUse! = 0.0. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 327
  • V550 Uma comparação estranha e precisa: stod (stringToLocalize) == 0. Provavelmente é melhor usar uma comparação com precisão definida: fabs (A - B) <Epsilon. CalcViewModel UnitConverterViewModel.cpp 388

Sequência de função suspeita


V1020 A função saiu sem chamar a função 'TraceLogger :: GetInstance (). LogNewWindowCreationEnd'. Verifique as linhas: 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; .... }

O Diagnostic V1020 inspeciona blocos de código e procura ramificações com uma chamada de função ausente usando heurísticas.

O trecho acima contém um bloco com as chamadas para as funções LogNewWindowCreationBegin e LogNewWindowCreationEnd . Isso é seguido por outro bloco em que a função LogNewWindowCreationEnd é chamada apenas se determinadas condições forem atendidas, o que parece muito suspeito.

Testes não confiáveis


V621 Considere inspecionar o operador 'for'. É possível que o loop seja executado incorretamente ou não seja executado. 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); } .... }

O analisador detectou um loop for que não é executado, o que significa que os testes também não são executados. O valor inicial do botão do contador de loop (93) é maior que o valor final (0) desde o início.

V760 Foram encontrados dois blocos de texto idênticos. O segundo bloco começa na linha 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); }

Outro teste suspeito. O analisador detectou dois fragmentos de código idênticos sendo executados imediatamente um após o outro. Parece que esse código foi escrito usando a técnica de copiar e colar e o programador esqueceu de modificar as cópias.

V601 O valor 'false' é convertido implicitamente no tipo inteiro. Inspecione o 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); .... }

A função ToRational é chamada com o valor booleano false , enquanto o parâmetro correspondente é do tipo int32_t e é chamado de precisão .

Decidi rastrear o valor do código e vi que ele era passado para a função StringToRat :

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

e depois para StringToNumber :

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

Aqui está o corpo da função de destino:

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

A variável de precisão agora é denominada iniciando e está participando da expressão cdigits> iniciando , o que é muito suspeito porque false foi passado como o valor original.

Redundância


V560 Uma parte da expressão condicional é sempre verdadeira: 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) { .... } .... } .... }

A variável op já foi comparada com o valor NumbersAndOperatorsEnum :: None , portanto, a verificação duplicada pode ser removida.

V728 Uma verificação excessiva pode ser simplificada. O '(A && B) || (! A &&! B) 'expressão é equivalente à expressão' bool (A) == bool (B) '. Calculadora 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); } } }

Essa enorme expressão condicional tinha originalmente 218 caracteres, mas eu a dividi em várias linhas para fins de demonstração. Ele pode ser reescrito em uma versão muito mais curta e, mais importante, mais clara:

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

V524 É estranho que o corpo da função 'ConvertBack' seja totalmente equivalente ao corpo da função '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; }

O analisador detectou duas funções implementadas de forma idêntica. Como seus nomes, Convert e ConvertBack , sugerem, eles foram feitos para fazer coisas diferentes, mas os desenvolvedores deveriam saber melhor.

Conclusão


Eu acho que todos os projetos da Microsoft que foram feitos de código aberto nos deram a oportunidade de mostrar a importância da análise estática - mesmo em projetos tão pequenos quanto a Calculadora. Grandes empresas, como Microsoft, Google, Amazon etc., empregam muitos desenvolvedores talentosos, mas ainda são humanos cometendo erros. As ferramentas de análise estática são um dos melhores meios para ajudar qualquer equipe de desenvolvedores a melhorar a qualidade de seus produtos.

Bem-vindo ao baixar o PVS-Studio e tente por conta própria “Calculadora”. :-)

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

More articles:


All Articles