Geekly articles weekly

Contando bugs na calculadora do Windows


No outro dia, a Microsoft abriu o código fonte da calculadora. Este aplicativo foi incluído em todas as versões do sistema operacional Windows. O código fonte de vários projetos da Microsoft foi aberto com bastante frequência nos últimos anos, mas as notícias sobre a calculadora vazaram no primeiro dia, mesmo para mídias não tecnológicas. Bem, este é um programa C ++ popular, mas muito pequeno. No entanto, a análise de código estático usando o PVS-Studio revelou lugares suspeitos no projeto.

1. Introdução


A Calculadora do Windows provavelmente é familiar para todos os usuários deste sistema operacional e não requer uma apresentação especial. Agora, qualquer usuário pode estudar o código fonte da calculadora no GitHub e oferecer suas melhorias.

O público, por exemplo, já prestou atenção a essa função:
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);
}

, , Microsoft. . .

PVS-Studio. C++, , . C++/CLI C++/CX . - , , .

, PVS-Studio, , C C++, C# Java.


V547 Expression 'm_resolvedName == L«en-US»' is always false. To compare strings you should use wcscmp() function. 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());
  }
  ....
}

, , , .

, . . . , . , , wcscmp.

, , m_resolvedName std::wstring. . , , , , .


V773 The function was exited without releasing the 'temp' pointer. A memory leak is 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;
}

temp, 100 , . , , . , C++ .


V702 Classes should always be derived from std::exception (and alike) as 'public' (no keyword was specified, so compiler defaults it to '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;
};

, std::exception private ( , ). , std::exception CalcException . , .


V719 The switch statement does not cover all values of the 'DateUnit' enum: 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();
}

, switch DateUnit::Day. - ( m_calendar) , , AddDays .

:
  • V719 The switch statement does not cover all values of the 'eANGLE_TYPE' enum: ANGLE_RAD. CalcManager trans.cpp 109
  • V719 The switch statement does not cover all values of the 'eANGLE_TYPE' enum: ANGLE_RAD. CalcManager trans.cpp 204
  • V719 The switch statement does not cover all values of the 'eANGLE_TYPE' enum: ANGLE_RAD. CalcManager trans.cpp 276


V550 An odd precise comparison: ratio == threshold. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. Calculator 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);
}

ratio == threshold. double . , ratio .

«». :
  • V550 An odd precise comparison. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. CalcManager UnitConverter.cpp 752
  • V550 An odd precise comparison: stod(roundedString) != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcManager UnitConverter.cpp 778
  • V550 An odd precise comparison. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. CalcManager UnitConverter.cpp 790
  • V550 An odd precise comparison: stod(roundedString) != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcManager UnitConverter.cpp 820
  • V550 An odd precise comparison: conversionTable[m_toType].ratio == 1.0. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. CalcManager UnitConverter.cpp 980
  • V550 An odd precise comparison: conversionTable[m_toType].offset == 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. CalcManager UnitConverter.cpp 980
  • V550 An odd precise comparison: returnValue != 0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcManager UnitConverter.cpp 1000
  • V550 An odd precise comparison: sizeToUse != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcViewModel LocalizationService.cpp 270
  • V550 An odd precise comparison: sizeToUse != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcViewModel LocalizationService.cpp 289
  • V550 An odd precise comparison: sizeToUse != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcViewModel LocalizationService.cpp 308
  • V550 An odd precise comparison: sizeToUse != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcViewModel LocalizationService.cpp 327
  • V550 An odd precise comparison: stod(stringToLocalize) == 0. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. CalcViewModel UnitConverterViewModel.cpp 388


V1020 The function exited without calling the 'TraceLogger::GetInstance().LogNewWindowCreationEnd' function. Check lines: 396, 375. Calculator 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;
  ....
}

V1020 , .

LogNewWindowCreationBegin LogNewWindowCreationEnd. , LogNewWindowCreationEnd , .


V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. 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);
  }
  ....
}

for, , , , . button (93) (0).

V760 Two identical blocks of text were found. The second block begins from line 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);
}

. , . , , .

V601 The 'false' value is implicitly cast to the integer type. Inspect the second argument. CalculatorUnitTests CalcInputTest.cpp 352
Rational CalcInput::ToRational(uint32_t radix, int32_t precision) { .... }

TEST_METHOD(ToRational)
{
  ....
  auto rat = m_calcInput.ToRational(10, false);
  ....
}

ToRational false, int32_t precision.

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

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

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

, precision starting cdigits > starting, , false.


V560 A part of conditional expression is always true: 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)
    {
      ....
    }
    ....
  }
  ....
}

op NumbersAndOperatorsEnum::None .

V728 An excessive check can be simplified. The '(A && B) || (!A && !B)' expression is equivalent to the 'bool(A) == bool(B)' expression. Calculator 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);
    }
  }
}

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

V524 It is odd that the body of 'ConvertBack' function is fully equivalent to the body of 'Convert' function. 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;
}

, . Convert ConvertBack , , .


, Microsoft . , . , Microsoft, Google, Amazon , , , . — .

«», PVS-Studio . :-)



, : Svyatoslav Razmyslov. Counting Bugs in Windows Calculator

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

More articles:


All Articles