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

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 являются новыми и поэтому, позволили найти что-то новое и интересное. Если вы проверяли свой проект год назад, это ничего не значит. Вообще ничего. Вы написали новый непроверенный код. В анализаторе появились новые диагностические возможности. И продолжают появляться каждый месяц. Начните использовать статический анализ регулярно, и вы существенно сократите усилия на поиск и устранения многих ошибок.

#Cpp #StaticAnalysis