Introduction

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

Incorrect string comparison

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

Memory leak in native code

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

Elusive exception

class CalcException : std::exception { public: CalcException(HRESULT hr) { m_hr = hr; } HRESULT GetException() { return m_hr; } private: HRESULT m_hr; };

Missed day

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

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

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

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

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

Unreliable tests

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

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

Rational CalcInput::ToRational(uint32_t radix, int32_t precision) { .... } TEST_METHOD(ToRational) { .... auto rat = m_calcInput.ToRational(10, false); .... }

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

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

Redundancy

void UnitConverterViewModel::OnPaste(String^ stringToPaste, ViewMode mode) { .... NumbersAndOperatorsEnum op = MapCharacterToButtonId(*it, canSendNegate); if (NumbersAndOperatorsEnum::None != op) // <= { .... if (NumbersAndOperatorsEnum::None != op && // <= NumbersAndOperatorsEnum::Negate != op) { .... } .... } .... }

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

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

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

Conclusion

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.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: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. V547 Expression 'm_resolvedName == L«en-US»' is always false. To compare strings you should use wcscmp() function. Calculator LocalizationSettings.h 180When 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, for instance.By the way, while I was writing this article, the character arraywas fixed in the header file and became a full-blown string of type, 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. V773 The function was exited without releasing the 'temp' pointer. A memory leak is possible. CalcViewModel StandardCalculatorViewModel.cpp 529Thepointer 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. 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 4The analyzer has detected a class derived from theclass using themodifier (which is default if no other modifiers are specified). The problem with this code is that the handler will ignore the exception of typewhen trying to catch a genericsince private inheritance forbids implicit type conversion. V719 The switch statement does not cover all values of the 'DateUnit' enum: Day. CalcViewModel DateCalculator.cpp 279It's suspicious that the switch statement has nocase. Because of that, the day value won't be added to the calendar (thevariable), although the calendar does have themethod.Other suspicious cases with another enumeration: 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 80The analyzer pointed out the suspicious expression. These variables are of typeand, therefore, could hardly be compared precisely using the regular equal operator. Besides, the value of thevariable 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: V1020 The function exited without calling the 'TraceLogger::GetInstance().LogNewWindowCreationEnd' function. Check lines: 396, 375. Calculator App.xaml.cpp 396Diagnostic 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 functionsand. This is followed by another block where thefunction is called only if certain conditions are met, which looks very suspicious. 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 500The analyzer has detected aloop that doesn't run at all, which means the tests don't execute either. The initial value of the loop counter(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 683Another 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 352Thefunction is called with the Boolean value, while the corresponding parameter is of typeand is calledI decided to track the value down the code and saw that it was then passed to thefunction:and then toHere's the body of the target function:Thevariable is now namedand is participating in the expression, which is very suspicious becausewas passed as the original value. V560 A part of conditional expression is always true: NumbersAndOperatorsEnum::None != op. CalcViewModel UnitConverterViewModel.cpp 991Thevariable was already compared with the value, 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 239This 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: V524 It is odd that the body of 'ConvertBack' function is fully equivalent to the body of 'Convert' function. Calculator BooleanNegationConverter.cpp 24The analyzer has detected two identically implemented functions. As their names,and, suggest, they were meant to do different things, but the developers should know better.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». :-)