to the top
close form
Для получения триального ключа
заполните форму ниже
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Статический анализ следует применять ре…

Статический анализ следует применять регулярно

20 Авг 2012

Время от времени мы повторно анализируем проекты, которые уже проверяли с помощью PVS-Studio. Причин несколько. Например, хочется посмотреть, удалось ли избавиться от определенных ложных срабатываний. Но самое интересное, это посмотреть, как работают новые диагностические правила и какие ошибки они находят. Очень интересно наблюдать, как в уже казалось бы очищенном проекте находятся всё новые и новые дефекты. Очередным перепроверенным проектом стал Clang.

Clang очень интересный для нас проект. Во-первых, он очень качественный. А значит найти в нём новую ошибку, это уже целое достижение. Во-вторых, на нем очень хорошо видны различные недоделки в PVS-Studio, из-за которых возникают ложные срабатывания.

К сожалению, с момента очередной проверки проекта и написанием этой заметки прошло более месяца. Причина тому мой отпуск. Есть вероятность, что описанный здесь подозрительный код к моменту публикации заметки уже будет исправлен. Но я думаю это не страшно. Главное я смогу напомнить читателям, что статический анализ это инструмент постоянного, а не разового применения.

Статический анализ следует применять регулярно, так как:

  • В проекте появляется новый код. Если его не проверить сразу, многие ошибки будут исправляться медленно. О них будет сообщать отдел тестирования или клиенты.
  • Инструменты учатся выявлять всё новые и новые паттерны ошибок.

Всё это звучит очень просто и даже банально. К сожалению, разработчики ленятся интегрировать статический анализ в процесс разработки. Вновь и вновь приходится подталкивать их к этому полезному шагу.

Предыдущий раз проект Clang мы проверяли приблизительно год назад. За это время у нас появились новые диагностические правила, которые позволили выявить очередные места с подозрительным кодом. Правда, таких мест совсем немного. Это не удивительно, так как проект Clang сам включает в себя статический анализатор и разрабатывается высококвалифицированными разработчиками. Странно, что вообще удается найти хоть что-то подозрительное.

Посмотрим, что удалось заметить интересного в коде. В основном подозрительные фрагменты связаны с операторами сдвига.

int64_t DataExtractor::getSLEB128(....) const {
  int64_t result = 0;
  ...
  // Sign bit of byte is 2nd high order bit (0x40)
  if (shift < 64 && (byte & 0x40))
    result |= -(1 << shift);
  ...
}

PVS-Studio: V629 Consider inspecting the '1 << shift' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. dataextractor.cpp 171

Судя по проверке "shift < 64", число 1 может быть сдвинуто влево на [0..63] позиций. Однако этот код может привести к неопределенному поведению (undefined behaviour). Подробнее, почему здесь возможно неопределенное поведение, можно познакомиться в статье "Не зная брода, не лезь в воду. Часть третья". Коварство подобных дефектов заключается в том, что программа может долгое время делать вид, что корректно работает. Сбои могут возникать при смене версий компилятора, при изменении ключей оптимизации и после рефакторинга кода.

Код станет безопасным, если число 1 будет представлено 64-битным беззнаковым типом данных. Тогда его можно будет смело сдвигать на 63 разряда. Безопасный код:

result |= -(1ui64 << shift);

К сожалению, я не соображу, как лучше поступить со знаком минус.

Рассмотрим другой пример, содержащий подозрительную операцию сдвига:

void EmitVBR64(uint64_t Val, unsigned NumBits) {
  if ((uint32_t)Val == Val)
    return EmitVBR((uint32_t)Val, NumBits);

  uint64_t Threshold = 1U << (NumBits-1);
  ...
}

PVS-Studio: V629 Consider inspecting the '1U << (NumBits - 1)' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. bitstreamwriter.h 173

Если аргумент 'NumBits' может быть больше 32, то функция будет работать некорректно. Как и в предыдущем примере при сдвиге '1U' на большие значения, будет возникать неопределенное поведение программы. На практике неопределенное поведение, скорее всего, будет проявлять себя как помещение в переменную 'Threshold' бессмысленных значений.

Безопасный вариант:

uint64_t Threshold = 1UI64 << (NumBits-1);

Рассмотренные выше примеры будут приводить к ошибкам, только в том случае, если сдвиг осуществляется на большое количество бит. Но есть фрагменты, которые всегда приводят к неопределенному поведению. Например, это сдвиг отрицательных чисел.

int find_next(unsigned Prev) const {
  ...
  // Mask off previous bits.
  Copy &= ~0L << BitPos;
  ...
}

PVS-Studio: V610 Undefined behavior. Check the shift operator '<<. The left operand '~0L' is negative. bitvector.h 175

Так делать небезопасно. Проект Clang собирается для различных платформ. Нужно быть аккуратным с использованием таких конструкций. Сложно предвидеть, как где-то поведёт себя сдвиг отрицательных чисел.

