Geekly Articles each Day
📜 ⬆️ ⬇️

Calculate bugs in the calculator Windows


Recently, Microsoft has opened the source code of the calculator. This application is included in all versions of the Windows operating system. The source code of various Microsoft projects has often 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, however, static code analysis using PVS-Studio revealed suspicious places in the code.

Introduction


Windows calculator is probably familiar to every user of this operating system and does not require any special presentation. Now, any user can explore the source code of the calculator on GitHub and suggest improvements.

The public, for example, has already attracted the attention of 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 suspicious code examples will be many.
')
We checked the source code of the calculator with the PVS-Studio static analyzer. Since the code is written in non-standard C ++, many regular readers of the analyzer blog doubted 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 that would prevent 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 through the analyzer reports, sorting them by ascending diagnostic numbers and a warning to this code was the very first in the list and very successful.

The fact is that the lines are incorrectly compared. It was a comparison of pointers instead of string values. Pointers are always unequal and therefore the condition is always false. To correctly compare strings, it is recommended to use, for example, the wcscmp () function.

Memory leak in native code


V773 The function of 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; }

We see a pointer temp, referring to an array of 100 elements, under which the dynamic memory is allocated. Unfortunately, the memory is released in just one place of the function; in all other places there is a memory leak. It is not very big, but it is still a mistake for C ++ code.

Elusive exception


V702 Classes should be kept from std :: exception (and alike) as 'public'. 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 detected a class inherited from the class std :: exception 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 eliminates implicit type conversion.

Missed day


V719 The switch statement 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(); }

Suspiciously, the switch does not consider the case with DateUnit :: Day . Because of this, the calendar value ( m_calendar variable) does not add the value associated with the day, although the AddDays () method is present in the calendar.

A few more suspicious places with a different listing:


Suspicious comparison of real numbers


V550 An odd precise comparison: ratio == threshold. It is probably better to use: 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 to the suspicious expression ratio == threshold . These double variables and an accurate comparison of such variables with a simple equality operator is 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:


Suspicious sequence of functions


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 blocks of code and heuristically tries to find branches of code in which the function call is forgotten.

In the above code snippet, the analyzer found a code block with a call to 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 is possible that you will not 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 detected a for loop in which no iteration is performed, and therefore no tests are executed. The initial value of the loop 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 a suspicious code. The analyzer detected identical code fragments running one after the other. Perhaps the code was written by copying these fragments, but the programmer forgot to change part of the code.

V601 The ' fake ' value is an 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. Then 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:

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

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

Redundancy


V560 A part of the 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 variable op has already been compared with the value of NumbersAndOperatorsEnum :: None and duplicate checks can be removed.

V728 Anonymous 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 originally had a width of 218 characters, but I broke it up into several lines to demonstrate a 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. 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 detected two functions that are implemented in the same way. The names of the functions Convert () and ConvertBack () suggest that they should perform different actions, but the developers know better.

Conclusion


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

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

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

More articles:


All Articles