Counting Bugs in Windows Calculator

A few days ago, Microsoft made the source code of their Windows Calculator publicly available. Calculator is an application that has traditionally shipped with every Windows version. A number of Microsoft projects went open-source over the recent years, but this time the news was covered even by non-IT media on the very first day. Well, it's a popular yet tiny program in C++. Despite its size, we still managed to find a number of suspicious fragments in its code using the PVS-Studio static analyzer.

Introduction

I don't think we need to introduce Calculator as you would hardly find a Windows user who doesn't know what it is. Now anyone can download the app's source code from GitHub and suggest their improvements.

The following function, for example, already attracted the community's attention:

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

This function logs text from the clipboard and apparently sends it off to Microsoft servers. This post, however, isn't about that function, but you'll see lots of suspicious snippets for sure.

We used the PVS-Studio static analyzer to check Calculator's source code. Since it isn't written in standard C++, many of our regular readers doubted such a check would be possible, but we did it. The analyzer supports C++/CLI and C++/CX, and even though some diagnostics did produce a few false positives, we didn't encounter any critical problems that would hinder the work of PVS-Studio.

Just as a reminder, in case you missed the news about other capabilities of our tool, PVS-Studio supports not only C and C++ but C# and Java as well.

Incorrect string comparison

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

When viewing analyzer reports, I sort warnings by diagnostic code in ascending order, and this one, which makes quite a vivid example, happened to be the first on the list.

You see, the example above shows incorrect comparison of strings. The programmer is in fact comparing pointers instead of string values by comparing the address of a character array with that of a string literal. These pointers are never equal, so the condition is always false, too. For correct comparison of strings, one should use the function wcscmp, for instance.

By the way, while I was writing this article, the character array m_resolvedName was fixed in the header file and became a full-blown string of type std::wstring, so the comparison can be done properly now. By the moment you will be reading this article, many other bugs will be probably fixed too thanks to the enthusiasts and reviews like this.

Memory leak in native code

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

The temp pointer refers to a dynamically allocated array of 100 elements. Unfortunately, the memory is released only in one part of the function, while all the rest end up with a memory leak. It's not too bad, but it's still considered a bug in C++ code.

Elusive exception

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

The analyzer has detected a class derived from the std::exception class using the private modifier (which is default if no other modifiers are specified). The problem with this code is that the handler will ignore the exception of type CalcException when trying to catch a generic std::exception since private inheritance forbids implicit type conversion.

Missed day

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

It's suspicious that the switch statement has no DateUnit::Day case. Because of that, the day value won't be added to the calendar (the m_calendar variable), although the calendar does have the AddDays method.

Other suspicious cases with another enumeration:

• 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

Suspicious comparison of real numbers

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

The analyzer pointed out the suspicious expression ratio == threshold. These variables are of type double and, therefore, could hardly be compared precisely using the regular equal operator. Besides, the value of the ratio variable is the result of a division operation.

Code like that looks particularly strange in an application like Calculator. I'm including a complete list of the warnings of this type just in case:

• 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

Suspicious function sequence

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

Diagnostic V1020 inspects code blocks and looks for branches with a missing function call using heuristics.

The snippet above contains a block with the calls to functions LogNewWindowCreationBegin and LogNewWindowCreationEnd. This is followed by another block where the LogNewWindowCreationEnd function is called only if certain conditions are met, which looks very suspicious.

Unreliable tests

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

The analyzer has detected a for loop that doesn't run at all, which means the tests don't execute either. The initial value of the loop counter button (93) is greater than the final value (0) right from the start.

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

Another suspicious test. The analyzer has detected two identical code fragments executing immediately one after the other. It looks like this code was written using the copy-paste technique and the programmer forgot to modify the copies.

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

The ToRational function is called with the Boolean value false, while the corresponding parameter is of type int32_t and is called precision.

I decided to track the value down the code and saw that it was then passed to the StringToRat function:

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

and then to StringToNumber:

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

Here's the body of the target function:

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

The precision variable is now named starting and is participating in the expression cdigits > starting, which is very suspicious because false was passed as the original value.

Redundancy

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)
    {
      ....
    }
    ....
  }
  ....
}

The op variable was already compared with the value NumbersAndOperatorsEnum::None, so the duplicate check can be removed.

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

This huge conditional expression was originally 218 characters long, but I split it into several lines for the purpose of demonstration. It can be rewritten into a much shorter and, most importantly, clearer version:

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

The analyzer has detected two identically implemented functions. As their names, Convert and ConvertBack, suggest, they were meant to do different things, but the developers should know better.

Conclusion

I guess every Microsoft project that was made open-source gave us an opportunity to show the importance of static analysis — even on projects as tiny as Calculator. Big companies, such as Microsoft, Google, Amazon, etc., employ lots of talented developers, but they are still humans making mistakes. Static analysis tools are one of the best means to help any developer team improve the quality of their products.

Welcome to download PVS-Studio and try it on your own «Calculator». :-)

Автор: SvyatoslavMC

Источник