Изминаха повече от две години от последната проверка на кода на проекта LLVM с помощта на нашия анализатор PVS-Studio. Нека се уверим, че анализаторът на PVS-Studio все още е водещ инструмент за идентифициране на грешки и потенциални уязвимости. За да направим това, ще проверим и ще намерим нови грешки в изданието LLVM 8.0.0.
Статия, която трябва да бъде написана
Честно казано, не исках да пиша тази статия. Не е интересно да пишем за проект, който вече сме проверили няколко пъти (, , ). По-добре е да пиша за нещо ново, но нямам избор.
Всеки път, когато се пусне или актуализира нова версия на LLVM , получаваме въпроси от следния тип в нашата поща:
Вижте, новата версия на Clang Static Analyzer се научи да намира нови грешки! Струва ми се, че уместността на използването на PVS-Studio намалява. Clang открива повече грешки от преди и се изравнява с възможностите на PVS-Studio. Какво мислиш за това?
На това винаги искам да отговоря нещо като:
Ние също не седим без работа! Значително подобрихме възможностите на анализатора PVS-Studio. Така че не се притеснявайте, ние продължаваме да водим както преди.
За съжаление, това е лош отговор. В него няма никакви доказателства. И затова пиша тази статия сега. И така, проектът LLVM беше проверен отново и в него бяха открити различни грешки. Сега ще демонстрирам тези, които ми се сториха интересни. Статичният анализатор на Clang не може да намери тези грешки (или е изключително неудобно да го направите с негова помощ). Но можем. Освен това намерих и записах всички тези грешки за една вечер.
Но писането на статията отне няколко седмици. Просто не можах да се накарам да вложа всичко това в текст :).
Между другото, ако се интересувате какви технологии се използват в анализатора на PVS-Studio за идентифициране на грешки и потенциални уязвимости, тогава предлагам да се запознаете с това .
Нова и стара диагностика
Както вече беше отбелязано, преди около две години проектът LLVM беше проверен отново и откритите грешки бяха коригирани. Сега тази статия ще представи нова група грешки. Защо бяха открити нови грешки? Има 3 причини за това:
- Проектът LLVM се развива, променя стар код и добавя нов код. Естествено, има нови грешки в модифицирания и написан код. Това ясно показва, че статичният анализ трябва да се използва редовно, а не от време на време. Нашите статии показват добре възможностите на анализатора PVS-Studio, но това няма нищо общо с подобряването на качеството на кода и намаляването на разходите за коригиране на грешки. Използвайте редовно анализатор на статичен код!
- Ние финализираме и подобряваме съществуващата диагностика. Следователно анализаторът може да идентифицира грешки, които не е забелязал по време на предишни сканирания.
- В PVS-Studio се появи нова диагностика, която не съществуваше преди 2 години. Реших да ги подчертая в отделен раздел, за да покажа ясно развитието на PVS-Studio.
Дефекти, установени с диагностика, съществуващи преди 2 години
Фрагмент N1: Копиране-поставяне
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] Има идентични подизрази 'Name.startswith("avx512.mask.permvar.")' отляво и отдясно на '||' оператор. 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 Има идентични подизрази „CXNameRange_WantQualifier“ отляво и отдясно на „|“ оператор. 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] Може би операторът '?:' работи по начин, различен от очаквания. Операторът „?:“ има по-нисък приоритет от оператора „==“. 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] Може да се извърши дерефериране на нулевия указател „LHS“. TGParser.cpp 2152
Ако показалецът LHS е нула, трябва да се издаде предупреждение. Вместо това обаче същият този нулев указател ще бъде дерефериран: LHS->getAsString().
Това е много типична ситуация, когато грешка е скрита в манипулатор на грешки, тъй като никой не ги тества. Статичните анализатори проверяват целия достъпен код, независимо колко често се използва. Това е много добър пример за това как статичният анализ допълва други техники за тестване и защита от грешки.
Подобна грешка при обработка на указател RHS позволено в кода точно по-долу: V522 [CWE-476] Може да има дерефериране на нулевия указател „RHS“. 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] Може да се извърши дерефериране на нулевия указател „ProgClone“. Грешно компилиране.cpp 601
В началото умна показалка ProgClone престава да притежава обекта:
BD.setNewProgram(std::move(ProgClone));Всъщност сега ProgClone е нулев указател. Следователно дереференцирането на нулев указател трябва да се появи точно под:
Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);Но в действителност това няма да се случи! Имайте предвид, че цикълът всъщност не се изпълнява.
В началото на контейнера Неправилно компилирани функции изчистено:
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] Може да се извърши дерефериране на нулевия указател „Тест“. Грешно компилиране.cpp 709
Отново същата ситуация. Първоначално съдържанието на обекта се премества, а след това се използва, сякаш нищо не се е случило. Виждам тази ситуация все по-често в програмния код, след като семантиката на движението се появи в C++. Ето защо обичам езика 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] Може да се извърши дерефериране на нулевия указател „Тип“. 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] Може да се извърши дерефериране на нулевия указател „Ty“. 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 предупреждение: Променливата „Идентификатор->Тип“ се присвоява сама на себе си. FormatTokenLexer.cpp 249
Няма смисъл да присвоявате променлива сама на себе си. Най-вероятно са искали да напишат:
Identifier->Type = Question->Type;Фрагмент N11: Подозрително счупване
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] Помислете за проверка на израза „превключване“. Възможно е първият оператор 'case' да липсва. 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] Указателят „Callee“ беше използван, преди да бъде проверен срещу nullptr. Проверете редове: 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] Указателят „CalleeFn“ е използван, преди да бъде проверен срещу nullptr. Проверете редове: 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] Указателят 'ND' е използван, преди да бъде проверен срещу nullptr. Проверете редове: 532, 534. SemaTemplateInstantiateDecl.cpp 532
И тук:
- V595 [CWE-476] Указателят 'U' беше използван, преди да бъде проверен срещу nullptr. Проверете редове: 404, 407. DWARFormValue.cpp 404
- V595 [CWE-476] Указателят 'ND' беше използван, преди да бъде проверен срещу nullptr. Проверете редове: 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] Помислете за проверка на израза '~(Size - 1) << 1'. Преместване на битове на 32-битовата стойност с последващо разширение до 64-битов тип. AArch64AddressingModes.h 260
Възможно е да не е грешка и кодът да работи точно както е предназначен. Но това очевидно е много подозрително място и трябва да се провери.
Да кажем променливата Размер е равно на 16 и след това авторът на кода планира да го получи в променлива NImms стойност:
1111111111111111111111111111111111111111111111111111111111100000
Но в действителност резултатът ще бъде:
0000000000000000000000000000000011111111111111111111111111100000
Факт е, че всички изчисления се извършват с помощта на 32-битов неподписан тип. И едва тогава този 32-битов неподписан тип ще бъде имплицитно разширен до uint64_t. В този случай най-значимите битове ще бъдат нула.
Можете да коригирате ситуацията по следния начин:
uint64_t NImms = ~static_cast<uint64_t>(Size-1) << 1;Подобна ситуация: V629 [CWE-190] Помислете за проверка на израза „Immr << 6“. Преместване на битове на 32-битовата стойност с последващо разширение до 64-битов тип. 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] Помислете за проверка на логиката на приложението. Възможно е ключовата дума „друго“ да липсва. AMDGPUAsmParser.cpp 5655
Тук няма грешка. От тогавашния блок на първия 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] Низовете бяха свързани, но не се използват. Помислете за проверка на израза „Резултат + Име.str()“. Symbol.cpp 32
- V655 [CWE-480] Низовете бяха свързани, но не се използват. Помислете за проверка на израза „Резултат + „(ObjC Class)“ + Name.str()“. Symbol.cpp 35
- V655 [CWE-480] Низовете бяха свързани, но не се използват. Помислете за проверка на израза "Резултат + "(ObjC Class EH) " + Name.str()". Symbol.cpp 38
- V655 [CWE-480] Низовете бяха свързани, но не се използват. Помислете за проверка на израза „Резултат + „(ObjC IVar)“ + Name.str()“. 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] Използва се опасна конструкция: „FeaturesMap[Op] = FeaturesMap.size()“, където „FeaturesMap“ е от клас „map“. Това може да доведе до неопределено поведение. 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] На променливата „NType“ се присвояват стойности два пъти последователно. Може би това е грешка. Проверете редове: 1663, 1664. MachOObjectFile.cpp 1664
Не мисля, че тук има истинска грешка. Просто ненужна повторна задача. Но пак гаф.
По същия начин:
- V519 [CWE-563] На променливата 'B.NDesc' се присвояват стойности два пъти последователно. Може би това е грешка. Проверете редове: 1488, 1489. llvm-nm.cpp 1489
- V519 [CWE-563] На променливата се присвояват стойности два пъти последователно. Може би това е грешка. Проверете редове: 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] На променливата „Alignment“ се присвояват стойности два пъти последователно. Може би това е грешка. Проверете редове: 1158, 1160. LoadStoreVectorizer.cpp 1160
Това е много странен код, който очевидно съдържа логическа грешка. В началото променлива Подравняване присвоява се стойност в зависимост от състоянието. И тогава присвояването се случва отново, но вече без никаква проверка.
Подобни ситуации можете да видите тук:
- V519 [CWE-563] На променливата „Ефекти“ се присвояват стойности два пъти последователно. Може би това е грешка. Проверете редове: 152, 165. WebAssemblyRegStackify.cpp 165
- V519 [CWE-563] На променливата 'ExpectNoDerefChunk' се присвояват стойности два пъти последователно. Може би това е грешка. Проверете редове: 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] Изразът 'nextByte != 0x90' винаги е верен. 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] Част от условния израз винаги е невярна: RegNo == 0xe. ARMDisassembler.cpp 939
Константата 0xE е десетичната стойност 14. Преглед Рег. № == 0xe няма смисъл, защото ако Рег. № > 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] Изразът '!HasError' винаги е false. UnwrappedLineParser.cpp 1635
Фрагмент N30: Подозрително връщане
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] Безусловно „връщане“ в рамките на цикъл. 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] Открит е недостъпен код. Възможно е да има грешка. 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] Открит е недостъпен код. Възможно е да има грешка. 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)
0xFFFFFFFF8u
Сега забележете, че променливата DataSize има 64-битов неподписан тип. Оказва се, че при извършване на операцията DataSize & 0xFFFFFFF8u, всичките тридесет и два бита от висок ред ще бъдат нулирани. Най-вероятно това не е това, което програмистът е искал. Подозирам, че е искал да изчисли: DataSize & 0xFFFFFFFFFFFFFFFF8u.
За да коригирате грешката, трябва да напишете това:
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] Възможно преливане. Помислете за кастинг на операндите на оператора „NumElts * Scale“ към типа „size_t“, а не към резултата. X86ISelLowering.h 1577
Използва се изрично прехвърляне на тип, за да се избегне препълване при умножаване на променливи на тип Int. Изричното преобразуване на типове тук обаче не предпазва от препълване. Първо променливите ще бъдат умножени и едва след това 32-битовият резултат от умножението ще бъде разширен до типа .
Фрагмент N35: Неуспешно копиране-поставяне
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] Бяха намерени два подобни кодови фрагмента. Може би това е правописна грешка и трябва да се използва променлива 'Op1' вместо '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' е присвоена, но не се използва до края на функцията. 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] Променливата 'Size' е присвоена, но не се използва до края на функцията. 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] Указателят „Ptr“ е използван небезопасно, след като е проверен срещу nullptr. Проверете редове: 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] Указателят „FD“ е използван небезопасно, след като е проверен срещу nullptr. Проверете редове: 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] Указателят „PtrTy“ е използван небезопасно, след като е проверен срещу nullptr. Проверете редове: 960, 965. InterleavedLoadCombinePass.cpp 965
Как да се предпазите от подобни грешки? Бъдете по-внимателни в Code-Review и използвайте статичния анализатор на PVS-Studio, за да проверявате редовно кода си.
Няма смисъл да се цитират други кодови фрагменти с грешки от този тип. Ще оставя само списък с предупреждения в статията:
- V1004 [CWE-476] Указателят „Expr“ е използван небезопасно, след като е проверен срещу nullptr. Проверете редове: 1049, 1078. DebugInfoMetadata.cpp 1078
- V1004 [CWE-476] Указателят „PI“ е използван небезопасно, след като е проверен срещу nullptr. Проверете редове: 733, 753. LegacyPassManager.cpp 753
- V1004 [CWE-476] Указателят „StatepointCall“ беше използван небезопасно, след като беше проверен срещу nullptr. Проверете редове: 4371, 4379. Verifier.cpp 4379
- V1004 [CWE-476] Указателят „RV“ е използван небезопасно, след като е проверен срещу nullptr. Проверете редове: 2263, 2268. TGParser.cpp 2268
- V1004 [CWE-476] Указателят „CalleeFn“ е използван небезопасно, след като е проверен срещу nullptr. Проверете редове: 1081, 1096. SimplifyLibCalls.cpp 1096
- V1004 [CWE-476] Указателят „TC“ е използван небезопасно, след като е проверен срещу nullptr. Проверете редове: 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] Указател без собственик се добавя към контейнера „Стратегии“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. llvm-isel-fuzzer.cpp 58
За да добавите елемент в края на контейнер като std::vector > не можеш просто да пишеш xxx.push_back(нов X), тъй като няма имплицитно преобразуване от X* в std::unique_ptr.
Често срещано решение е да пишете xxx.emplace_back(нов X)тъй като компилира: метод emplace_back конструира елемент директно от неговите аргументи и следователно може да използва изрични конструктори.
Не е безопасно. Ако векторът е пълен, тогава паметта се преразпределя. Операцията за преразпределение на паметта може да се провали, което да доведе до хвърляне на изключение std::bad_alloc. В този случай указателят ще бъде загубен и създаденият обект никога няма да бъде изтрит.
Безопасно решение е да създавате уникален_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] Указател без собственик се добавя към контейнера „Пропуски“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. PassManager.h 546
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „AAs“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. AliasAnalysis.h 324
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Entries“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. DWARFDebugFrame.cpp 519
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „AllEdges“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. CFGMST.h 268
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „VMaps“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. SimpleLoopUnswitch.cpp 2012
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Записи“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. FDRLogBuilder.h 30
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „PendingSubmodules“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. ModuleMap.cpp 810
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Обекти“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. DebugMap.cpp 88
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Стратегии“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. llvm-isel-fuzzer.cpp 60
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Модификатори“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. llvm-stress.cpp 685
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Модификатори“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. llvm-stress.cpp 686
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Модификатори“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. llvm-stress.cpp 688
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Модификатори“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. llvm-stress.cpp 689
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Модификатори“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. llvm-stress.cpp 690
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Модификатори“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. llvm-stress.cpp 691
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Модификатори“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. llvm-stress.cpp 692
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Модификатори“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. llvm-stress.cpp 693
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Модификатори“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. llvm-stress.cpp 694
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Операнди“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. GlobalISelEmitter.cpp 1911
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Stash“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. GlobalISelEmitter.cpp 2100
- V1023 [CWE-460] Указател без собственик се добавя към контейнера „Съвпадения“ чрез метода „emplace_back“. В случай на изключение ще възникне изтичане на памет. GlobalISelEmitter.cpp 2702
Заключение
Издадох общо 60 предупреждения и след това спрях. Има ли други дефекти, които PVS-Studio анализаторът открива в LLVM? Да, имам. Въпреки това, когато пишех кодови фрагменти за статията, беше късно вечер или по-скоро дори нощ и реших, че е време да го нарека ден.
Надявам се, че ви е било интересно и ще искате да опитате анализатора на PVS-Studio.
Можете да изтеглите анализатора и да получите ключа за миночистач от .
Най-важното е редовно да използвате статичен анализ. Еднократни чекове, извършени от нас с цел популяризиране на методологията на статичния анализ и PVS-Studio не са нормален сценарий.
Успех в подобряването на качеството и надеждността на вашия код!
Ако искате да споделите тази статия с англоезична аудитория, моля, използвайте връзката за превод: Андрей Карпов. .
Източник: www.habr.com