Есть и другие потенциально опасные операции сдвига. Они однотипны, поэтому не будем их рассматривать подробнее. Просто перечислю их расположение в коде:

  • V610 Undefined behavior. Check the shift operator '<<=. The left operand 'Val' is negative. pointerintpair.h 139
  • V610 Undefined behavior. Check the shift operator '<<. The left operand '~0L' is negative. bitvector.h 454
  • V610 Undefined behavior. Check the shift operator '<<. The left operand '~0L' is negative. sparsebitvector.h 161
  • V610 Undefined behavior. Check the shift operator '<<=. The left operand 'Val' is negative. pointerintpair.h 144
  • V610 Undefined behavior. Check the shift operator '<<=. The left operand 'Val' is negative. densemapinfo.h 35
  • V610 Undefined behavior. Check the shift operator '<<=. The left operand 'Val' is negative. densemapinfo.h 40
  • V629 Consider inspecting the '1U << (NumBits - 1)' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. bitstreamreader.h 362
  • V629 Consider inspecting the 'Bit->getValue() << i' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. record.cpp 248

Помимо подозрительных сдвигов, было найдено несколько подозрительных циклов. Дело в том, что они выполняются только один раз.

bool ObjCARCOpt::VisitBottomUp(....) {
  ...
  for (BBState::edge_iterator SI(MyStates.succ_begin()),
       SE(MyStates.succ_end()); SI != SE; ++SI)
  {
    const BasicBlock *Succ = *SI;
    DenseMap<const BasicBlock *, BBState>::iterator I =
      BBStates.find(Succ);
    assert(I != BBStates.end());
    MyStates.InitFromSucc(I->second);
    ++SI;
    for (; SI != SE; ++SI) {
      Succ = *SI;
      I = BBStates.find(Succ);
      assert(I != BBStates.end());
      MyStates.MergeSucc(I->second);
    }
    break;
  }
  ...
}

PVS-Studio: V612 An unconditional 'break' within a loop. objcarc.cpp 2763

Обратите внимание на последний оператор 'break'. Перед ним нет никакого условия, и он всегда останавливает цикл. Таким образом, выполняется только одна итерация цикла.

Аналогичные странные фрагменты кода:

  • V612 An unconditional 'break' within a loop. objcarc.cpp 2948
  • V612 An unconditional 'break' within a loop. undefinedassignmentchecker.cpp 75
  • V612 An unconditional 'break' within a loop. bugreporter.cpp 1095

Заключение

Диагностики V610, V612, V629 являются новыми и поэтому, позволили найти что-то новое и интересное. Если вы проверяли свой проект год назад, это ничего не значит. Вообще ничего. Вы написали новый непроверенный код. В анализаторе появились новые диагностические возможности. И продолжают появляться каждый месяц. Начните использовать статический анализ регулярно, и вы существенно сократите усилия на поиск и устранения многих ошибок.

Популярные статьи по теме
Под капотом SAST: как инструменты анализа кода ищут дефекты безопасности

Дата: 26 Янв 2023

Автор: Сергей Васильев

Сегодня речь о том, как SAST-решения ищут дефекты безопасности. Расскажу, как разные подходы к поиску потенциальных уязвимостей дополняют друг друга, зачем нужен каждый из них и как теория ложится на…
Межмодульный анализ C и C++ проектов в деталях. Часть 2

Дата: 14 Июл 2022

Автор: Олег Лысый

В первой части статьи мы рассматривали основы теории компиляции C и C++ проектов, в частности особое внимание уделили алгоритмам компоновки и оптимизациям. Во второй части мы погрузимся глубже и пока…
Межмодульный анализ C и C++ проектов в деталях. Часть 1

Дата: 08 Июл 2022

Автор: Олег Лысый

Начиная с PVS-Studio 7.14, для C и C++ анализатора появилась поддержка межмодульного анализа. В этой статье, которая будет состоять из двух частей, мы расскажем, как устроены похожие механизмы в комп…
Ускоряем сборку и анализ при помощи Incredibuild

Дата: 17 Май 2021

Автор: Максим Звягинцев

"Да сколько ты ещё будешь собирать?" – фраза, которую каждый разработчик произносил хотя бы раз посреди ночи. Да, сборка бывает долгой и от этого никуда не деться. Нельзя же просто так взять и распар…
GTK: как выглядит первый запуск анализатора в цифрах

Дата: 04 Янв 2021

Автор: Святослав Размыслов

Внедрение статического анализатора в проект для некоторых людей выглядит непреодолимой преградой. Почему-то очень распространено мнение, что объём выданных результатов анализа при первом запуске наст…

Комментарии (0)

Следующие комментарии next comments
close comment form
Unicorn with delicious cookie
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо