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

суббота, 9 марта 2019 г.

Подсчитаем баги в калькуляторе 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().

Утечка памяти в нативном коде

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)
{
  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 давал нам возможность показать важность применения методологии статического анализа. Даже на таких маленьких проектах, как калькулятор. В таких крупных компаниях, как Micfosoft, Google, Amazon и других, работает много талантливых программистов, но они такие же люди, которые делают ошибки в коде. Применение инструментов статического анализа кода — один из хороших способов повысить качество программ в любых командах разработчиков.

Let's block ads! (Why?)

Парсер Хабра

...

суббота, 9 марта 2019 г.

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

Введение

Про неправильное сравнение строк

Утечка памяти в нативном коде

Неуловимое исключение

Пропущенный день

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

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

Ненадёжные тесты

Избыточность

Заключение

Комментариев нет:

Отправить комментарий

...

суббота, 9 марта 2019 г.

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

Введение

Про неправильное сравнение строк

Утечка памяти в нативном коде

Неуловимое исключение

Пропущенный день

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

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

Ненадёжные тесты

Избыточность

Заключение

Комментариев нет:

Отправить комментарий

суббота, 9 марта 2019 г.