Geekly articles weekly

计算Windows计算器中的错误


几天前，Microsoft公开提供了其Windows计算器的源代码。 计算器是传统上随每个Windows版本一起提供的应用程序。 近年来，许多Microsoft项目都变成了开源项目，但是这次新闻甚至在第一天就被非IT媒体报道。 好吧，它是C ++中一个流行但很小的程序。 尽管大小很大，但我们仍然可以使用PVS-Studio静态分析器在其代码中找到许多可疑片段。

引言


我认为我们不需要引入计算器，因为您几乎找不到不知道它是什么的Windows用户。 现在，任何人都可以从GitHub下载该应用程序的源代码并提出改进建议。

例如，以下功能已经引起了社区的关注：

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静态分析器检查Calculator的源代码。 由于它不是用标准C ++编写的，因此许多普通读者都怀疑是否可以进行这种检查，但是我们做到了。 该分析仪支持C ++ / CLI和C ++ / CX，尽管某些诊断确实产生了一些误报，但我们并未遇到任何会阻碍PVS-Studio工作的重大问题。

提醒您，如果您错过了有关我们工具其他功能的消息，PVS-Studio不仅支持C和C ++，还支持C＃和Java。

字符串比较不正确


V547表达式'm_resolvedName == L“ en-US”'始终为false。 要比较字符串，您应该使用wcscmp（）函数。 计算器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在不释放“ temp”指针的情况下退出了该功能。 可能发生内存泄漏。 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; }

临时指针是指动态分配的包含100个元素的数组。 不幸的是，内存仅在该功能的一部分中释放，而其余所有最终都会导致内存泄漏。 还算不错，但仍然被认为是C ++代码中的错误。

难以捉摸的例外


V702类应始终从std :: exception（类似）作为“ public”派生（未指定关键字，因此编译器默认将其设置为“ 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; };

分析器已使用private修饰符（如果未指定其他修饰符，则为默认值）检测到从std :: exception类派生的类。 此代码的问题在于，当尝试捕获通用std ::异常时，处理程序将忽略CalcException类型的异常，因为私有继承禁止隐式类型转换。

错过的日子


V719 switch语句未涵盖'DateUnit'枚举的所有值：天。 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大小写，这很可疑。 因此，尽管日历确实具有AddDays方法，但日期值不会添加到日历中（ m_calendar变量）。

其他可疑案例，另加枚举：

  • V719 switch语句未涵盖'eANGLE_TYPE'枚举的所有值：ANGLE_RAD。 CalcManager trans.cpp 109
  • V719 switch语句未涵盖'eANGLE_TYPE'枚举的所有值：ANGLE_RAD。 CalcManager trans.cpp 204
  • V719 switch语句未涵盖'eANGLE_TYPE'枚举的所有值：ANGLE_RAD。 CalcManager trans.cpp 276

可疑的实数比较


V550奇怪的精确比较：比率==阈值。 最好使用定义精度的比较：晶圆厂（A-B）<Epsilon。 计算器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); }

分析人员指出可疑表达率==阈值 。 这些变量的类型为double ，因此很难使用常规的等于运算符进行精确比较。 此外， 比率变量的值是除法运算的结果。

这样的代码在计算器之类的应用程序中看起来特别奇怪。 我提供了此类警告的完整列表，以防万一：

  • V550奇怪的精确比较。 最好使用定义精度的比较：晶圆厂（A-B）<Epsilon。 CalcManager UnitConverter.cpp 752
  • V550一个奇怪的精确比较：stod（roundedString）！= 0.0。 使用定义精度的比较可能更好：晶圆厂（A-B）> Epsilon。 CalcManager UnitConverter.cpp 778
  • V550奇怪的精确比较。 最好使用定义精度的比较：晶圆厂（A-B）<Epsilon。 CalcManager UnitConverter.cpp 790
  • V550一个奇怪的精确比较：stod（roundedString）！= 0.0。 使用定义精度的比较可能更好：晶圆厂（A-B）> Epsilon。 CalcManager UnitConverter.cpp 820
  • V550一个奇怪的精确比较：conversionTable [m_toType] .ratio == 1.0。 最好使用定义精度的比较：晶圆厂（A-B）<Epsilon。 CalcManager UnitConverter.cpp 980
  • V550一个奇怪的精确比较：conversionTable [m_toType] .offset == 0.0。 最好使用定义精度的比较：晶圆厂（A-B）<Epsilon。 CalcManager UnitConverter.cpp 980
  • V550一个奇怪的精确比较：returnValue！=0。最好使用定义精度的比较：fab（A-B）> Epsilon。 CalcManager UnitConverter.cpp 1000
  • V550一个奇怪的精确比较：sizeToUse！= 0.0。 使用定义精度的比较可能更好：晶圆厂（A-B）> Epsilon。 CalcViewModel LocalizationService.cpp 270
  • V550一个奇怪的精确比较：sizeToUse！= 0.0。 使用定义精度的比较可能更好：晶圆厂（A-B）> Epsilon。 CalcViewModel LocalizationService.cpp 289
  • V550一个奇怪的精确比较：sizeToUse！= 0.0。 使用定义精度的比较可能更好：晶圆厂（A-B）> Epsilon。 CalcViewModel LocalizationService.cpp 308
  • V550一个奇怪的精确比较：sizeToUse！= 0.0。 使用定义精度的比较可能更好：晶圆厂（A-B）> Epsilon。 CalcViewModel LocalizationService.cpp 327
  • V550一个奇怪的精确比较：stod（stringToLocalize）==0。使用具有定义的精确度的比较可能更好：fab（A-B）<Epsilon。 CalcViewModel UnitConverterViewModel.cpp 388

可疑功能顺序


V1020该函数退出而未调用'TraceLogger :: GetInstance（）。LogNewWindowCreationEnd'函数。 检查行：396，375。计算器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检查代码块，并使用启发式方法查找功能缺失的分支。

上面的代码段包含一个块，其中包含对LogNewWindowCreationBegin和LogNewWindowCreationEnd函数的调用。 这之后是另一个块，其中仅在满足某些条件时才调用LogNewWindowCreationEnd函数，这看起来非常可疑。

测试不可靠


V621考虑检查“ for”操作员。 循环有可能执行不正确或根本不执行。 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循环，这意味着测试也不会执行。 循环计数器按钮 （93）的初始值从一开始就大于最终值（0）。

V760找到两个相同的文本块。 第二个块从第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'false '值隐式转换为整数类型。 检查第二个参数。 计算器单元测试CalcInputTest.cpp 352

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

使用布尔值false调用ToRational函数，而相应的参数的类型为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条件表达式的一部分始终为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过度检查可以简化。 '（A && B）|| （！A &&！B）'表达式等同于'bool（A）== bool（B）'表达式。 计算器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 “ ConvertBack”功能的主体完全等同于“ Convert”功能的主体，这很奇怪。 计算器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项目都是开源的，这给了我们展示静态分析重要性的机会，即使在像计算器这样小的项目上也是如此。 微软，谷歌，亚马逊等大公司雇用了许多有才华的开发人员，但他们仍然是犯错误的人。 静态分析工具是帮助任何开发团队提高产品质量的最佳手段之一。

欢迎下载PVS-Studio并在您自己的“计算器”上进行尝试。 :-)

Source: https://habr.com/ru/post/zh-CN443400/

More articles:


All Articles