Пройшло понад два роки з моменту останньої перевірки коду проекту LLVM за допомогою нашого аналізатора PVS-Studio. Давайте переконаємося, що аналізатор PVS-Studio, як і раніше, є лідируючим інструментом виявлення помилок і потенційних уразливостей. Для цього перевіримо та знайдемо нові помилки у релізі LLVM 8.0.0.
Стаття, яка має бути написана
Чесно кажучи, мені не хотілося писати цю статтю. Нецікаво писати про проект, який ми вже неодноразово перевіряли., , ). Краще написати про щось нове, але я не маю вибору.
Щоразу, коли виходить нова версія LLVM або оновлюється , у нас у пошті постають питання наступного типу:
Дивіться, нова версія Clang Static Analyzer навчилася знаходити нові помилки! Мені здається, що актуальність використовувати PVS-Studio зменшується. Clang знаходить більше помилок, ніж раніше, і наздоганяє за можливостями PVS-Studio. Що ви думаєте про це?
На це мені завжди хочеться відповісти щось на кшталт:
Ми теж не сидимо без діла! Ми значно покращили можливості аналізатора PVS-Studio. Тож не хвилюйтеся, ми продовжуємо лідирувати, як і раніше.
На жаль, це погана відповідь. У ньому немає proof-ів. І саме тому зараз я пишу цю статтю. Отже, проект LLVM вкотре перевірено і у ньому знайдено різноманітні помилки. Ті, що мені здалися цікавими, я зараз продемонструю. Ці помилки не може знайти Clang Static Analyzer (або вкрай незручно робити з його допомогою). А ми можемо. До того ж я знайшов і виписав усі ці помилки за один вечір.
А ось написання статті затягнулося на кілька тижнів. Не міг себе змусити все це оформити як тексту :).
До речі, якщо вам цікаво, які технології використовуються в аналізаторі PVS-Studio для виявлення помилок та потенційних уразливостей, то я пропоную познайомитись із цією .
Нові та старі діагностики
Як уже було зазначено, близько двох років тому проект LLVM було в чергове перевірено, а знайдені помилки виправлено. Тепер у цій статті буде представлено нову порцію помилок. Чому знайшли нові помилки? На це є 3 причини:
- Проект LLVM розвивається, у ньому змінюється старий код і з'являється новий. Природно, у зміненому та написаному коді є нові помилки. Це добре демонструє, що статичний аналіз повинен застосовуватися регулярно, а не час від часу. Наші статті добре показують можливості аналізатора PVS-Studio, але це не має нічого спільного з підвищенням якості коду та зниженням вартості виправлення помилок. Використовуйте статичне аналізатор коду регулярно!
- Ми доопрацьовуємо та вдосконалимо вже існуючі діагностики. Тому аналізатор може виявити помилки, які не помічали під час попередніх перевірок.
- У PVS-Studio з'явилися нові діагностики, яких не було 2 роки тому. Я вирішив виділити їх в окремий розділ, щоб показати розвиток PVS-Studio.
Дефекти, виявлені діагностиками, які існували 2 роки тому
Фрагмент N1: Copy-Paste
static bool ShouldUpgradeX86Intrinsic(Function *F, StringRef Name) {
if (Name == "addcarryx.u32" || // Added in 8.0
....
Name == "avx512.mask.cvtps2pd.128" || // Added in 7.0
Name == "avx512.mask.cvtps2pd.256" || // Added in 7.0
Name == "avx512.cvtusi2sd" || // Added in 7.0
Name.startswith("avx512.mask.permvar.") || // Added in 7.0 // <=
Name.startswith("avx512.mask.permvar.") || // Added in 7.0 // <=
Name == "sse2.pmulu.dq" || // Added in 7.0
Name == "sse41.pmuldq" || // Added in 7.0
Name == "avx2.pmulu.dq" || // Added in 7.0
....
}Попередження PVS-Studio: [CWE-570] Є identical sub-expressions 'Name.startswith("avx512.mask.permvar.")' Operator. AutoUpgrade.cpp 73
Двічі перевіряється, що ім'я починається з підрядка "avx512.mask.permvar.". У другій перевірці хотіли написати щось ще, але забули виправити скопійований текст.
Фрагмент N2: Помилка
enum CXNameRefFlags {
CXNameRange_WantQualifier = 0x1,
CXNameRange_WantTemplateArgs = 0x2,
CXNameRange_WantSinglePiece = 0x4
};
void AnnotateTokensWorker::HandlePostPonedChildCursor(
CXCursor Cursor, unsigned StartTokenIndex) {
const auto flags = CXNameRange_WantQualifier | CXNameRange_WantQualifier;
....
}Попередження PVS-Studio: V501 Є identical sub-expressions 'CXNameRange_WantQualifier' до лівого і правого '|' Operator. CIndex.cpp 7245
Через друкарську помилку двічі використовується одна і та ж іменована константа CXNameRange_WantQualifier.
Фрагмент N3: Плутанина з пріоритетами операторів
int PPCTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) {
....
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian() ? 1 : 0)
return 0;
....
}Попередження PVS-Studio: [CWE-783] Perhaps the '?:' роботодавець працює в різних випадках, тому що він був виявлений. The '?:' operator has a lower priority than the '==' operator. PPCTargetTransformInfo.cpp 404
На мою думку, це дуже гарна помилка. Так, я знаю, що у мене дивні уявлення про красу :).
Зараз, згідно , Вираз обчислюється наступним чином:
(ISD == ISD::EXTRACT_VECTOR_ELT && (Index == ST->isLittleEndian())) ? 1 : 0З практичної точки зору така умова не має сенсу, тому що його можна скоротити до:
(ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian())Це очевидна помилка. Швидше за все, 0/1 хотіли порівняти зі змінною індекс. Щоб виправити код, необхідно додати дужки навколо тернарного оператора:
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == (ST->isLittleEndian() ? 1 : 0))До речі, тернарний оператор є дуже небезпечним і провокує логічні помилки. Будьте дуже обережні з ним і не скупіться ставити круглі дужки. Докладніше цю тему я розглядав , у розділі «Бійтеся оператора?: і укладайте його у круглі дужки».
Фрагмент N4, N5: Нульовий покажчик
Init *TGParser::ParseValue(Record *CurRec, RecTy *ItemType, IDParseMode Mode) {
....
TypedInit *LHS = dyn_cast<TypedInit>(Result);
....
LHS = dyn_cast<TypedInit>(
UnOpInit::get(UnOpInit::CAST, LHS, StringRecTy::get())
->Fold(CurRec));
if (!LHS) {
Error(PasteLoc, Twine("can't cast '") + LHS->getAsString() +
"' to string");
return nullptr;
}
....
}Попередження PVS-Studio: [CWE-476] Збереження null pointer 'LHS' might take place. TGParser.cpp 2152
Якщо покажчик LHS виявиться нульовим, то має бути видано попередження. Однак натомість відбудеться розйменування цього самого нульового покажчика: LHS->getAsString().
Це дуже типова ситуація, коли помилка ховається в обробнику помилок, оскільки їх ніхто не тестує. Статичні аналізатори перевіряють весь доступний код незалежно від того, як часто він використовується. Це дуже добрий приклад, як статичний аналіз доповнює інші методики тестування та захисту від помилок.
Аналогічна помилка обробки покажчика RHS допущена в коді трохи нижче: V522 [CWE-476] Збереження null pointer 'RHS' might take place. TGParser.cpp 2186
Фрагмент N6: Використання вказівника після переміщення
static Expected<bool>
ExtractBlocks(....)
{
....
std::unique_ptr<Module> ProgClone = CloneModule(BD.getProgram(), VMap);
....
BD.setNewProgram(std::move(ProgClone)); // <=
MiscompiledFunctions.clear();
for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {
Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first); // <=
assert(NewF && "Function not found??");
MiscompiledFunctions.push_back(NewF);
}
....
}Попередження PVS-Studio: V522 [CWE-476] Відхилення від null pointer 'ProgClone' might take place. Miscompilation.cpp 601
На початку розумний покажчик ProgClone перестає володіти об'єктом:
BD.setNewProgram(std::move(ProgClone));Фактично тепер ProgClone - Це нульовий покажчик. Тому трохи нижче має відбутися розіменування нульового покажчика:
Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);Але насправді цього не станеться! Зауважте, що цикл насправді не виконується.
На початку контейнер MiscompiledFunctions очищується:
MiscompiledFunctions.clear();Далі розмір цього контейнера використовується за умови циклу:
for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {Легко бачити, що цикл не запускається. Думаю, це теж помилка, і код має бути написаний іншим чином.
Здається, ми зустріли ту саму знамениту парність помилок! Одна помилка маскує іншу:).
Фрагмент N7: Використання вказівника після переміщення
static Expected<bool> TestOptimizer(BugDriver &BD, std::unique_ptr<Module> Test,
std::unique_ptr<Module> Safe) {
outs() << " Optimizing functions being tested: ";
std::unique_ptr<Module> Optimized =
BD.runPassesOn(Test.get(), BD.getPassesToRun());
if (!Optimized) {
errs() << " Error running this sequence of passes"
<< " on the input program!n";
BD.setNewProgram(std::move(Test)); // <=
BD.EmitProgressBitcode(*Test, "pass-error", false); // <=
if (Error E = BD.debugOptimizerCrash())
return std::move(E);
return false;
}
....
}Попередження PVS-Studio: V522 [CWE-476] Збереження null pointer 'Test' might take place. Miscompilation.cpp 709
Знову та сама ситуація. Спочатку вміст об'єкта переміщається, а потім він використовується як ні в чому не бувало. Я все частіше зустрічаю цю ситуацію в коді програм після того, як у С++ з'явилася семантика переміщення. За це я люблю мову C++! З'являються нові й нові способи відстрілити собі ногу. Аналізатору PVS-Studio завжди буде робота :).
Фрагмент N8: Нульовий покажчик
void FunctionDumper::dump(const PDBSymbolTypeFunctionArg &Symbol) {
uint32_t TypeId = Symbol.getTypeId();
auto Type = Symbol.getSession().getSymbolById(TypeId);
if (Type)
Printer << "<unknown-type>";
else
Type->dump(*this);
}Попередження PVS-Studio: V522 [CWE-476] Збереження null pointer 'Type' might take place. PrettyFunctionDumper.cpp 233
Крім обробників помилок, зазвичай не тестуються і функції розладу налагодження даних. Перед нами якраз такий випадок. Функція чекає користувача, який замість того, щоб вирішити свої проблеми, буде змушений зайнятися її виправленням.
правильно:
if (Type)
Type->dump(*this);
else
Printer << "<unknown-type>";Фрагмент N9: Нульовий покажчик
void SearchableTableEmitter::collectTableEntries(
GenericTable &Table, const std::vector<Record *> &Items) {
....
RecTy *Ty = resolveTypes(Field.RecType, TI->getType());
if (!Ty) // <=
PrintFatalError(Twine("Field '") + Field.Name + "' of table '" +
Table.Name + "' has incompatible type: " +
Ty->getAsString() + " vs. " + // <=
TI->getType()->getAsString());
....
}Попередження PVS-Studio: V522 [CWE-476] Відхилення null pointer 'Ty' might take place. SearchableTableEmitter.cpp 614
Думаю, і так все зрозуміло і не потребує пояснень.
Фрагмент N10: Помилка
bool FormatTokenLexer::tryMergeCSharpNullConditionals() {
....
auto &Identifier = *(Tokens.end() - 2);
auto &Question = *(Tokens.end() - 1);
....
Identifier->ColumnWidth += Question->ColumnWidth;
Identifier->Type = Identifier->Type; // <=
Tokens.erase(Tokens.end() - 1);
return true;
}Попередження PVS-Studio: 'Identifier->Type' variable is assigned to itself. FormatTokenLexer.cpp 249
Немає сенсу надавати змінну саму собі. Швидше за все хотіли написати:
Identifier->Type = Question->Type;Фрагмент N11: підозрілий break
void SystemZOperand::print(raw_ostream &OS) const {
switch (Kind) {
break;
case KindToken:
OS << "Token:" << getToken();
break;
case KindReg:
OS << "Reg:" << SystemZInstPrinter::getRegisterName(getReg());
break;
....
}Попередження PVS-Studio: [CWE-478] Consider inspecting the 'switch' statement. Це можливо, що перший 'case' operator is missing. SystemZAsmParser.cpp 652
На початку присутній дуже підозрілий оператор перерву. Чи не забули тут ще щось написати?
Фрагмент N12: Перевірка вказівника після розіменування
InlineCost AMDGPUInliner::getInlineCost(CallSite CS) {
Function *Callee = CS.getCalledFunction();
Function *Caller = CS.getCaller();
TargetTransformInfo &TTI = TTIWP->getTTI(*Callee);
if (!Callee || Callee->isDeclaration())
return llvm::InlineCost::getNever("undefined callee");
....
}Попередження PVS-Studio: [CWE-476] The 'Callee' pointer був використаний перед ним, був verified against nullptr. Check lines: 172, 174. AMDGPUInline.cpp 172
покажчик Callee на початку розіменовується в момент виклику функції getTTI.
А потім виявляється, що цей покажчик слід перевіряти на рівність nullptr:
if (!Callee || Callee->isDeclaration())Але вже пізно…
Фрагмент N13 - N…: Перевірка вказівника після розйменування
Ситуація, розглянута у попередньому фрагменті коду, не унікальна. Вона зустрічається тут:
static Value *optimizeDoubleFP(CallInst *CI, IRBuilder<> &B,
bool isBinary, bool isPrecise = false) {
....
Function *CalleeFn = CI->getCalledFunction();
StringRef CalleeNm = CalleeFn->getName(); // <=
AttributeList CalleeAt = CalleeFn->getAttributes();
if (CalleeFn && !CalleeFn->isIntrinsic()) { // <=
....
}Попередження PVS-Studio: V595 [CWE-476] The 'CalleeFn' pointer був використаний перед тим, що він був здійснений проти nullptr. Check lines: 1079, 1081. SimplifyLibCalls.cpp 1079
І тут:
void Sema::InstantiateAttrs(const MultiLevelTemplateArgumentList &TemplateArgs,
const Decl *Tmpl, Decl *New,
LateInstantiatedAttrVec *LateAttrs,
LocalInstantiationScope *OuterMostScope) {
....
NamedDecl *ND = dyn_cast<NamedDecl>(New);
CXXRecordDecl *ThisContext =
dyn_cast_or_null<CXXRecordDecl>(ND->getDeclContext()); // <=
CXXThisScopeRAII ThisScope(*this, ThisContext, Qualifiers(),
ND && ND->isCXXInstanceMember()); // <=
....
}Попередження PVS-Studio: V595 [CWE-476] The 'ND' pointer був використаний перед тим, що він був здійснений проти nullptr. Check lines: 532, 534. SemaTemplateInstantiateDecl.cpp 532
І тут:
- V595 [CWE-476] The 'U' pointer був використаний перед тим, що він був здійснений проти nullptr. Check lines: 404, 407. DWARFFormValue.cpp 404
- V595 [CWE-476] The 'ND' pointer був використаний перед тим, що він був здійснений проти nullptr. Check lines: 2149, 2151. SemaTemplateInstantiate.cpp 2149
А далі мені почало нецікаво вивчати попередження з номером V595. Так що я не знаю, чи є ще подібні помилки, окрім перерахованих тут. Швидше за все є.
Фрагмент N17, N18: підозрілий зсув
static inline bool processLogicalImmediate(uint64_t Imm, unsigned RegSize,
uint64_t &Encoding) {
....
unsigned Size = RegSize;
....
uint64_t NImms = ~(Size-1) << 1;
....
}Попередження PVS-Studio: [CWE-190] Consider inspecting the '~(Size - 1) << 1' expression. Bit shifting of the 32-bit value with subsequent expansion до 64-bit type. AArch64AddressingModes.h 260
Можливо, це не помилка, і код працює саме так, як і задумано. Але це явно дуже підозріле місце і його потрібно перевірити.
Припустимо, змінна Розмір дорівнює 16, і тоді автор коду планував отримати у змінній NImms значення:
1111111111111111111111111111111111111111111111111111111111100000
Однак насправді вийде значення:
0000000000000000000000000000000011111111111111111111111111100000
Справа в тому, що всі обчислення відбуваються з використанням 32-бітного типу unsigned. І тільки потім, цей 32-бітний беззнаковий тип буде неявно розширено до uint64_t. У цьому старші біти виявиться нульовими.
Виправити ситуацію можна так:
uint64_t NImms = ~static_cast<uint64_t>(Size-1) << 1;Аналогічна ситуація: V629 [CWE-190] Consider inspecting the 'Immr << 6' expression. Bit shifting of the 32-bit value with subsequent expansion до 64-bit type. AArch64AddressingModes.h 269
Фрагмент N19: Пропущено ключове слово ще?
void AMDGPUAsmParser::cvtDPP(MCInst &Inst, const OperandVector &Operands) {
....
if (Op.isReg() && Op.Reg.RegNo == AMDGPU::VCC) {
// VOP2b (v_add_u32, v_sub_u32 ...) dpp use "vcc" token.
// Skip it.
continue;
} if (isRegOrImmWithInputMods(Desc, Inst.getNumOperands())) { // <=
Op.addRegWithFPInputModsOperands(Inst, 2);
} else if (Op.isDPPCtrl()) {
Op.addImmOperands(Inst, 1);
} else if (Op.isImm()) {
// Handle optional arguments
OptionalIdx[Op.getImmTy()] = I;
} else {
llvm_unreachable("Invalid operand type");
}
....
}Попередження PVS-Studio: [CWE-670] Consider inspecting the application's logic. It's posible that 'else' keyword is missing. AMDGPUAsmParser.cpp 5655
Помилки тут нема. Так як then-блок першого if закінчується на продовжувати, то не важливо, чи є ключове слово ще чи ні. У будь-якому випадку код працюватиме однаково. Тим не менш, пропущений ще робить код більш незрозумілим та небезпечним. Якщо надалі продовжувати зникне, то код почне працювати зовсім по-іншому. На мій погляд, краще додати ще.
Фрагмент N20: Чотири однотипні друкарські помилки
LLVM_DUMP_METHOD void Symbol::dump(raw_ostream &OS) const {
std::string Result;
if (isUndefined())
Result += "(undef) ";
if (isWeakDefined())
Result += "(weak-def) ";
if (isWeakReferenced())
Result += "(weak-ref) ";
if (isThreadLocalValue())
Result += "(tlv) ";
switch (Kind) {
case SymbolKind::GlobalSymbol:
Result + Name.str(); // <=
break;
case SymbolKind::ObjectiveCClass:
Result + "(ObjC Class) " + Name.str(); // <=
break;
case SymbolKind::ObjectiveCClassEHType:
Result + "(ObjC Class EH) " + Name.str(); // <=
break;
case SymbolKind::ObjectiveCInstanceVariable:
Result + "(ObjC IVar) " + Name.str(); // <=
break;
}
OS << Result;
}Попередження PVS-Studio:
- V655 [CWE-480] strings були виявлені, але не були використані. Consider inspecting the 'Result + Name.str()' expression. Symbol.cpp 32
- V655 [CWE-480] strings були виявлені, але не були використані. Consider inspecting the 'Result + '(ObjC Class) ' + Name.str()' expression. Symbol.cpp 35
- V655 [CWE-480] strings були виявлені, але не були використані. Consider inspecting the 'Result + '(ObjC Class EH) ' + Name.str()' expression. Symbol.cpp 38
- V655 [CWE-480] strings були виявлені, але не були використані. Consider inspecting the 'Result + '(ObjC IVar) ' + Name.str()' expression. Symbol.cpp 41
Випадково замість оператора + використовується оператор +. В результаті виходять конструкції, позбавлені сенсу.
Фрагмент N21: Невизначена поведінка
static void getReqFeatures(std::map<StringRef, int> &FeaturesMap,
const std::vector<Record *> &ReqFeatures) {
for (auto &R : ReqFeatures) {
StringRef AsmCondString = R->getValueAsString("AssemblerCondString");
SmallVector<StringRef, 4> Ops;
SplitString(AsmCondString, Ops, ",");
assert(!Ops.empty() && "AssemblerCondString cannot be empty");
for (auto &Op : Ops) {
assert(!Op.empty() && "Empty operator");
if (FeaturesMap.find(Op) == FeaturesMap.end())
FeaturesMap[Op] = FeaturesMap.size();
}
}
}Спробуйте самостійно знайти небезпечний код. А це картинка для відвернення уваги, щоб одразу не подивитися на відповідь:
Попередження PVS-Studio: [CWE-758] Висока construction використовується: 'FeaturesMap[Op] = FeaturesMap.size()', де 'FeaturesMap' є 'map' class. Це може бути спрямований на undefined behavior. RISCVCompressInstEmitter.cpp 490
Проблемний рядок:
FeaturesMap[Op] = FeaturesMap.size();Якщо елемент Op не знайдено, то створюється новий елемент у карті і туди записується кількість елементів у цій карті. Ось тільки невідомо, чи буде викликана функція розмір до або після додавання нового елемента.
Фрагмент N22-N24: Повторні надання
Error MachOObjectFile::checkSymbolTable() const {
....
} else {
MachO::nlist STE = getSymbolTableEntry(SymDRI);
NType = STE.n_type; // <=
NType = STE.n_type; // <=
NSect = STE.n_sect;
NDesc = STE.n_desc;
NStrx = STE.n_strx;
NValue = STE.n_value;
}
....
}Попередження PVS-Studio: [CWE-563] The 'NType' variable є визначеним значенням twice successively. Perhaps this is a mistake. Check lines: 1663, 1664. MachOObjectFile.cpp 1664
Думаю, справжньої помилки тут нема. Просто зайве повторюване привласнення. Але все одно ляп.
аналогічно:
- V519 [CWE-563] The 'B.NDesc' variable є визначеним значенням twice successively. Perhaps this is a mistake. Check lines: 1488, 1489. llvm-nm.cpp 1489
- V519 [CWE-563] Варіант є визначеним значенням twice успішно. Perhaps this is a mistake. Check lines: 59, 61. coff2yaml.cpp 61
Фрагмент N25-N27: Ще раз присвоювання
Тепер розглянемо трохи інший варіант повторного присвоєння.
bool Vectorizer::vectorizeLoadChain(
ArrayRef<Instruction *> Chain,
SmallPtrSet<Instruction *, 16> *InstructionsProcessed) {
....
unsigned Alignment = getAlignment(L0);
....
unsigned NewAlign = getOrEnforceKnownAlignment(L0->getPointerOperand(),
StackAdjustedAlignment,
DL, L0, nullptr, &DT);
if (NewAlign != 0)
Alignment = NewAlign;
Alignment = NewAlign;
....
}Попередження PVS-Studio: V519 [CWE-563] The 'Alignment' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1158, 1160. LoadStoreVectorizer.cpp 1160
Це дуже дивний код, який, очевидно, містить логічну помилку. На початку, змінної Вирівнювання надається значення залежно від умови. А потім знову відбувається привласнення, але тепер уже без жодної перевірки.
Аналогічні ситуації можна побачити тут:
- V519 [CWE-563] The 'Effects' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 152, 165. WebAssemblyRegStackify.cpp 165
- V519 [CWE-563] The 'ExpectNoDerefChunk' variable є визначеним значенням twice successively. Perhaps this is a mistake. Check lines: 4970, 4973. SemaType.cpp 4973
Фрагмент N28: Завжди дійсна умова
static int readPrefixes(struct InternalInstruction* insn) {
....
uint8_t byte = 0;
uint8_t nextByte;
....
if (byte == 0xf3 && (nextByte == 0x88 || nextByte == 0x89 ||
nextByte == 0xc6 || nextByte == 0xc7)) {
insn->xAcquireRelease = true;
if (nextByte != 0x90) // PAUSE instruction support // <=
break;
}
....
}Попередження PVS-Studio: [CWE-571] Expression 'nextByte! = 0x90' is always true. X86DisassemblerDecoder.cpp 379
Перевірка не має сенсу. Змінна nextByte завжди не дорівнює значенню 0x90що випливає з попередньої перевірки. Це якась логічна помилка.
Фрагмент N29 — N…: Завжди дійсні/хибні умови
Аналізатор видає багато попереджень про те, що вся умова () або його частина () завжди істинно чи хибно. Часто це не справжні помилки, а просто неакуратний код, результат розгортання макросів тощо. Тим не менш, є сенс подивитися всі ці попередження, оскільки іноді зустрічаються реальні логічні помилки. Наприклад, підозріла ось ця ділянка коду:
static DecodeStatus DecodeGPRPairRegisterClass(MCInst &Inst, unsigned RegNo,
uint64_t Address, const void *Decoder) {
DecodeStatus S = MCDisassembler::Success;
if (RegNo > 13)
return MCDisassembler::Fail;
if ((RegNo & 1) || RegNo == 0xe)
S = MCDisassembler::SoftFail;
....
}Попередження PVS-Studio: [CWE-570] Apart of conditional expression is always false: RegNo == 0xe. ARMDisassembler.cpp 939
Константа 0xE це значення 14 у десятковій системі. Перевірка RegNo == 0xe немає сенсу, оскільки якщо RegNo > 13, то функція завершить виконання.
Було безліч інших попереджень з ідентифікатором V547 та V560, але, як і у випадку з вивчати ці попередження мені було нецікаво. Було й так зрозуміло, що мені вистачить матеріалу для написання статті. Тому невідомо скільки всього можна виявити помилок цього типу в LLVM за допомогою PVS-Studio.
Наведу приклад, чому вивчати ці спрацьовування нудно. Аналізатор має рацію, видаючи попередження на наступний код. Але це не помилка.
bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
tok::TokenKind ClosingBraceKind) {
bool HasError = false;
....
HasError = true;
if (!ContinueOnSemicolons)
return !HasError;
....
}Попередження PVS-Studio: V547 [CWE-570] Expression '!HasError' is always false. UnwrappedLineParser.cpp 1635
Фрагмент N30: підозрілий return
static bool
isImplicitlyDef(MachineRegisterInfo &MRI, unsigned Reg) {
for (MachineRegisterInfo::def_instr_iterator It = MRI.def_instr_begin(Reg),
E = MRI.def_instr_end(); It != E; ++It) {
return (*It).isImplicitDef();
}
....
}Попередження PVS-Studio: [CWE-670] An unconditional 'return' within a loop. R600OptimizeVectorRegisters.cpp 63
Це або помилка, або специфічний прийом, який має щось пояснити програмістам, які читають код. Мені така конструкція нічого не пояснює та виглядає дуже підозрілою. Краще так не писати:).
Втомилися? Тоді час заварити чай чи каву.
Дефекти, виявлені новими діагностиками
Думаю, 30 спрацьовувань старих діагностик достатньо. Давайте тепер подивимося, що цікавого можна знайти новими діагностиками, які з'явилися в аналізаторі вже після перевірки. Усього цей час у C++ аналізаторі додалося 66 діагностик загального призначення.
Фрагмент N31: Недоступний код
Error CtorDtorRunner::run() {
....
if (auto CtorDtorMap =
ES.lookup(JITDylibSearchList({{&JD, true}}), std::move(Names),
NoDependenciesToRegister, true))
{
....
return Error::success();
} else
return CtorDtorMap.takeError();
CtorDtorsByPriority.clear();
return Error::success();
}Попередження PVS-Studio: [CWE-561] Unreachable code detected. It is possible that an error is present. ExecutionUtils.cpp 146
Як бачите, обидві гілки оператора if закінчуються викликом оператора повертати. Відповідно, контейнер CtorDtorsByPriority ніколи не буде очищений.
Фрагмент N32: Недоступний код
bool LLParser::ParseSummaryEntry() {
....
switch (Lex.getKind()) {
case lltok::kw_gv:
return ParseGVEntry(SummaryID);
case lltok::kw_module:
return ParseModuleEntry(SummaryID);
case lltok::kw_typeid:
return ParseTypeIdEntry(SummaryID); // <=
break; // <=
default:
return Error(Lex.getLoc(), "unexpected summary kind");
}
Lex.setIgnoreColonInIdentifiers(false); // <=
return false;
}Попередження PVS-Studio: V779 [CWE-561] Unreachable code detected. It is possible that an error is present. LLParser.cpp 835
Цікава ситуація. Давайте розглянемо на початку ось це місце:
return ParseTypeIdEntry(SummaryID);
break;На перший погляд, здається, що помилки тут немає. Схоже, що оператор перерву тут зайвий і його можна просто видалити. Однак не все так просто.
Аналізатор видає попередження на рядки:
Lex.setIgnoreColonInIdentifiers(false);
return false;Цей код недосяжний. Всі випадки у перемикання закінчуються викликом оператором повертати. І тепер безглуздий самотній перерву не виглядає таким невинним! Можливо одна з гілок повинна закінчуватися на перерву, А не на повертати?
Фрагмент N33: Випадкове обнулення старших біт
unsigned getStubAlignment() override {
if (Arch == Triple::systemz)
return 8;
else
return 1;
}
Expected<unsigned>
RuntimeDyldImpl::emitSection(const ObjectFile &Obj,
const SectionRef &Section,
bool IsCode) {
....
uint64_t DataSize = Section.getSize();
....
if (StubBufSize > 0)
DataSize &= ~(getStubAlignment() - 1);
....
}Попередження PVS-Studio: Середня частина битої маски є меншою, ніж розмір першої операційної. Це буде спричинити високі битви. RuntimeDyld.cpp 815
Зверніть увагу, що функція getStubAlignment повертає тип непідписаний. Обчислимо значення виразу, якщо припустити, що функція поверне значення 8:
~(getStubAlignment() - 1)
~(8u-1)
0xFFFFFFF8u
Тепер зверніть увагу, що змінна DataSize має 64-бітний беззнаковий тип. Виходить, що при виконанні операції DataSize & 0xFFFFFFF8u всі тридцять два старші біти будуть обнулені. Швидше за все це не те, що хотів програміст. Підозрюю, що він хотів обчислити: DataSize & 0xFFFFFFFFFFFFFFF8u.
Щоб виправити помилку, слід написати так:
DataSize &= ~(static_cast<uint64_t>(getStubAlignment()) - 1);Або так:
DataSize &= ~(getStubAlignment() - 1ULL);Фрагмент N34: Невдале явне наведення типу
template <typename T>
void scaleShuffleMask(int Scale, ArrayRef<T> Mask,
SmallVectorImpl<T> &ScaledMask) {
assert(0 < Scale && "Unexpected scaling factor");
int NumElts = Mask.size();
ScaledMask.assign(static_cast<size_t>(NumElts * Scale), -1);
....
}Попередження PVS-Studio: [CWE-190] Можливий overflow. Вважається, що гравець керує 'NumElts * Scale' функціонал до 'type_size_t', не є результатом. X86ISelLowering.h 1577
Явне приведення типу використовується для того, щоб не виникло переповнення при перемноженні змінних типу Int. Однак тут явне приведення типу не захищає від переповнення. На початку змінні будуть перемножені, і тільки потім 32-бітовий результат множення буде розширено до типу .
Фрагмент N35: Невдалий Copy-Paste
Instruction *InstCombiner::visitFCmpInst(FCmpInst &I) {
....
if (!match(Op0, m_PosZeroFP()) && isKnownNeverNaN(Op0, &TLI)) {
I.setOperand(0, ConstantFP::getNullValue(Op0->getType()));
return &I;
}
if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
I.setOperand(1, ConstantFP::getNullValue(Op0->getType())); // <=
return &I;
}
....
}[CWE-682] Two similar code fragments were found. Perhaps, this is a typo and 'Op1' variable should be used instead of 'Op0'. InstCombineCompares.cpp 5507
Ця нова цікава діагностика виявляє ситуації, коли фрагмент коду було скопійовано, і в ньому почали змінювати деякі імена, але в одному місці не виправили.
Зверніть увагу, що у другому блоці міняли Op0 на Op1. Але в одному місці не виправили. Швидше за все, мало бути написано так:
if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
I.setOperand(1, ConstantFP::getNullValue(Op1->getType()));
return &I;
}Фрагмент N36: Плутанина у змінних
struct Status {
unsigned Mask;
unsigned Mode;
Status() : Mask(0), Mode(0){};
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
Mode &= Mask;
};
....
};Попередження PVS-Studio: [CWE-563] 'Mode' variable є визначеним, але не використовуваний для завершення функції. SIModeRegister.cpp 48
Дуже небезпечно давати аргументам функцій ті самі імена, що й членам класу. Дуже легко заплутатися. Перед нами якраз такий випадок. Цей вислів немає сенсу:
Mode &= Mask;Змінюється аргумент функції. І все. Цей аргумент більше не використовується. Швидше за все, треба було написати так:
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
this->Mode &= Mask;
};Фрагмент N37: Плутанина у змінних
class SectionBase {
....
uint64_t Size = 0;
....
};
class SymbolTableSection : public SectionBase {
....
};
void SymbolTableSection::addSymbol(Twine Name, uint8_t Bind, uint8_t Type,
SectionBase *DefinedIn, uint64_t Value,
uint8_t Visibility, uint16_t Shndx,
uint64_t Size) {
....
Sym.Value = Value;
Sym.Visibility = Visibility;
Sym.Size = Size;
Sym.Index = Symbols.size();
Symbols.emplace_back(llvm::make_unique<Symbol>(Sym));
Size += this->EntrySize;
}Попередження PVS-Studio: V1001 [CWE-563] The 'Size' variable є визначеним, але він не буде використовуватися для завершення функції. Object.cpp 424
Ситуація аналогічна до попередньої. Має бути написано:
this->Size += this->EntrySize;Фрагмент N38-N47: Покажчик забули перевірити
Раніше ми розглядали приклади спрацьовування діагностики . Її суть у тому, що покажчик на початку розіменовується, а потім перевіряється. Молода діагностика є оберненою їй за змістом, але також виявляє дуже багато помилок. Вона виявляє ситуації, коли покажчик спочатку перевіряли, а потім забули це зробити. Розглянемо такі випадки, знайдені усередині LLVM.
int getGEPCost(Type *PointeeType, const Value *Ptr,
ArrayRef<const Value *> Operands) {
....
if (Ptr != nullptr) { // <=
assert(....);
BaseGV = dyn_cast<GlobalValue>(Ptr->stripPointerCasts());
}
bool HasBaseReg = (BaseGV == nullptr);
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType()); // <=
....
}Попередження PVS-Studio: V1004 [CWE-476] The 'Ptr' pointer був використаний unsafely after it був verified against nullptr. Check lines: 729, 738. TargetTransformInfoImpl.h 738
Мінлива Ptr може бути рівна nullptr, про що свідчить перевірка:
if (Ptr != nullptr)Однак нижче цей покажчик розіменовується вже без попередньої перевірки:
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());Розглянемо інший аналогічний випадок.
llvm::DISubprogram *CGDebugInfo::getFunctionFwdDeclOrStub(GlobalDecl GD,
bool Stub) {
....
auto *FD = dyn_cast<FunctionDecl>(GD.getDecl());
SmallVector<QualType, 16> ArgTypes;
if (FD) // <=
for (const ParmVarDecl *Parm : FD->parameters())
ArgTypes.push_back(Parm->getType());
CallingConv CC = FD->getType()->castAs<FunctionType>()->getCallConv(); // <=
....
}Попередження PVS-Studio: V1004 [CWE-476] The 'FD' pointer був використаний unsafely after it was verified against nullptr. Check lines: 3228, 3231. CGDebugInfo.cpp 3231
Зверніть увагу на вказівник FD. Впевнений, що проблема добре видно, і спеціальних пояснень не потрібно.
І ще:
static void computePolynomialFromPointer(Value &Ptr, Polynomial &Result,
Value *&BasePtr,
const DataLayout &DL) {
PointerType *PtrTy = dyn_cast<PointerType>(Ptr.getType());
if (!PtrTy) { // <=
Result = Polynomial();
BasePtr = nullptr;
}
unsigned PointerBits =
DL.getIndexSizeInBits(PtrTy->getPointerAddressSpace()); // <=
....
}Попередження PVS-Studio: V1004 [CWE-476] The 'PtrTy' pointer був використаний unsafely after it was verified against nullptr. Check lines: 960, 965. InterleavedLoadCombinePass.cpp 965
Як захиститись від таких помилок? Будьте уважнішими на Code-Review та використовуйте для регулярної перевірки коду статичний аналізатор PVS-Studio.
Наводити інші фрагменти коду з помилками цього виду сенсу немає. Залишу в статті лише список попереджень:
- V1004 [CWE-476] The 'Expr' pointer був використаний unsafely after it був verified against nullptr. Check lines: 1049, 1078. DebugInfoMetadata.cpp 1078
- V1004 [CWE-476] The 'PI' pointer був використаний unsafely after it був verified against nullptr. Check lines: 733, 753. LegacyPassManager.cpp 753
- V1004 [CWE-476] The 'StatepointCall' pointer був використаний unsafely after it був verified against nullptr. Check lines: 4371, 4379. Verifier.cpp 4379
- V1004 [CWE-476] The 'RV' pointer був використаний unsafely after it був verified against nullptr. Check lines: 2263, 2268. TGParser.cpp 2268
- V1004 [CWE-476] The 'CalleeFn' pointer був використаний unsafely after it був verified against nullptr. Check lines: 1081, 1096. SimplifyLibCalls.cpp 1096
- V1004 [CWE-476] The 'TC' pointer був використаний unsafely after it був verified against nullptr. Check lines: 1819, 1824. Driver.cpp 1824
Фрагмент N48-N60: Не критично, але дефект (можливий витік пам'яті)
std::unique_ptr<IRMutator> createISelMutator() {
....
std::vector<std::unique_ptr<IRMutationStrategy>> Strategies;
Strategies.emplace_back(
new InjectorIRStrategy(InjectorIRStrategy::getDefaultOps()));
....
}Попередження PVS-Studio: [CWE-460] A pointer withowner is added to 'Strategies' container by 'emplace_back' метод. A memory leak will occur in case of an exception. llvm-isel-fuzzer.cpp 58
Для додавання елемента до кінця контейнера типу std::vector > не можна просто написати xxx.push_back(new X), так як немає неявного перетворення з X* в std::unique_ptr.
Поширеним рішенням є написання xxx.emplace_back(new X), оскільки він компілюється: метод emplace_back конструює елемент безпосередньо з аргументів і може використовувати явні конструктори.
Це не безпечно. Якщо вектор повний, відбувається перевиділення пам'яті. Операція перевиділення пам'яті може закінчитися невдачею, внаслідок чого буде згенеровано виняток std::bad_alloc. У цьому випадку вказівник буде втрачено, і створений об'єкт ніколи не буде видалено.
Безпечним рішенням є створення unique_ptr, який володітиме покажчиком до того, як вектор спробує перевиділити пам'ять:
xxx.push_back(std::unique_ptr<X>(new X))Починаючи з C++14, можна використовувати 'std::make_unique':
xxx.push_back(std::make_unique<X>())Даний вид дефекту не є критичним для LLVM. Якщо не вдасться виділити пам'ять, то компілятор буде просто зупинено. Однак, для додатків з довгим , які не можуть просто так завершуватися, якщо не вдалося виділити пам'ять, це може бути справжньою неприємною помилкою.
Отже, хоча цей код не становить практичної небезпеки для LLVM, я вважав за корисне розповісти про цей патерн помилок і що аналізатор PVS-Studio навчився його виявляти.
Інші попередження цього типу:
- V1023 [CWE-460] Показник без власника приєднаний до 'Passes' container by 'emplace_back' метод. A memory leak will occur in case of an exception. PassManager.h 546
- V1023 [CWE-460] Показник без власника приєднаний до 'AAs' container by 'emplace_back' метод. A memory leak will occur in case of an exception. AliasAnalysis.h 324
- V1023 [CWE-460] Показник без власника приєднаний до 'Entries' container by 'emplace_back' метод. A memory leak will occur in case of an exception. DWARFDebugFrame.cpp 519
- V1023 [CWE-460] Показник без власника приєднаний до 'AllEdges' container by 'emplace_back' метод. A memory leak will occur in case of an exception. CFGMST.h 268
- V1023 [CWE-460] Показник без власника приєднаний до 'VMaps' container by 'emplace_back' метод. A memory leak will occur in case of an exception. SimpleLoopUnswitch.cpp 2012
- V1023 [CWE-460] Показник без власника приєднаний до 'Records' container by 'emplace_back' метод. A memory leak will occur in case of an exception. FDRLogBuilder.h 30
- V1023 [CWE-460] Показник без власника приєднаний до 'PendingSubmodules' container by 'emplace_back' метод. A memory leak will occur in case of an exception. ModuleMap.cpp 810
- V1023 [CWE-460] Показник без власника приєднаний до 'Objects' container by 'emplace_back' метод. A memory leak will occur in case of an exception. DebugMap.cpp 88
- V1023 [CWE-460] Показник без власника приєднаний до 'Strategies' container by 'emplace_back' метод. A memory leak will occur in case of an exception. llvm-isel-fuzzer.cpp 60
- V1023 [CWE-460] Показник без власника приєднаний до 'Modifiers' container by 'emplace_back' метод. A memory leak will occur in case of an exception. llvm-stress.cpp 685
- V1023 [CWE-460] Показник без власника приєднаний до 'Modifiers' container by 'emplace_back' метод. A memory leak will occur in case of an exception. llvm-stress.cpp 686
- V1023 [CWE-460] Показник без власника приєднаний до 'Modifiers' container by 'emplace_back' метод. A memory leak will occur in case of an exception. llvm-stress.cpp 688
- V1023 [CWE-460] Показник без власника приєднаний до 'Modifiers' container by 'emplace_back' метод. A memory leak will occur in case of an exception. llvm-stress.cpp 689
- V1023 [CWE-460] Показник без власника приєднаний до 'Modifiers' container by 'emplace_back' метод. A memory leak will occur in case of an exception. llvm-stress.cpp 690
- V1023 [CWE-460] Показник без власника приєднаний до 'Modifiers' container by 'emplace_back' метод. A memory leak will occur in case of an exception. llvm-stress.cpp 691
- V1023 [CWE-460] Показник без власника приєднаний до 'Modifiers' container by 'emplace_back' метод. A memory leak will occur in case of an exception. llvm-stress.cpp 692
- V1023 [CWE-460] Показник без власника приєднаний до 'Modifiers' container by 'emplace_back' метод. A memory leak will occur in case of an exception. llvm-stress.cpp 693
- V1023 [CWE-460] Показник без власника приєднаний до 'Modifiers' container by 'emplace_back' метод. A memory leak will occur in case of an exception. llvm-stress.cpp 694
- V1023 [CWE-460] Показник без власника приєднаний до 'Operands' container by 'emplace_back' метод. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 1911
- V1023 [CWE-460] Показник без власника приєднаний до 'Stash' container by 'emplace_back' метод. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 2100
- V1023 [CWE-460] Показник без власника приєднаний до 'Matchers' container by 'emplace_back' метод. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 2702
Висновок
Загалом я виписав 60 попереджень, після чого зупинився. Чи є інші дефекти, які виявляє у LLVM аналізатор PVS-Studio? Так є. Однак, коли я виписував фрагменти коду для статті, настав пізній вечір, вірніше, навіть ніч, і я вирішив, що настав час закруглюватися.
Сподіваюся, вам було цікаво і ви захочете спробувати аналізатор PVS-Studio.
Ви можете завантажити аналізатор і отримати тральний ключ на .
Найголовніше, використовуйте статичний аналіз регулярно. Разові перевірки, що виконуються нами з метою популяризації методології статичного аналізу та PVS-Studio не є нормальним сценарієм.
Успіхів у покращенні якості та надійності коду!
Якщо хочете поділитися цією статтею з англомовною аудиторією, прошу використати посилання на переклад: Andrey Karpov. .
Джерело: habr.com