Подсчитаем баги в калькуляторе Windows

На днях компания 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 у календаря присутствует.

Ещё несколько подозрительных мест с другим перечислением:

• 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

Подозрительные сравнение вещественных чисел

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 получено после операции деления.

Это очень странный код для приложения «Калькулятор». Поэтому прикладываю весь список предупреждений этого типа:

• 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

Подозрительная последовательность функций

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 и попробовав на своём проекте. :-)

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Counting Bugs in Windows Calculator