Geekly articles weekly

Contando errores en la calculadora de Windows


El otro día, Microsoft abrió el código fuente de la calculadora. Esta aplicación se incluyó en todas las versiones del sistema operativo Windows. El código fuente de varios proyectos de Microsoft a menudo se ha abierto en los últimos años, pero las noticias sobre la calculadora el primer día se filtraron incluso a medios no tecnológicos. Bueno, este es un programa C ++ popular pero muy pequeño. Sin embargo, el análisis de código estático usando PVS-Studio reveló lugares sospechosos en el proyecto.

Introduccion


La Calculadora de Windows probablemente sea familiar para todos los usuarios de este sistema operativo y no requiere una presentación especial. Ahora, cualquier usuario puede estudiar el código fuente de la calculadora en GitHub y ofrecer sus mejoras.

El público, por ejemplo, ya ha prestado atención a dicha función:
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/443100/

More articles:


All Articles