Поминаа повеќе од две години од последната проверка на кодот на проектот LLVM со помош на нашиот анализатор PVS-Studio. Да се увериме дека анализаторот PVS-Studio сè уште е водечка алатка за идентификување на грешки и потенцијални пропусти. За да го направите ова, ќе провериме и ќе пронајдеме нови грешки во изданието LLVM 8.0.0.
Статијата што треба да се напише
Да бидам искрен, не сакав да ја напишам оваа статија. Не е интересно да се пишува за проект што веќе го проверивме неколку пати (, , ). Подобро е да пишувам за нешто ново, но немам избор.
Секој пат кога се објавува или ажурира нова верзија на LLVM , добиваме прашања од следниот тип во нашата пошта:
Погледнете, новата верзија на Clang Static Analyzer научи да наоѓа нови грешки! Ми се чини дека релевантноста за користење на PVS-Studio се намалува. Кланг наоѓа повеќе грешки отколку порано и ги достигнува можностите на PVS-Studio. Што мислиш ЗА ова?
На ова секогаш сакам да одговорам нешто како:
И ние не седиме без работа! Значително ги подобривме можностите на анализаторот PVS-Studio. Затоа, не грижете се, ние продолжуваме да водиме како и досега.
За жал, ова е лош одговор. Во него нема никакви докази. И затоа ја пишувам оваа статија сега. Значи, проектот 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] Има идентични подизрази „Name.startswith("avx512.mask.permvar.")" лево и десно од '||' оператор. Автоматска надградба.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: Променливата 'Identifier->Type' е доделена на самата себе. 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] Размислете за проверка на изјавата „прекинувач“. Можно е да недостасува првиот оператор за „случај“. 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
Покажувач Кали на почетокот се дереференцира во моментот кога се повикува функцијата 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] Размислете за проверка на изразот '~(Големина - 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] Низите беа споени, но не се користат. Размислете за проверка на изразот „Result + Name.str()“. Симбол.cpp 32
- V655 [CWE-480] Низите беа споени, но не се користат. Размислете за проверка на изразот „Резултат + „(Класа ObjC)“ + Име.str()“. Симбол.cpp 35
- V655 [CWE-480] Низите беа споени, но не се користат. Размислете за проверка на изразот „Резултат + „(ObjC Класа EH)“ + Име.str()“. Симбол.cpp 38
- V655 [CWE-480] Низите беа споени, но не се користат. Размислете за проверка на изразот „Резултат + „(ObjC IVar)“ + Име.str()“. Симбол.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] На променливата „Порамнување“ и се доделуваат вредности двапати последователно. Можеби ова е грешка. Проверете ги линиите: 1158, 1160. LoadStoreVetorizer.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
Проверувањето нема смисла. Променлива следниот Бајт секогаш не е еднаква на вредноста 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 во децимални. Испитување RegNo == 0xe нема смисла бидејќи ако RegNo > 13, тогаш функцијата ќе го заврши своето извршување.
Имаше многу други предупредувања со ID 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“ е секогаш неточен. 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
Сега забележи дека променливата Големина на податоци има 64-битен непотпишан тип. Излегува дека при извршување на операцијата DataSize & 0xFFFFFFF8u, сите триесет и два бита од висок ред ќе се ресетираат на нула. Најверојатно, ова не е она што го сакаше програмерот. Се сомневам дека сакал да пресмета: DataSize & 0xFFFFFFFFFFFFFFFFFF8u.
За да ја поправите грешката, треба да го напишете ова:
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', а не резултатот. X86ISelSuling.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
Оваа нова интересна дијагностика идентификува ситуации кога дел од кодот е копиран и некои имиња во него почнале да се менуваат, но на едно место не го коригирале.
Забележете дека во вториот блок тие се сменија Оп0 на Оп1. Но, на едно место не го поправија. Најверојатно требало да биде напишано вака:
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] Променливата „Големина“ е доделена, но не се користи до крајот на функцијата. Објект.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
Променлива Птр може да бидат еднакви 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::вектор > не можеш само да пишуваш xxx.push_back (нов X), бидејќи не постои имплицитна конверзија од X* в std::unique_ptr.
Вообичаено решение е да се напише xxx.emplace_back (нов 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“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. PassManager.h 546
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „AA“ со методот „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] Покажувач без сопственик се додава во контејнерот „Modifiers“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. llvm-stress.cpp 685
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „Modifiers“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. llvm-stress.cpp 686
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „Modifiers“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. llvm-stress.cpp 688
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „Modifiers“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. llvm-stress.cpp 689
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „Modifiers“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. llvm-stress.cpp 690
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „Modifiers“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. llvm-stress.cpp 691
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „Modifiers“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. llvm-stress.cpp 692
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „Modifiers“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. llvm-stress.cpp 693
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „Modifiers“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. llvm-stress.cpp 694
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „Operands“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. GlobalISelEmitter.cpp 1911 година
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „Stash“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. GlobalISelEmitter.cpp 2100
- V1023 [CWE-460] Покажувач без сопственик се додава во контејнерот „Matchers“ со методот „emplace_back“. Во случај на исклучок ќе дојде до истекување на меморијата. GlobalISelEmitter.cpp 2702
Заклучок
Издадов вкупно 60 опомени и потоа прекинав. Дали има други дефекти што анализаторот PVS-Studio ги открива во LLVM? Да, имам. Меѓутоа, кога пишував фрагменти од код за статијата, беше доцна навечер, поточно дури и ноќ, и решив дека е време да го наречам ден.
Се надевам дека ви беше интересно и ќе сакате да го пробате анализаторот PVS-Studio.
Можете да го преземете анализаторот и да го добиете клучот за миночистач на .
Што е најважно, редовно користете статичка анализа. Еднократни проверки, спроведена од нас со цел да се популаризира методологијата на статичка анализа и PVS-Studio не се нормално сценарио.
Среќно во подобрувањето на квалитетот и сигурноста на вашиот код!
Ако сакате да ја споделите оваа статија со публика што зборува англиски, ве молиме користете ја врската за превод: Андреј Карпов. .
Извор: www.habr.com