Задача коммерческих статических анализаторов выполнять более глубокий и полный анализ кода, чем компиляторы. Давайте посмотрим, что смог обнаружить PVS-Studio в исходном коде проекта LLVM 13.0.0.
Разрабатывая проект, рационально использовать минимально необходимое количество приложений. Поэтому если встроенные механизмы обеспечивают необходимый уровень анализа кода, то есть смысл ограничиться ими и не добавлять в пайплайн дополнительные утилиты.
Для нашей команды разработчиков PVS-Studio это проявляется следующим образом. Время от времени нас спрашивают, обеспечиваем ли мы анализ лучше, чем компилятор X или встроенный в этот компилятор статический анализатор? Как правило, количество таких вопросов увеличивается в момент очередного релиза компилятора.
Правильные варианты ответов на такие вопросы:
. Закрадывается впечатление, что мы хотим уйти от вопроса в сторону. Выходом является написание таких статей, как эта. Наша команда проверяет компиляторы и тем самым демонстрирует возможности продукта.
В данном случае мы проверим недавний релиз LLVM 13.0.0. Конечно, читателю и нам интересен не сам LLVM. Подразумевается, что мы оцениваем мощь PVS-Studio в сравнении с компилятором Clang, Clang Static Analyzer и Clang-Tidy. Перечисленные программы используются при разработке проекта LLVM. И если, несмотря на их использование, удастся что-то найти, это продемонстрирует пользу внедрения PVS-Studio в процесс разработки.
Если интересно, здесь можно познакомиться с предыдущей аналогичной статьёй времён LLVM 11.
Если вы хотите начать использовать анализатор регулярно, то требуется его настройка. Помимо этого, на начальном этапе будет полезно объявить все предупреждения техническим долгом и спрятать их. После чего начать работать только с новыми предупреждениями, постепенно ликвидируя технический долг. Более подробно этот подход описан здесь.
Настройке и внедрению анализатора посвящены многие другие статьи, поэтому не будем здесь удаляться от главной темы. Давайте же, наконец, посмотрим, что удалось найти интересного.
Я потратил на просмотр лога один вечер и выписал то, что показалось мне интересным. Уверен, можно найти гораздо больше ошибок. Однако даже то, что, просто бегло пробежавшись по отчёту, можно исправить 20 ошибок, демонстрирует анализатор в выгодном свете.
.
Несложно придумать идеи правил для обнаружения опечаток. А вот реализовать их куда сложнее. Нужно соблюсти баланс между полезными предупреждениями и ложно-положительными срабатываниями. В компиляторе Clang и сопутствующих анализаторах есть диагностики для выявления многих разновидностей ошибок, которые будут описаны ниже. Но раз они не помогли, значит в нашем анализаторе эти диагностики сделаны более качественно.
....
bool DWARFFormValue::extractValue(const DWARFDataExtractor &Data,
uint64_t *OffsetPtr, dwarf::FormParams FP,
const DWARFContext *Ctx,
const DWARFUnit *CU) {
....
case DW_FORM_LLVM_addrx_offset:
Value.uval = Data.getULEB128(OffsetPtr, &Err) << 32;
Value.uval = Data.getU32(OffsetPtr, &Err);
break;
....
}
Предупреждение PVS-Studio: V519 [CWE-563, CERT-MSC13-C] The 'Value.uval' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 334, 335. DWARFFormValue.cpp 335
Нет смысла в одну и ту же переменную записывать поочередно разные значения. Об этом и предупреждает анализатор. Скорее всего, автор кода немного поспешил и допустил опечатку, забыв добавить '|'. Данный код должен создавать из двух 32-битных значений одно 64-битное. Корректный вариант этого кода:
case DW_FORM_LLVM_addrx_offset:
Value.uval = Data.getULEB128(OffsetPtr, &Err) << 32;
Value.uval |= Data.getU32(OffsetPtr, &Err);
break;
class ExecutorAddress {
....
ExecutorAddress &operator++() {
++Addr;
return *this;
}
ExecutorAddress &operator--() {
--Addr;
return *this;
}
ExecutorAddress operator++(int) { return ExecutorAddress(Addr++); }
ExecutorAddress operator--(int) { return ExecutorAddress(Addr++); }
ExecutorAddress &operator+=(const ExecutorAddrDiff Delta) {
Addr += Delta.getValue();
return *this;
}
ExecutorAddress &operator-=(const ExecutorAddrDiff Delta) {
Addr -= Delta.getValue();
return *this;
}
....
private:
uint64_t Addr = 0;
}
К сожалению, за скорость написания, приходится расплачиваться высокой вероятностью забыть что-то исправить в скопированном коде. А поскольку скучно не только писать, но и проверять такой код, то он "притягивает" к себе ошибки.
К счастью, статические анализаторы не устают и не ленятся. PVS-Studio хорошо дополняет классический обзор кода, предлагая обратить внимание на эти две функции:
ExecutorAddress operator++(int) { return ExecutorAddress(Addr++); }
ExecutorAddress operator--(int) { return ExecutorAddress(Addr++); }
Предупреждение PVS-Studio: V524 It is odd that the body of '--' function is fully equivalent to the body of '++' function. ExecutorAddress.h 104
Явная ошибка: забыли заменить в правой части скопированной строки оператор ++ на --.
return OriginalValue == OriginalValue && BaseValue == Other.BaseValue &&
Status == Other.Status;
}
V501 [CWE-571] There are identical sub-expressions to the left and to the right of the '==' operator: OriginalValue == OriginalValue RewriteStatepointsForGC.cpp 758
Классика! Я даже отдельную большую статью писал на эту тему — "Зло живёт в функциях сравнения".
Чтобы снизить количество таких ошибок, рекомендую всегда оформлять однотипные операции табличным методом. Я бы оформил эту функцию так:
bool operator==(const BDVState &Other) const {
return
OriginalValue == OriginalValue
&& BaseValue == Other.BaseValue
&& Status == Other.Status;
}
Так длиннее, но зато больше вероятность, что опечатка будет замечена на code review. Впрочем, даже так ошибку можно просмотреть и лучше дополнительно подстраховаться, используя профессиональный анализатор.
bool TypeInfer::EnforceSmallerThan(TypeSetByHwMode &Small,
TypeSetByHwMode &Big) {
....
if (Small.empty())
Changed |= EnforceAny(Small);
if (Big.empty())
Changed |= EnforceAny(Big);
assert(Small.hasDefault() && Big.hasDefault());
SmallVector<unsigned, 4> Modes;
union_modes(Small, Big, Modes);
for (unsigned M : Modes) {
TypeSetByHwMode::SetType &S = Small.get(M);
TypeSetByHwMode::SetType &B = Big.get(M);
if (any_of(S, isIntegerOrPtr) && any_of(S, isIntegerOrPtr)) {
auto NotInt = [](MVT VT) { return !isIntegerOrPtr(VT); };
Changed |= berase_if(S, NotInt);
Changed |= berase_if(B, NotInt);
} else if (any_of(S, isFloatingPoint) && any_of(B, isFloatingPoint)) {
auto NotFP = [](MVT VT) { return !isFloatingPoint(VT); };
Changed |= berase_if(S, NotFP);
Changed |= berase_if(B, NotFP);
} else if (S.empty() || B.empty()) {
Changed = !S.empty() || !B.empty();
S.clear();
B.clear();
} else {
TP.error("Incompatible types");
return Changed;
}
....
}
Прежде чем я покажу, где ошибка, можете попробовать проверить свою внимательность и найти опечатку самостоятельно. Вставлю картинку, чтобы вы сразу не посмотрели ответ.
Проблема здесь:
if (any_of(S, isIntegerOrPtr) && any_of(S, isIntegerOrPtr))
Предупреждение PVS-Studio: V501 [CWE-571] There are identical sub-expressions 'any_of(S, isIntegerOrPtr)' to the left and to the right of the '&&' operator. CodeGenDAGPatterns.cpp 479
Правильно:
if (any_of(S, isIntegerOrPtr) && any_of(B, isIntegerOrPtr))
Register Dst = MI.getOperand(0).getReg();
Register Src = MI.getOperand(1).getReg();
Register Amt = MI.getOperand(2).getReg();
LLT DstTy = MRI.getType(Dst);
LLT SrcTy = MRI.getType(Dst);
LLT AmtTy = MRI.getType(Amt);
....
}
Предупреждение PVS-Studio: V656 [CWE-665] Variables 'DstTy', 'SrcTy' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'MRI.getType(Dst)' expression. Check lines: 5953, 5954. LegalizerHelper.cpp 5954
Ранее я писал, что от опечаток помогает защититься форматирование кода таблицей. Да, помогает, но гарантии не даёт. Перед нами красиво оформленный код, похожий на таблицу. Но ошибка всё равно там есть.
Скорее всего, код писался копированием. Скопировали строчку
LLT DstTy = MRI.getType(Dst);
Но только в одном месте заменили Dst на Src:
LLT SrcTy = MRI.getType(Dst);
Правильный вариант кода:
LLT DstTy = MRI.getType(Dst);
LLT SrcTy = MRI.getType(Src);
LLT AmtTy = MRI.getType(Amt);
. Есть такие места и в LLVM. Изучать предупреждения про нулевые указатели скучно и утомительно. Я просмотрел такие предупреждения очень бегло и подозреваю, что при желании таких ошибок будет выявлено намного больше.
const MCSymbol *Label) {
....
if (Label)
DD->addArangeLabel(SymbolCU(this, Label));
bool UseAddrOffsetFormOrExpressions =
DD->useAddrOffsetForm() || DD->useAddrOffsetExpressions();
const MCSymbol *Base = nullptr;
if (Label->isInSection() && UseAddrOffsetFormOrExpressions)
Base = DD->getSectionLabel(&Label->getSection());
....
}
Предупреждение PVS-Studio: V1004 [CWE-476, CERT-EXP34-C] The 'Label' pointer was used unsafely after it was verified against nullptr. Check lines: 74, 81. DwarfCompileUnit.cpp 81
Проверка "if (Label)" говорит нам и анализатору, что указатель Label может быть нулевым. Но ниже этот указатель смело разыменовывается без всякой проверки:
if (Label->isInSection() && UseAddrOffsetFormOrExpressions)
Так явно не стоит делать.
{
....
if (Pat->isLeaf()) {
DefInit *DI = dyn_cast<DefInit>(Pat->getLeafValue());
if (!DI)
I.error("Input $" + Pat->getName() + " must be an identifier!");
Rec = DI->getDef();
}
....
}
Предупреждение PVS-Studio: V1004 [CWE-476, CERT-EXP34-C] The 'DI' pointer was used unsafely after it was verified against nullptr. Check lines: 3349, 3351. CodeGenDAGPatterns.cpp 3351
Указатель DI проверятся, но далее сразу разыменовывается уже без проверки. Обосновано задаться вопросом, а является ли этой ошибкой? Ведь если указатель DI нулевой, то вызывается некая функция error, которая может генерировать исключение. Давайте заглянем в эту функцию:
void TreePattern::error(const Twine &Msg) {
if (HasError)
return;
dump();
PrintError(TheRecord->getLoc(), "In " + TheRecord->getName() + ": " + Msg);
HasError = true;
}
Нет, эта функция не генерирует исключение и не останавливает выполнение программы каким-либо способом.
Получается, что сразу после фиксации ошибочного состояния (записи в лог) произойдёт разыменование нулевого указателя.
Как минимум, есть ещё пара аналогичных ошибок, которые нет смысла рассматривать отдельно:
arse(...., raw_ostream *OS, bool Verbose) {
assert((OS || !Verbose) && "cannot have verbose output without stream");
....
auto EmitRow = [&] {
if (!TombstonedAddress) {
if (Verbose) {
*OS << "\n";
OS->indent(12);
}
if (OS)
State.Row.dump(*OS);
State.appendRowToMatrix();
}
};
....
}
Предупреждение PVS-Studio: V595 [CWE-476, CERT-EXP12-C] The 'OS' pointer was utilized before it was verified against nullptr. Check lines: 791, 793. DWARFDebugLine.cpp 791
Указатель OS может быть нулевым, о чём свидетельствует проверка "if (OS)". Однако ранее этот указатель может быть разыменован без предварительной проверки.
В начале есть assert, который защищает от нулевых указателей. Однако этого явно недостаточно, так как в релизной сборке макрос assert раскрывается в ничто.
Рационально сделать код более безопасным:
auto EmitRow = [&] {
if (!TombstonedAddress) {
if (OS)
{
if (Verbose) {
*OS << "\n";
OS->indent(12);
}
State.Row.dump(*OS);
}
State.appendRowToMatrix();
}
};
....
static Error loadObj(....) {
....
auto Kind = Extractor.getU8(&OffsetPtr);
static constexpr SledEntry::FunctionKinds Kinds[] = {
SledEntry::FunctionKinds::ENTRY, SledEntry::FunctionKinds::EXIT,
SledEntry::FunctionKinds::TAIL,
SledEntry::FunctionKinds::LOG_ARGS_ENTER,
SledEntry::FunctionKinds::CUSTOM_EVENT};
if (Kind >= sizeof(Kinds))
return errorCodeToError(
std::make_error_code(std::errc::executable_format_error));
Entry.Kind = Kinds[Kind];
....
}
Предупреждение PVS-Studio: V557 [CWE-125, CERT-ARR30-C] Array overrun is possible. The value of 'Kind' index could reach 19. InstrumentationMap.cpp 196
Текст сообщения требует пояснения. Механизм анализа потока данных обрабатывает этот код:
if (Kind >= sizeof(Kinds))
return errorCodeToError(...);
И приходит к выводу, что если условие не выполняется, то переменная Kind имеет далее значение [0..19].
Почему 19, а не 4? Я осуществлял проверку проекта с помощью плагина для Visual Studio 2019. Соответственно, анализатор знает, что использовался компилятор Visual C++ и что перечисление будет представлено четырьмя байтами. В этом можно убедиться, написав следующую тестовую программу:
int main()
{
enum class FunctionKinds { ENTRY, EXIT, TAIL, LOG_ARGS_ENTER, CUSTOM_EVENT };
static constexpr FunctionKinds Kinds[] = {
FunctionKinds::ENTRY, FunctionKinds::EXIT, FunctionKinds::TAIL,
FunctionKinds::LOG_ARGS_ENTER, FunctionKinds::CUSTOM_EVENT
};
std::cout << sizeof(Kinds) << std::endl;
return 0;
}
Собираем с помощью компилятора Visual C++, запускаем и видим число "20".
Получается, что защита от выхода за границу массива не работает. Чтобы исправить код, нужно сравнивать Kind не с размером массива в байтах, а с количеством элементом массива.
Корректная проверка:
if (Kind >= sizeof(Kinds) / sizeof(Kinds[0]))
return errorCodeToError(....);
// Opcode N U L G E Intuitive operation
SETFALSE, // 0 0 0 0 Always false (always folded)
SETOEQ, // 0 0 0 1 True if ordered and equal
....
SETCC_INVALID // Marker value.
};
static void InitCmpLibcallCCs(ISD::CondCode *CCs) {
memset(CCs, ISD::SETCC_INVALID, sizeof(ISD::CondCode)*RTLIB::UNKNOWN_LIBCALL);
....
}
Предупреждение PVS-Studio: V575 [CWE-628, CERT-EXP37-C] The 'memset' function processes the pointer to enum type. Inspect the first argument. TargetLoweringBase.cpp 662
Код будет работать только в том случае, если повезёт и элементы перечисления CondCode будут представлены одним байтом.
Функция memset заполняет массив байт. В каждый байт записывается значение SETCC_INVALID. Если enum представлен 4 байтами, как это происходит в случае сборки с помощью Visual C++, массив будет заполнен бессмысленными значениями. Эти значения будут равны результату повторения константы в каждом из 4 байт:
SETCC_INVALID << 24 | SETCC_INVALID << 16 | SETCC_INVALID << 8 | SETCC_INVALID
Правильный вариант заполнения массива:
std::fill(CCs, CCs + RTLIB::UNKNOWN_LIBCALL, ISD::SETCC_INVALID);
air<JITTargetAddress, Edge::Kind>>
EHFrameEdgeFixer::readEncodedPointer(uint8_t PointerEncoding,
JITTargetAddress PointerFieldAddress,
BinaryStreamReader &RecordReader) {
.....
Edge::Kind PointerEdgeKind;
switch (EffectiveType) {
case DW_EH_PE_udata4: {
....
PointerEdgeKind = Delta32;
break;
}
case DW_EH_PE_udata8: {
....
PointerEdgeKind = Delta64;
break;
}
case DW_EH_PE_sdata4: {
....
PointerEdgeKind = Delta32;
break;
}
case DW_EH_PE_sdata8: {
....
PointerEdgeKind = Delta64;
break;
}
}
if (PointerEdgeKind == Edge::Invalid)
return make_error<JITLinkError>(
"Unspported edge kind for encoded pointer at " +
formatv("{0:x}", PointerFieldAddress));
return std::make_pair(Addr, Delta64);
}
Предупреждение PVS-Studio: V614 [CWE-457, CERT-EXP53-CPP] Potentially uninitialized variable 'PointerEdgeKind' used. EHFrameSupport.cpp 704
Переменная PointerEdgeKind может остаться неинициализированной после выполнения switch-блока. Однако если переменная ничем не инициализировалась, то ожидается, что она равна именованной константе Edge::Invalid.
Следует сразу при объявлении переменной инициализировать её этой константой:
Edge::Kind PointerEdgeKind = Edge::Invalid;
Ещё одна такая ошибка: V614 [CWE-457, CERT-EXP53-CPP] Potentially uninitialized variable 'Result' used. llvm-rtdyld.cpp 998
void llvm::report_fatal_error(const Twine &Reason, bool GenCrashDiag) {
....
abort();
}
Для нас важно то, что она прекращает выполнение программы с помощью вызова функции abort. Т. е. функция report_fatal_error никогда не возвращает управление.
Ещё есть вот такая промежуточная функция, вызов которой мы рассмотрим далее:
void llvm::report_fatal_error(const char *Reason, bool GenCrashDiag) {
report_fatal_error(Twine(Reason), GenCrashDiag);
}
Примечание. Аргумент GenCrashDiag является необязательным:
__declspec(noreturn) void report_fatal_error(const char *reason,
bool gen_crash_diag = true);
Кстати, сейчас подумал, что тело функции можно было и не рассматривать. Уже из аннотации функции __declspec(noreturn) понятно, что она не возвращает управление. Но решил оставить как есть, чтобы объяснить ситуацию максимально подробно.
Теперь к сути. Рассмотрим вот этот фрагмент кода:
int AMDGPUCFGStructurizer::improveSimpleJumpintoIf(....)
{
....
if (LandBlkHasOtherPred) {
report_fatal_error("Extra register needed to handle CFG");
Register CmpResReg =
HeadMBB->getParent()->getRegInfo().createVirtualRegister(I32RC);
report_fatal_error("Extra compare instruction needed to handle CFG");
insertCondBranchBefore(LandBlk, I, R600::IF_PREDICATE_SET,
CmpResReg, DebugLoc());
}
....
}
Предупреждение PVS-Studio: V779 [CWE-561, CERT-MSC12-C] Unreachable code detected. It is possible that an error is present. AMDILCFGStructurizer.cpp 1286
Обратите внимание, что после вызова функции report_fatal_error программа ещё пытается что-то делать. Все эти действия уже не имеют смысла.
Есть подозрение, что автор кода вовсе не планировал полностью прекратить выполнение программы, а просто хотел сообщить об ошибке. Возможно, нужно было использовать какую-то другую функцию для логирования информации о проблеме.
const MCAsmLayout &Layout) {
....
if (EmitAddrsigSection) {
auto Frag = new MCDataFragment(AddrsigSection);
Frag->setLayoutOrder(0);
raw_svector_ostream OS(Frag->getContents());
for (const MCSymbol *S : AddrsigSyms) {
if (!S->isTemporary()) {
encodeULEB128(S->getIndex(), OS);
continue;
}
MCSection *TargetSection = &S->getSection();
assert(SectionMap.find(TargetSection) != SectionMap.end() &&
"Section must already have been defined in "
"executePostLayoutBinding!");
encodeULEB128(SectionMap[TargetSection]->Symbol->getIndex(), OS);
}
}
....
}
Предупреждение PVS-Studio: V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] Visibility scope of the 'Frag' pointer was exited without releasing the memory. A memory leak is possible. WinCOFFObjectWriter.cpp 1116
Возможно, я чего-то не понимаю и это не ошибка. Но я не вижу, где и как может быть удалён объект, на который ссылается указатель Frag. Я согласен с анализатором: это очень похоже на утечку памяти.
Аналогичная ситуация: V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] Visibility scope of the 'Frag' pointer was exited without releasing the memory. A memory leak is possible. WinCOFFObjectWriter.cpp 1130
uint16_t Ret = 0;
if (Flags & COFF::IMAGE_SCN_MEM_READ)
Ret |= static_cast<uint16_t>(OMFSegDescFlags::Read);
if (Flags & COFF::IMAGE_SCN_MEM_WRITE)
Ret |= static_cast<uint16_t>(OMFSegDescFlags::Write);
if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);
if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);
if (!(Flags & COFF::IMAGE_SCN_MEM_16BIT))
Ret |= static_cast<uint16_t>(OMFSegDescFlags::AddressIs32Bit);
....
}
Предупреждение PVS-Studio: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 335, 337. DbiStreamBuilder.cpp 337
Вот этот фрагмент повторяется два раза:
if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);
Я склонен считать, что это случайный лишний код и его следует просто удалить. Впрочем, это может быть и настоящей ошибкой, если во втором блоке предполагалось выполнять другие проверки и действия.
....
void FilePath::Normalize() {
if (pathname_.c_str() == nullptr) {
pathname_ = "";
return;
}
....
}
Предупреждение PVS-Studio: V547 [CWE-570] Expression 'pathname_.c_str() == nullptr' is always false. gtest-filepath.cc 349
Если удалить всё содержимое функции, то ничего не изменится. Она всё равно ничего не делает. Похоже на артефакт нескольких последовательных рефакторингов.
bool UseHexEscapes) {
....
*this << hexdigit((c >> 4 & 0xF));
*this << hexdigit((c >> 0) & 0xF);
....
}
Предупреждение PVS-Studio: V592 The expression was enclosed by parentheses twice: '((c >> 4 & 0xF))'. One pair of parentheses is unnecessary or misprint is present. raw_ostream.cpp 188
В первой строчке используются двойные скобки. Эта избыточность скобок говорит, что хотели написать выражение как-то по-другому. И действительно, как хотели написать на самом деле видно в следующей строке. Становится понятно, что скобки хотели использовать для упрощения чтения выражения.
Задумывалось написать такой код:
*this << hexdigit((c >> 4) & 0xF);
*this << hexdigit((c >> 0) & 0xF);
Хотя скобка поставлена не там, это не ошибка. Всё равно приоритет сдвига (>>) выше, чем приоритет двоичного И (&). Всё вычисляется корректно.
void ELFState<ELFT>::writeSectionContent(
Elf_Shdr &SHeader, const ELFYAML::StackSizesSection &Section,
ContiguousBlobAccumulator &CBA) {
if (!Section.Entries)
return;
if (!Section.Entries)
return;
....
}
Предупреждение PVS-Studio: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 1380, 1383. ELFEmitter.cpp 1383
Это похоже на какое-то неудачное слияние двух веток кода, из-за которого возникли дублирующиеся строки. Не ошибка, но стоит удалить дубликат.
Вот ещё схожие фрагменты с дубликатами кода:
Раз PVS-Studio способен находить ошибки даже в таких хорошо оттестированных приложениях, как компиляторы, то есть смысл посмотреть, что он сможет найти в ваших проектах. Предлагаю, не откладывая попробовать триальную версию анализатора. Спасибо за внимание.
habr.com
Как появилась эта статья
Компиляторы и встроенные в них анализаторы кода постоянно развиваются. Развиваются и анализаторы, встроенные в среды разработки, такие как Visual Studio и CLion. У разработчиков возникает резонный вопрос, есть ли смысл использовать дополнительные решения для контроля качества кода? Или можно положиться на встроенные средства современного компилятора или IDE?Разрабатывая проект, рационально использовать минимально необходимое количество приложений. Поэтому если встроенные механизмы обеспечивают необходимый уровень анализа кода, то есть смысл ограничиться ими и не добавлять в пайплайн дополнительные утилиты.
Для нашей команды разработчиков PVS-Studio это проявляется следующим образом. Время от времени нас спрашивают, обеспечиваем ли мы анализ лучше, чем компилятор X или встроенный в этот компилятор статический анализатор? Как правило, количество таких вопросов увеличивается в момент очередного релиза компилятора.
Правильные варианты ответов на такие вопросы:
- наш анализатор постоянно развивается. Появляются новые диагностики (пример), совершенствуется механизм анализа потока данных (пример) и так далее. Компиляторы учатся находить новые ошибки, а PVS-Studio учится ещё быстрее. В этом суть существования платных профессиональных решений статического анализа кода;
- некорректно сравнивать анализаторы по количеству диагностик. Важно и их качество, и возможность лёгкой интеграции в процесс разработки. Много значит развитая инфраструктура и интеграция с такими системами, как SonarQube, PlatformIO, Azure DevOps, Travis CI, CircleCI, GitLab CI/CD, Jenkins и так далее. И, конечно, следует помнить про поддержку. Поэтому несколько новых диагностических правил, появившихся в компиляторе, ничего не меняют.
. Закрадывается впечатление, что мы хотим уйти от вопроса в сторону. Выходом является написание таких статей, как эта. Наша команда проверяет компиляторы и тем самым демонстрирует возможности продукта.В данном случае мы проверим недавний релиз LLVM 13.0.0. Конечно, читателю и нам интересен не сам LLVM. Подразумевается, что мы оцениваем мощь PVS-Studio в сравнении с компилятором Clang, Clang Static Analyzer и Clang-Tidy. Перечисленные программы используются при разработке проекта LLVM. И если, несмотря на их использование, удастся что-то найти, это продемонстрирует пользу внедрения PVS-Studio в процесс разработки.
Если интересно, здесь можно познакомиться с предыдущей аналогичной статьёй времён LLVM 11.
Проверка LLVM
Предупреждения PVS-Studio удобнее всего смотреть в среде разработки. У меня на машине из сред разработки оказалась доступна Visual Studio 2019. Ей я и воспользовался. Процесс крайне прост:- скачиваем исходные коды LLVM 13.0.0;
- создаём проект для VS2019: cmake -S llvm -B build -G "Visual Studio 16 2019";
- компилируем. Этот шаг необходим, чтобы были сгенерированы различные inc-файлы. Без них невозможно препроцессировать, а следовательно, и анализировать многие cpp-файлы;
- удивляемся, что создалось разных файлов более чем на 100 Gb;
- в меню Visual Studio указываем плагину PVS-Studio проверить проект;
- profit.
Если вы хотите начать использовать анализатор регулярно, то требуется его настройка. Помимо этого, на начальном этапе будет полезно объявить все предупреждения техническим долгом и спрятать их. После чего начать работать только с новыми предупреждениями, постепенно ликвидируя технический долг. Более подробно этот подход описан здесь.
Настройке и внедрению анализатора посвящены многие другие статьи, поэтому не будем здесь удаляться от главной темы. Давайте же, наконец, посмотрим, что удалось найти интересного.
Я потратил на просмотр лога один вечер и выписал то, что показалось мне интересным. Уверен, можно найти гораздо больше ошибок. Однако даже то, что, просто бегло пробежавшись по отчёту, можно исправить 20 ошибок, демонстрирует анализатор в выгодном свете.
Опечатки
PVS-Studio всегда был и остаётся силён в обнаружении опечаток. Опечатки сразу хорошо видны в демонстрируемом фрагменте кода. Но, несмотря на свою простоту, эти ошибки проходят обзоры кода и затем заставляют морщиться, когда выявляются после отладки.Несложно придумать идеи правил для обнаружения опечаток. А вот реализовать их куда сложнее. Нужно соблюсти баланс между полезными предупреждениями и ложно-положительными срабатываниями. В компиляторе Clang и сопутствующих анализаторах есть диагностики для выявления многих разновидностей ошибок, которые будут описаны ниже. Но раз они не помогли, значит в нашем анализаторе эти диагностики сделаны более качественно.
Баг N1, ошибка конструирования 64-битного значения из двух 32-битных значений
uint64_t uval;....
bool DWARFFormValue::extractValue(const DWARFDataExtractor &Data,
uint64_t *OffsetPtr, dwarf::FormParams FP,
const DWARFContext *Ctx,
const DWARFUnit *CU) {
....
case DW_FORM_LLVM_addrx_offset:
Value.uval = Data.getULEB128(OffsetPtr, &Err) << 32;
Value.uval = Data.getU32(OffsetPtr, &Err);
break;
....
}
Предупреждение PVS-Studio: V519 [CWE-563, CERT-MSC13-C] The 'Value.uval' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 334, 335. DWARFFormValue.cpp 335
Нет смысла в одну и ту же переменную записывать поочередно разные значения. Об этом и предупреждает анализатор. Скорее всего, автор кода немного поспешил и допустил опечатку, забыв добавить '|'. Данный код должен создавать из двух 32-битных значений одно 64-битное. Корректный вариант этого кода:
case DW_FORM_LLVM_addrx_offset:
Value.uval = Data.getULEB128(OffsetPtr, &Err) << 32;
Value.uval |= Data.getU32(OffsetPtr, &Err);
break;
Баг N2, поспешный copy-paste кода
В классе ExecutorAddress потребовалось реализовать набор однотипных операторов. Почти наверняка это делалось с помощью copy-paste. Согласитесь, достаточно скучно писать следующий код, не копируя строки.class ExecutorAddress {
....
ExecutorAddress &operator++() {
++Addr;
return *this;
}
ExecutorAddress &operator--() {
--Addr;
return *this;
}
ExecutorAddress operator++(int) { return ExecutorAddress(Addr++); }
ExecutorAddress operator--(int) { return ExecutorAddress(Addr++); }
ExecutorAddress &operator+=(const ExecutorAddrDiff Delta) {
Addr += Delta.getValue();
return *this;
}
ExecutorAddress &operator-=(const ExecutorAddrDiff Delta) {
Addr -= Delta.getValue();
return *this;
}
....
private:
uint64_t Addr = 0;
}
К сожалению, за скорость написания, приходится расплачиваться высокой вероятностью забыть что-то исправить в скопированном коде. А поскольку скучно не только писать, но и проверять такой код, то он "притягивает" к себе ошибки.
К счастью, статические анализаторы не устают и не ленятся
. PVS-Studio хорошо дополняет классический обзор кода, предлагая обратить внимание на эти две функции:ExecutorAddress operator++(int) { return ExecutorAddress(Addr++); }
ExecutorAddress operator--(int) { return ExecutorAddress(Addr++); }
Предупреждение PVS-Studio: V524 It is odd that the body of '--' function is fully equivalent to the body of '++' function. ExecutorAddress.h 104
Явная ошибка: забыли заменить в правой части скопированной строки оператор ++ на --.
Баг N3, никто не умеет писать функции сравнения
bool operator==(const BDVState &Other) const {return OriginalValue == OriginalValue && BaseValue == Other.BaseValue &&
Status == Other.Status;
}
V501 [CWE-571] There are identical sub-expressions to the left and to the right of the '==' operator: OriginalValue == OriginalValue RewriteStatepointsForGC.cpp 758
Классика! Я даже отдельную большую статью писал на эту тему — "Зло живёт в функциях сравнения".
Чтобы снизить количество таких ошибок, рекомендую всегда оформлять однотипные операции табличным методом. Я бы оформил эту функцию так:
bool operator==(const BDVState &Other) const {
return
OriginalValue == OriginalValue
&& BaseValue == Other.BaseValue
&& Status == Other.Status;
}
Так длиннее, но зато больше вероятность, что опечатка будет замечена на code review. Впрочем, даже так ошибку можно просмотреть и лучше дополнительно подстраховаться, используя профессиональный анализатор.
Баг N4, никто не умеет писать функции сравнения (я серьезно)
Возможно, рассматривая предыдущий пример, вы подумали, что я преувеличиваю и перед нами одиночный случайный ляп. К сожалению, функции сравнения действительно тяготеют к опечаткам. Рассмотрим ещё один пример.bool TypeInfer::EnforceSmallerThan(TypeSetByHwMode &Small,
TypeSetByHwMode &Big) {
....
if (Small.empty())
Changed |= EnforceAny(Small);
if (Big.empty())
Changed |= EnforceAny(Big);
assert(Small.hasDefault() && Big.hasDefault());
SmallVector<unsigned, 4> Modes;
union_modes(Small, Big, Modes);
for (unsigned M : Modes) {
TypeSetByHwMode::SetType &S = Small.get(M);
TypeSetByHwMode::SetType &B = Big.get(M);
if (any_of(S, isIntegerOrPtr) && any_of(S, isIntegerOrPtr)) {
auto NotInt = [](MVT VT) { return !isIntegerOrPtr(VT); };
Changed |= berase_if(S, NotInt);
Changed |= berase_if(B, NotInt);
} else if (any_of(S, isFloatingPoint) && any_of(B, isFloatingPoint)) {
auto NotFP = [](MVT VT) { return !isFloatingPoint(VT); };
Changed |= berase_if(S, NotFP);
Changed |= berase_if(B, NotFP);
} else if (S.empty() || B.empty()) {
Changed = !S.empty() || !B.empty();
S.clear();
B.clear();
} else {
TP.error("Incompatible types");
return Changed;
}
....
}
Прежде чем я покажу, где ошибка, можете попробовать проверить свою внимательность и найти опечатку самостоятельно. Вставлю картинку, чтобы вы сразу не посмотрели ответ.
Проблема здесь:
if (any_of(S, isIntegerOrPtr) && any_of(S, isIntegerOrPtr))
Предупреждение PVS-Studio: V501 [CWE-571] There are identical sub-expressions 'any_of(S, isIntegerOrPtr)' to the left and to the right of the '&&' operator. CodeGenDAGPatterns.cpp 479
Правильно:
if (any_of(S, isIntegerOrPtr) && any_of(B, isIntegerOrPtr))
Баг N5, форматирование таблицей не всегда помогает
LegalizerHelper::LegalizeResult LegalizerHelper::lowerRotate(MachineInstr &MI) {Register Dst = MI.getOperand(0).getReg();
Register Src = MI.getOperand(1).getReg();
Register Amt = MI.getOperand(2).getReg();
LLT DstTy = MRI.getType(Dst);
LLT SrcTy = MRI.getType(Dst);
LLT AmtTy = MRI.getType(Amt);
....
}
Предупреждение PVS-Studio: V656 [CWE-665] Variables 'DstTy', 'SrcTy' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'MRI.getType(Dst)' expression. Check lines: 5953, 5954. LegalizerHelper.cpp 5954
Ранее я писал, что от опечаток помогает защититься форматирование кода таблицей. Да, помогает, но гарантии не даёт. Перед нами красиво оформленный код, похожий на таблицу. Но ошибка всё равно там есть.
Скорее всего, код писался копированием. Скопировали строчку
LLT DstTy = MRI.getType(Dst);
Но только в одном месте заменили Dst на Src:
LLT SrcTy = MRI.getType(Dst);
Правильный вариант кода:
LLT DstTy = MRI.getType(Dst);
LLT SrcTy = MRI.getType(Src);
LLT AmtTy = MRI.getType(Amt);
Нулевые указатели
Писать код на C или C++ и случайно не разыменовать где-то нулевой указатель – это научная фантастика. Есть такие места и в LLVM. Изучать предупреждения про нулевые указатели скучно и утомительно. Я просмотрел такие предупреждения очень бегло и подозреваю, что при желании таких ошибок будет выявлено намного больше.Баг N6, разыменование указателя, который может быть нулевым
void DwarfCompileUnit::addLabelAddress(DIE &Die, dwarf::Attribute Attribute,const MCSymbol *Label) {
....
if (Label)
DD->addArangeLabel(SymbolCU(this, Label));
bool UseAddrOffsetFormOrExpressions =
DD->useAddrOffsetForm() || DD->useAddrOffsetExpressions();
const MCSymbol *Base = nullptr;
if (Label->isInSection() && UseAddrOffsetFormOrExpressions)
Base = DD->getSectionLabel(&Label->getSection());
....
}
Предупреждение PVS-Studio: V1004 [CWE-476, CERT-EXP34-C] The 'Label' pointer was used unsafely after it was verified against nullptr. Check lines: 74, 81. DwarfCompileUnit.cpp 81
Проверка "if (Label)" говорит нам и анализатору, что указатель Label может быть нулевым. Но ниже этот указатель смело разыменовывается без всякой проверки:
if (Label->isInSection() && UseAddrOffsetFormOrExpressions)
Так явно не стоит делать.
Баг N7-N9, разыменование указателя, который может быть нулевым
static bool HandleUse(....){
....
if (Pat->isLeaf()) {
DefInit *DI = dyn_cast<DefInit>(Pat->getLeafValue());
if (!DI)
I.error("Input $" + Pat->getName() + " must be an identifier!");
Rec = DI->getDef();
}
....
}
Предупреждение PVS-Studio: V1004 [CWE-476, CERT-EXP34-C] The 'DI' pointer was used unsafely after it was verified against nullptr. Check lines: 3349, 3351. CodeGenDAGPatterns.cpp 3351
Указатель DI проверятся, но далее сразу разыменовывается уже без проверки. Обосновано задаться вопросом, а является ли этой ошибкой? Ведь если указатель DI нулевой, то вызывается некая функция error, которая может генерировать исключение. Давайте заглянем в эту функцию:
void TreePattern::error(const Twine &Msg) {
if (HasError)
return;
dump();
PrintError(TheRecord->getLoc(), "In " + TheRecord->getName() + ": " + Msg);
HasError = true;
}
Нет, эта функция не генерирует исключение и не останавливает выполнение программы каким-либо способом.
Получается, что сразу после фиксации ошибочного состояния (записи в лог) произойдёт разыменование нулевого указателя.
Как минимум, есть ещё пара аналогичных ошибок, которые нет смысла рассматривать отдельно:
- V1004 [CWE-476, CERT-EXP34-C] The 'OpDef' pointer was used unsafely after it was verified against nullptr. Check lines: 2843, 2844. CodeGenDAGPatterns.cpp 2844
- V1004 [CWE-476, CERT-EXP34-C] The 'Val' pointer was used unsafely after it was verified against nullptr. Check lines: 3418, 3420. CodeGenDAGPatterns.cpp 3420
Баг N10, недостаточная защита от нулевого указателя
Error DWARFDebugLine::LineTable:arse(...., raw_ostream *OS, bool Verbose) {assert((OS || !Verbose) && "cannot have verbose output without stream");
....
auto EmitRow = [&] {
if (!TombstonedAddress) {
if (Verbose) {
*OS << "\n";
OS->indent(12);
}
if (OS)
State.Row.dump(*OS);
State.appendRowToMatrix();
}
};
....
}
Предупреждение PVS-Studio: V595 [CWE-476, CERT-EXP12-C] The 'OS' pointer was utilized before it was verified against nullptr. Check lines: 791, 793. DWARFDebugLine.cpp 791
Указатель OS может быть нулевым, о чём свидетельствует проверка "if (OS)". Однако ранее этот указатель может быть разыменован без предварительной проверки.
В начале есть assert, который защищает от нулевых указателей. Однако этого явно недостаточно, так как в релизной сборке макрос assert раскрывается в ничто.
Рационально сделать код более безопасным:
auto EmitRow = [&] {
if (!TombstonedAddress) {
if (OS)
{
if (Verbose) {
*OS << "\n";
OS->indent(12);
}
State.Row.dump(*OS);
}
State.appendRowToMatrix();
}
};
Проблемы с перечислениями (enum)
Разработчики LLVM иногда полагаются на то, что небольшие перечисления (enum) будут представлены одним байтом. То есть sizeof(enum) == sizeof(char). Такое предположение опасно. Например, компилятор Visual C++ предпочитает по умолчанию делать размер перечисления, равным размеру int.Баг N11, опасный индекс
enum class FunctionKinds { ENTRY, EXIT, TAIL, LOG_ARGS_ENTER, CUSTOM_EVENT };....
static Error loadObj(....) {
....
auto Kind = Extractor.getU8(&OffsetPtr);
static constexpr SledEntry::FunctionKinds Kinds[] = {
SledEntry::FunctionKinds::ENTRY, SledEntry::FunctionKinds::EXIT,
SledEntry::FunctionKinds::TAIL,
SledEntry::FunctionKinds::LOG_ARGS_ENTER,
SledEntry::FunctionKinds::CUSTOM_EVENT};
if (Kind >= sizeof(Kinds))
return errorCodeToError(
std::make_error_code(std::errc::executable_format_error));
Entry.Kind = Kinds[Kind];
....
}
Предупреждение PVS-Studio: V557 [CWE-125, CERT-ARR30-C] Array overrun is possible. The value of 'Kind' index could reach 19. InstrumentationMap.cpp 196
Текст сообщения требует пояснения. Механизм анализа потока данных обрабатывает этот код:
if (Kind >= sizeof(Kinds))
return errorCodeToError(...);
И приходит к выводу, что если условие не выполняется, то переменная Kind имеет далее значение [0..19].
Почему 19, а не 4? Я осуществлял проверку проекта с помощью плагина для Visual Studio 2019. Соответственно, анализатор знает, что использовался компилятор Visual C++ и что перечисление будет представлено четырьмя байтами. В этом можно убедиться, написав следующую тестовую программу:
int main()
{
enum class FunctionKinds { ENTRY, EXIT, TAIL, LOG_ARGS_ENTER, CUSTOM_EVENT };
static constexpr FunctionKinds Kinds[] = {
FunctionKinds::ENTRY, FunctionKinds::EXIT, FunctionKinds::TAIL,
FunctionKinds::LOG_ARGS_ENTER, FunctionKinds::CUSTOM_EVENT
};
std::cout << sizeof(Kinds) << std::endl;
return 0;
}
Собираем с помощью компилятора Visual C++, запускаем и видим число "20".
Получается, что защита от выхода за границу массива не работает. Чтобы исправить код, нужно сравнивать Kind не с размером массива в байтах, а с количеством элементом массива.
Корректная проверка:
if (Kind >= sizeof(Kinds) / sizeof(Kinds[0]))
return errorCodeToError(....);
Баг N12, ошибка инициализации массива
enum CondCode {// Opcode N U L G E Intuitive operation
SETFALSE, // 0 0 0 0 Always false (always folded)
SETOEQ, // 0 0 0 1 True if ordered and equal
....
SETCC_INVALID // Marker value.
};
static void InitCmpLibcallCCs(ISD::CondCode *CCs) {
memset(CCs, ISD::SETCC_INVALID, sizeof(ISD::CondCode)*RTLIB::UNKNOWN_LIBCALL);
....
}
Предупреждение PVS-Studio: V575 [CWE-628, CERT-EXP37-C] The 'memset' function processes the pointer to enum type. Inspect the first argument. TargetLoweringBase.cpp 662
Код будет работать только в том случае, если повезёт и элементы перечисления CondCode будут представлены одним байтом.
Функция memset заполняет массив байт. В каждый байт записывается значение SETCC_INVALID. Если enum представлен 4 байтами, как это происходит в случае сборки с помощью Visual C++, массив будет заполнен бессмысленными значениями. Эти значения будут равны результату повторения константы в каждом из 4 байт:
SETCC_INVALID << 24 | SETCC_INVALID << 16 | SETCC_INVALID << 8 | SETCC_INVALID
Правильный вариант заполнения массива:
std::fill(CCs, CCs + RTLIB::UNKNOWN_LIBCALL, ISD::SETCC_INVALID);
Ошибки в потоке управления
Баг N13-N14, неинициализированная переменная
Expected<std:air<JITTargetAddress, Edge::Kind>>EHFrameEdgeFixer::readEncodedPointer(uint8_t PointerEncoding,
JITTargetAddress PointerFieldAddress,
BinaryStreamReader &RecordReader) {
.....
Edge::Kind PointerEdgeKind;
switch (EffectiveType) {
case DW_EH_PE_udata4: {
....
PointerEdgeKind = Delta32;
break;
}
case DW_EH_PE_udata8: {
....
PointerEdgeKind = Delta64;
break;
}
case DW_EH_PE_sdata4: {
....
PointerEdgeKind = Delta32;
break;
}
case DW_EH_PE_sdata8: {
....
PointerEdgeKind = Delta64;
break;
}
}
if (PointerEdgeKind == Edge::Invalid)
return make_error<JITLinkError>(
"Unspported edge kind for encoded pointer at " +
formatv("{0:x}", PointerFieldAddress));
return std::make_pair(Addr, Delta64);
}
Предупреждение PVS-Studio: V614 [CWE-457, CERT-EXP53-CPP] Potentially uninitialized variable 'PointerEdgeKind' used. EHFrameSupport.cpp 704
Переменная PointerEdgeKind может остаться неинициализированной после выполнения switch-блока. Однако если переменная ничем не инициализировалась, то ожидается, что она равна именованной константе Edge::Invalid.
Следует сразу при объявлении переменной инициализировать её этой константой:
Edge::Kind PointerEdgeKind = Edge::Invalid;
Ещё одна такая ошибка: V614 [CWE-457, CERT-EXP53-CPP] Potentially uninitialized variable 'Result' used. llvm-rtdyld.cpp 998
Баг N15, недостижимый код
В начале рассмотрим вспомогательную функцию report_fatal_error:void llvm::report_fatal_error(const Twine &Reason, bool GenCrashDiag) {
....
abort();
}
Для нас важно то, что она прекращает выполнение программы с помощью вызова функции abort. Т. е. функция report_fatal_error никогда не возвращает управление.
Ещё есть вот такая промежуточная функция, вызов которой мы рассмотрим далее:
void llvm::report_fatal_error(const char *Reason, bool GenCrashDiag) {
report_fatal_error(Twine(Reason), GenCrashDiag);
}
Примечание. Аргумент GenCrashDiag является необязательным:
__declspec(noreturn) void report_fatal_error(const char *reason,
bool gen_crash_diag = true);
Кстати, сейчас подумал, что тело функции можно было и не рассматривать. Уже из аннотации функции __declspec(noreturn) понятно, что она не возвращает управление. Но решил оставить как есть, чтобы объяснить ситуацию максимально подробно.
Теперь к сути. Рассмотрим вот этот фрагмент кода:
int AMDGPUCFGStructurizer::improveSimpleJumpintoIf(....)
{
....
if (LandBlkHasOtherPred) {
report_fatal_error("Extra register needed to handle CFG");
Register CmpResReg =
HeadMBB->getParent()->getRegInfo().createVirtualRegister(I32RC);
report_fatal_error("Extra compare instruction needed to handle CFG");
insertCondBranchBefore(LandBlk, I, R600::IF_PREDICATE_SET,
CmpResReg, DebugLoc());
}
....
}
Предупреждение PVS-Studio: V779 [CWE-561, CERT-MSC12-C] Unreachable code detected. It is possible that an error is present. AMDILCFGStructurizer.cpp 1286
Обратите внимание, что после вызова функции report_fatal_error программа ещё пытается что-то делать. Все эти действия уже не имеют смысла.
Есть подозрение, что автор кода вовсе не планировал полностью прекратить выполнение программы, а просто хотел сообщить об ошибке. Возможно, нужно было использовать какую-то другую функцию для логирования информации о проблеме.
Баг N16-N17, утечка памяти
uint64_t WinCOFFObjectWriter::writeObject(MCAssembler &Asm,const MCAsmLayout &Layout) {
....
if (EmitAddrsigSection) {
auto Frag = new MCDataFragment(AddrsigSection);
Frag->setLayoutOrder(0);
raw_svector_ostream OS(Frag->getContents());
for (const MCSymbol *S : AddrsigSyms) {
if (!S->isTemporary()) {
encodeULEB128(S->getIndex(), OS);
continue;
}
MCSection *TargetSection = &S->getSection();
assert(SectionMap.find(TargetSection) != SectionMap.end() &&
"Section must already have been defined in "
"executePostLayoutBinding!");
encodeULEB128(SectionMap[TargetSection]->Symbol->getIndex(), OS);
}
}
....
}
Предупреждение PVS-Studio: V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] Visibility scope of the 'Frag' pointer was exited without releasing the memory. A memory leak is possible. WinCOFFObjectWriter.cpp 1116
Возможно, я чего-то не понимаю и это не ошибка. Но я не вижу, где и как может быть удалён объект, на который ссылается указатель Frag. Я согласен с анализатором: это очень похоже на утечку памяти.
Аналогичная ситуация: V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] Visibility scope of the 'Frag' pointer was exited without releasing the memory. A memory leak is possible. WinCOFFObjectWriter.cpp 1130
Код с запахом
В данном разделе я собрал фрагменты кода, которые обратили на себя внимание, но которые я не спешу назвать багами. Скорее это избыточный и неудачный код. Сейчас сразу станет понятно, что я имею в виду.Код с запахом N1, дубликаты строк
static uint16_t toSecMapFlags(uint32_t Flags) {uint16_t Ret = 0;
if (Flags & COFF::IMAGE_SCN_MEM_READ)
Ret |= static_cast<uint16_t>(OMFSegDescFlags::Read);
if (Flags & COFF::IMAGE_SCN_MEM_WRITE)
Ret |= static_cast<uint16_t>(OMFSegDescFlags::Write);
if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);
if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);
if (!(Flags & COFF::IMAGE_SCN_MEM_16BIT))
Ret |= static_cast<uint16_t>(OMFSegDescFlags::AddressIs32Bit);
....
}
Предупреждение PVS-Studio: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 335, 337. DbiStreamBuilder.cpp 337
Вот этот фрагмент повторяется два раза:
if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);
Я склонен считать, что это случайный лишний код и его следует просто удалить. Впрочем, это может быть и настоящей ошибкой, если во втором блоке предполагалось выполнять другие проверки и действия.
Код с запахом N2, атавизм
std::string pathname_;....
void FilePath::Normalize() {
if (pathname_.c_str() == nullptr) {
pathname_ = "";
return;
}
....
}
Предупреждение PVS-Studio: V547 [CWE-570] Expression 'pathname_.c_str() == nullptr' is always false. gtest-filepath.cc 349
Если удалить всё содержимое функции, то ничего не изменится. Она всё равно ничего не делает. Похоже на артефакт нескольких последовательных рефакторингов.
Код с запахом N3, не там поставлена скобка
raw_ostream &raw_ostream::write_escaped(StringRef Str,bool UseHexEscapes) {
....
*this << hexdigit((c >> 4 & 0xF));
*this << hexdigit((c >> 0) & 0xF);
....
}
Предупреждение PVS-Studio: V592 The expression was enclosed by parentheses twice: '((c >> 4 & 0xF))'. One pair of parentheses is unnecessary or misprint is present. raw_ostream.cpp 188
В первой строчке используются двойные скобки. Эта избыточность скобок говорит, что хотели написать выражение как-то по-другому. И действительно, как хотели написать на самом деле видно в следующей строке. Становится понятно, что скобки хотели использовать для упрощения чтения выражения.
Задумывалось написать такой код:
*this << hexdigit((c >> 4) & 0xF);
*this << hexdigit((c >> 0) & 0xF);
Хотя скобка поставлена не там, это не ошибка. Всё равно приоритет сдвига (>>) выше, чем приоритет двоичного И (&). Всё вычисляется корректно.
Код с запахом N4-N6, неудачное слияние кода?
template <class ELFT>void ELFState<ELFT>::writeSectionContent(
Elf_Shdr &SHeader, const ELFYAML::StackSizesSection &Section,
ContiguousBlobAccumulator &CBA) {
if (!Section.Entries)
return;
if (!Section.Entries)
return;
....
}
Предупреждение PVS-Studio: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 1380, 1383. ELFEmitter.cpp 1383
Это похоже на какое-то неудачное слияние двух веток кода, из-за которого возникли дублирующиеся строки. Не ошибка, но стоит удалить дубликат.
Вот ещё схожие фрагменты с дубликатами кода:
- V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 1488, 1491. ELFEmitter.cpp 1491
- V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 1663, 1666. ELFEmitter.cpp 1666
Заключение
PVS-Studio по прежнему занимает достойное место среди решений для разработчиков. Он производил и продолжает производить более глубокий и разнообразный анализ кода, чем компиляторы и бесплатные инструменты.
Раз PVS-Studio способен находить ошибки даже в таких хорошо оттестированных приложениях, как компиляторы, то есть смысл посмотреть, что он сможет найти в ваших проектах
. Предлагаю, не откладывая попробовать триальную версию анализатора. Спасибо за внимание.Дополнительные ссылки
- Как внедрить статический анализатор кода в legacy проект и не демотивировать команду.
- Технология статического анализа кода PVS-Studio.
- Как PVS-Studio защищает от поспешных правок кода.
- Ошибки, которые не находит статический анализ кода, потому что он не используется.
Выявляем ошибки в релизе LLVM 13.0.0
Задача коммерческих статических анализаторов выполнять более глубокий и полный анализ кода, чем компиляторы. Давайте посмотрим, что смог обнаружить PVS-Studio в исходном коде проекта LLVM 13.0.0. Как...
habr.com