На днях компания Microsoft открыла исходный код калькулятора. Это приложение входило во все версии операционной системы Windows. Исходный код разных проектов Microsoft достаточно часто открывался за последние годы, но новость о калькуляторе в первый же день просочилась даже в нетехнологические средства массовой информации. Что ж, это популярная, но очень маленькая программа на языке 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. Так как код написан на нестандартном C++, многие постоянные читатели блога анализатора усомнились в возможности анализа, но это оказалось возможным. C++/CLI и C++/CX поддерживаются анализатором. Некоторые диагностики выдали ложные предупреждения из-за этого, но ничего критичного не произошло, что помешало бы воспользоваться этим инструментом.
Обзоры ошибок в других проектах:
Возможно, вы пропустили новости и о других возможностях PVS-Studio, поэтому хочу напомнить, что кроме проектов на языках C и C++, можно проанализировать код и на языках C# и Java.
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());
}
....
}Я просматриваю отчёты анализатора, сортируя их по возрастанию номеров диагностик, и предупреждение на этот код было самым первым в списке, и очень удачным.
Дело в том, что здесь неправильно сравниваются строки. Получилось сравнение указателей вместо значений строк. Сравнивается адрес массива символов с адресом строкового литерала. Указатели всегда неравны, поэтому условие всегда ложно. Для правильного сравнения строк следует использовать, например, функцию wcscmp.
Кстати, пока я пишу эту статью, в заголовочном файле массив символов m_resolvedName превратился в полноценную строку типа std::wstring. И теперь сравнение работает правильно. К моменту, когда вы будете читать эту статью, скорее всего, многие другие ошибки тоже будут исправлены благодаря энтузиастам и таким исследованиям, как это.
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;
}Мы видим указатель temp, ссылающийся на массив из 100 элементов, под который выделена динамическая память. К сожалению, память освобождается всего в одном месте функции, во всех остальных местах возникает утечка памяти. Она не очень большая, но это всё равно ошибка для C++ кода.
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;
};Анализатор обнаружил класс, унаследованный от класса std::exception через модификатор private (модификатор по умолчанию, если ничего не указано). Проблема такого кода заключается в том, что при попытке поймать общее исключение std::exception исключение типа CalcException будет пропущено. Такое поведение возникает потому, что приватное наследование исключает неявное преобразование типов.
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();
}Подозрительно, что в switch не рассмотрен случай с DateUnit::Day. Из-за этого в календарь (переменная m_calendar) не добавляется значение, связанное с днём, хотя метод AddDays у календаря присутствует.
Ещё несколько подозрительных мест с другим перечислением:
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);
}Анализатор указал на подозрительное выражение ratio == threshold. Эти переменные типа double и точное сравнение таких переменных простым оператором равенства вряд ли возможно. Тем более, что значение переменной ratio получено после операции деления.
Это очень странный код для приложения "Калькулятор". Поэтому прикладываю весь список предупреждений этого типа:
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;
....
}Диагностика V1020 анализирует блоки кода и эвристически пытается найти ветви, в которых забыт вызов функции.
В приведённом выше фрагменте кода анализатор нашёл блок с вызовом функций LogNewWindowCreationBegin и LogNewWindowCreationEnd. Ниже он нашёл похожий блок кода, в котором функция LogNewWindowCreationEnd вызывается только при определённых условиях, что подозрительно.
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);
}
....
}Анализатор обнаружил цикл for, в котором не выполняется ни одна итерация, а, следовательно, не выполняются и тесты. Начальное значение счётчика цикла button (93) сразу превышает конечное (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);
}Ещё один тест с подозрительным кодом. Анализатор обнаружил идентичные фрагменты кода, выполняющиеся друг за другом. Возможно, код писался копированием этих фрагментов, но программист забыл изменить часть кода.
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);
....
}В функцию ToRational передают булевское значение false, хотя параметр имеет тип 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 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)
{
....
}
....
}
....
}Переменная op уже сравнивалась со значением NumbersAndOperatorsEnum::None и дублирующую проверку можно удалить.
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);
}
}
}Это гигантское условное выражение изначально имело ширину 218 символов, но я разбил его на несколько строк для демонстрации предупреждения. А переписать код можно до такого короткого и, главное, читабельного варианта:
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;
}Анализатор обнаружил две функции, которые реализованы одинаково. По названиям функций Convert и ConvertBack можно предположить, что они должны выполнять разные действия, но разработчикам виднее.
Наверное, каждый открытый проект от Microsoft давал нам возможность показать важность применения методологии статического анализа. Даже на таких маленьких проектах, как калькулятор. В таких крупных компаниях, как Microsoft, Google, Amazon и других, работает много талантливых программистов, но они такие же люди, которые делают ошибки в коде. Применение инструментов статического анализа кода — один из хороших способов повысить качество программ в любых командах разработчиков.
Проверь свой "Калькулятор", скачав PVS-Studio и попробовав на своём проекте. :-)