Counting bugs in the Windows calculator


    The other day, Microsoft has opened the source code of the calculator. This application was included in all versions of the Windows operating system. The source code of various Microsoft projects has often been opened in recent years, but the news about the calculator on the very first day leaked even to non-technological media. Well, this is a popular but very small C ++ program. Nevertheless, the static analysis of the code using PVS-Studio revealed suspicious places in the project.

    Introduction


    The Windows Calculator is probably familiar to every user of this operating system and does not require a special presentation. Now, any user can study the source code of the calculator on GitHub and offer their improvements.

    The public, for example, has already paid attention to such a function:
    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);
    }

    which logs text from the clipboard and possibly sends it to Microsoft servers. But this note is not about that. Although there will be many suspicious code examples.

    We checked the source code of the calculator using the PVS-Studio static analyzer . Since the code is written in non-standard C ++, many regular readers of the analyzer’s blog questioned the possibility of analysis, but this turned out to be possible. C ++ / CLI and C ++ / CX are supported by the analyzer. Some diagnostics gave false warnings because of this, but nothing critical happened, which would have prevented using this tool.

    Perhaps you missed the news about other features of PVS-Studio, so I want to remind you that in addition to projects in C and C ++, you can analyze the code in C # and Java.

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

    I look at the analyzer reports, sorting them by ascending diagnostic numbers, and the warning for this code was the very first in the list, and very successful.

    The fact is that strings are not compared correctly here. The result was a comparison of pointers instead of string values. The address of the character array is compared with the address of the string literal. Pointers are always unequal, so the condition is always false. For proper string comparisons, you should use, for example, the wcscmp function .

    By the way, while I am writing this article, in the header file the array of characters m_resolvedName has turned into a full-fledged string like std :: wstring. And now the comparison is working correctly. By the time you read this article, most likely many other errors will also be fixed thanks to enthusiasts and research 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 = newwchar_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;
    }

    We see a temp pointer that refers to an array of 100 elements, under which dynamic memory is allocated. Unfortunately, memory is freed in only one place of the function; in all other places, a memory leak occurs. It is not very big, but it is still an error for 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
    classCalcException :std::exception
    {
    public:
      CalcException(HRESULT hr)
      {
        m_hr = hr;
      }
      HRESULT GetException(){
        return m_hr;
      }
    private:
      HRESULT m_hr;
    };

    The analyzer detected a class inherited from the std :: exception class through the private modifier (the default modifier if nothing is specified). The problem with this code is that when you try to catch the general exception std :: exception, an exception of type CalcException will be skipped. This behavior occurs because private inheritance precludes implicit type conversion.

    Missed day


    V719 The switch statement does not cover all values ​​of the 'DateUnit' enum: Day. CalcViewModel DateCalculator.cpp 279
    publicenumclass _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 is suspicious that the case of DateUnit :: Day was not considered in switch . Because of this , the value associated with the day is not added to the calendar ( m_calendar variable ), although the AddDays method is present on the calendar.

    A few more suspicious places with a different listing:
    • 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 indicated a suspicious expression ratio == threshold . These double variables and the exact comparison of such variables with a simple equality operator are hardly possible. Moreover, the value of the ratio variable is obtained after the division operation.

    This is a very strange code for the Calculator application. Therefore, I attach the entire list of warnings of this type:
    • 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;
      ....
    }

    Diagnostics V1020 analyzes code blocks and heuristically tries to find branches in which a function call is forgotten.

    In the above code snippet, the analyzer found a block calling the LogNewWindowCreationBegin and LogNewWindowCreationEnd functions . Below he found a similar block of code in which the LogNewWindowCreationEnd function is called only under certain conditions, which is 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
    publicenumclassNumbersAndOperatorsEnum
    {
      ....
      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 detected a for loop in which no iteration is performed, and, therefore, tests are not executed. The initial value of the cycle counter button (93) immediately exceeds the final (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);
    }

    Another test with suspicious code. The analyzer detected identical pieces of code that are running one after another. Perhaps the code was written by copying these fragments, but the programmer forgot to change part of the code.

    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 Boolean value false is passed to the ToRational function , although the parameter is of type int32_t and is called precision . I decided to track the value used in the code. Next, it is passed to the StringToRat function :


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

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

    And here is the body of the desired function:
    boolstripzeroesnum(_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;
      }
      ....
    }

    Here we see that the precision variable began to be called starting and participates in the expression cdigits> starting , which is very strange, because false was initially passed there .

    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 has already been compared with the value NumbersAndOperatorsEnum :: None and duplicate checks 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 giant conditional expression was originally 218 characters wide, but I split it into several lines to demonstrate the warning. And you can rewrite the code to such a short and, most importantly, readable 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 parameterauto 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 parameterauto boxedBool = dynamic_cast<Box<bool>^>(value);
        auto boolValue = (boxedBool != nullptr && boxedBool->Value);
        return !boolValue;
    }

    The analyzer detected two functions that are implemented identically. By the names of the Convert and ConvertBack functions , we can assume that they should perform different actions, but the developers are more aware.

    Conclusion


    Probably every open source project from Microsoft gave us the opportunity to show the importance of applying the static analysis methodology. Even on small projects like a calculator. Large companies such as Microsoft, Google, Amazon and others have many talented programmers, but they are the same people who make mistakes in the code. Using static code analysis tools is one of the good ways to improve the quality of programs in any development team.

    Check your “Calculator” by downloading PVS-Studio and trying on your project. :-)



    If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Counting Bugs in Windows Calculator

    Also popular now: