Minęły ponad dwa lata od ostatniej kontroli kodu projektu LLVM przy użyciu naszego analizatora PVS-Studio. Zadbajmy o to, aby analizator PVS-Studio nadal był wiodącym narzędziem identyfikującym błędy i potencjalne podatności. W tym celu sprawdzimy i znajdziemy nowe błędy w wersji LLVM 8.0.0.
Artykuł do napisania
Szczerze mówiąc, nie chciałem pisać tego artykułu. Nie jest ciekawie pisać o projekcie, który sprawdzaliśmy już kilka razy (
Za każdym razem, gdy wydawana lub aktualizowana jest nowa wersja LLVM
Spójrz, nowa wersja Clang Static Analyzer nauczyła się znajdować nowe błędy! Wydaje mi się, że znaczenie korzystania z PVS-Studio maleje. Clang znajduje więcej błędów niż wcześniej i nadrabia zaległości w możliwościach PVS-Studio. Co o tym myślisz?
Na to zawsze chcę odpowiedzieć na coś takiego:
My też nie siedzimy bezczynnie! Znacząco poprawiliśmy możliwości analizatora PVS-Studio. Więc nie martw się, nadal prowadzimy tak jak wcześniej.
Niestety, jest to zła odpowiedź. Nie ma w nim żadnych dowodów. I dlatego teraz piszę ten artykuł. Zatem projekt LLVM został ponownie sprawdzony i znaleziono w nim szereg błędów. Pokażę teraz te, które wydały mi się interesujące. Clang Static Analyzer nie może znaleźć tych błędów (lub jest to wyjątkowo niewygodne przy jego pomocy). Ale możemy. Co więcej, w ciągu jednego wieczoru znalazłem i spisałem wszystkie te błędy.
Ale napisanie artykułu zajęło kilka tygodni. Nie mogłam się zebrać, żeby to wszystko przełożyć na tekst :).
Swoją drogą, jeśli ciekawi Cię, jakie technologie są wykorzystywane w analizatorze PVS-Studio do identyfikacji błędów i potencjalnych podatności, to sugeruję zapoznanie się z tym
Nowa i stara diagnostyka
Jak już wspomniano, około dwa lata temu projekt LLVM został ponownie sprawdzony i znalezione błędy zostały poprawione. Teraz w tym artykule przedstawimy nową partię błędów. Dlaczego znaleziono nowe błędy? Są ku temu 3 powody:
- Projekt LLVM ewoluuje, zmieniając stary kod i dodając nowy. Naturalnie w zmodyfikowanym i napisanym kodzie pojawiają się nowe błędy. To wyraźnie pokazuje, że analizę statyczną należy stosować regularnie, a nie sporadycznie. Nasze artykuły dobrze pokazują możliwości analizatora PVS-Studio, jednak nie ma to nic wspólnego z poprawą jakości kodu i zmniejszeniem kosztów naprawy błędów. Regularnie korzystaj ze statycznego analizatora kodu!
- Finalizujemy i udoskonalamy istniejącą diagnostykę. Dzięki temu analizator może zidentyfikować błędy, których nie zauważył podczas poprzednich skanów.
- W PVS-Studio pojawiła się nowa diagnostyka, która 2 lata temu nie istniała. Postanowiłem wyróżnić je w osobnej sekcji, aby wyraźnie pokazać rozwój PVS-Studio.
Usterki zidentyfikowane poprzez diagnostykę istniejącą 2 lata temu
Fragment N1: Kopiuj-wklej
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
....
}
Ostrzeżenie PVS-Studio:
Sprawdza się dwukrotnie, czy nazwa zaczyna się od podłańcucha „avx512.mask.permvar.”. Przy drugiej kontroli oczywiście chcieli napisać coś innego, ale zapomnieli poprawić skopiowany tekst.
Fragment N2: Literówka
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;
....
}
Ostrzeżenie PVS-Studio: V501 Po lewej i prawej stronie znaku „|” znajdują się identyczne wyrażenia podrzędne „CXNameRange_WantQualifier”. operator. CIndex.cpp 7245
Z powodu literówki dwukrotnie użyto tej samej nazwy stałej CXNameRange_WantQualifier.
Fragment N3: Zamieszanie z pierwszeństwem operatora
int PPCTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) {
....
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian() ? 1 : 0)
return 0;
....
}
Ostrzeżenie PVS-Studio:
Moim zdaniem jest to bardzo piękny błąd. Tak, wiem, że mam dziwne wyobrażenia o pięknie :).
Teraz wg
(ISD == ISD::EXTRACT_VECTOR_ELT && (Index == ST->isLittleEndian())) ? 1 : 0
Z praktycznego punktu widzenia taki warunek nie ma sensu, gdyż można go sprowadzić do:
(ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian())
To wyraźny błąd. Najprawdopodobniej chcieli porównać 0/1 ze zmienną wskaźnik. Aby naprawić kod, musisz dodać nawiasy wokół operatora trójskładnikowego:
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == (ST->isLittleEndian() ? 1 : 0))
Nawiasem mówiąc, operator trójskładnikowy jest bardzo niebezpieczny i powoduje błędy logiczne. Bądź z tym bardzo ostrożny i nie bądź zachłanny, jeśli chodzi o nawiasy. Przyjrzałem się temu tematowi dokładniej
Fragment N4, N5: Wskaźnik zerowy
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;
}
....
}
Ostrzeżenie PVS-Studio:
Jeśli wskaźnik LHS ma wartość null, powinno zostać wydane ostrzeżenie. Zamiast tego jednak ten sam wskaźnik zerowy zostanie wyłuskany: LHS->getAsString().
Jest to bardzo typowa sytuacja, gdy błąd jest ukryty w programie obsługi błędów, ponieważ nikt go nie testuje. Analizatory statyczne sprawdzają cały osiągalny kod, bez względu na to, jak często jest używany. Jest to bardzo dobry przykład tego, jak analiza statyczna uzupełnia inne techniki testowania i ochrony przed błędami.
Podobny błąd obsługi wskaźnika RHS dozwolone w kodzie tuż poniżej: V522 [CWE-476] Może nastąpić wyłuskanie wskaźnika zerowego „RHS”. TGparser.cpp 2186
Fragment N6: Używanie wskaźnika po przesunięciu
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);
}
....
}
Ostrzeżenie PVS-Studio: V522 [CWE-476] Może nastąpić wyłuskanie wskaźnika zerowego „ProgClone”. Błędna kompilacja.cpp 601
Na początek mądry wskaźnik ProgClone przestaje być właścicielem przedmiotu:
BD.setNewProgram(std::move(ProgClone));
Właściwie teraz ProgClone jest wskaźnikiem zerowym. Dlatego dereferencja wskaźnika zerowego powinna nastąpić tuż poniżej:
Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);
Ale w rzeczywistości tak się nie stanie! Należy pamiętać, że pętla w rzeczywistości nie jest wykonywana.
Na początku pojemnika Błędnie skompilowane funkcje wyczyszczone:
MiscompiledFunctions.clear();
Następnie w warunku pętli używany jest rozmiar tego kontenera:
for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {
Łatwo zauważyć, że pętla się nie rozpoczyna. Myślę, że to również jest błąd i kod powinien być napisany inaczej.
Wygląda na to, że napotkaliśmy słynną parytet błędów! Jeden błąd maskuje drugi :).
Fragment N7: Używanie wskaźnika po przesunięciu
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;
}
....
}
Ostrzeżenie PVS-Studio: V522 [CWE-476] Może nastąpić wyłuskanie wskaźnika zerowego „Test”. Błędna kompilacja.cpp 709
Znowu ta sama sytuacja. W pierwszej kolejności przesuwana jest zawartość obiektu, a następnie jest on używany tak, jakby nic się nie stało. Coraz częściej widzę tę sytuację w kodzie programu po pojawieniu się semantyki ruchu w C++. Dlatego kocham język C++! Istnieje coraz więcej nowych sposobów na odstrzelenie sobie nogi. Analizator PVS-Studio zawsze będzie miał pracę :).
Fragment N8: Wskaźnik zerowy
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);
}
Ostrzeżenie PVS-Studio: V522 [CWE-476] Może nastąpić wyłuskanie wskaźnika zerowego „Typ”. PrettyFunctionDumper.cpp 233
Oprócz obsługi błędów zwykle nie testuje się funkcji wydruku debugującego. Mamy właśnie taki przypadek przed sobą. Funkcja czeka na użytkownika, który zamiast rozwiązać swoje problemy, będzie zmuszony ją naprawić.
Poprawnie:
if (Type)
Type->dump(*this);
else
Printer << "<unknown-type>";
Fragment N9: Wskaźnik zerowy
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());
....
}
Ostrzeżenie PVS-Studio: V522 [CWE-476] Może nastąpić wyłuskanie wskaźnika zerowego „Ty”. SearchableTableEmitter.cpp 614
Myślę, że wszystko jest jasne i nie wymaga wyjaśnień.
Fragment N10: Literówka
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;
}
Ostrzeżenie PVS-Studio:
Nie ma sensu przypisywać zmiennej samej sobie. Najprawdopodobniej chcieli napisać:
Identifier->Type = Question->Type;
Fragment N11: Podejrzana przerwa
void SystemZOperand::print(raw_ostream &OS) const {
switch (Kind) {
break;
case KindToken:
OS << "Token:" << getToken();
break;
case KindReg:
OS << "Reg:" << SystemZInstPrinter::getRegisterName(getReg());
break;
....
}
Ostrzeżenie PVS-Studio:
Na początku jest bardzo podejrzany operator złamać. Zapomniałeś napisać tutaj coś jeszcze?
Fragment N12: Sprawdzanie wskaźnika po wyłuskaniu
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");
....
}
Ostrzeżenie PVS-Studio:
Wskaźnik wywołany na początku jest dereferowana w momencie wywołania funkcji uzyskaćTTI.
A potem okazuje się, że ten wskaźnik należy sprawdzić pod kątem równości nullptr:
if (!Callee || Callee->isDeclaration())
Ale jest za późno…
Fragment N13 - N...: Sprawdzanie wskaźnika po wyłuskaniu
Sytuacja omówiona w poprzednim fragmencie kodu nie jest wyjątkowa. Pojawia się tutaj:
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()) { // <=
....
}
Ostrzeżenie PVS-Studio: V595 [CWE-476] Wskaźnik „CalleeFn” został wykorzystany przed jego zweryfikowaniem pod kątem wartości nullptr. Sprawdź linie: 1079, 1081. SimplifyLibCalls.cpp 1079
I tu:
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()); // <=
....
}
Ostrzeżenie PVS-Studio: V595 [CWE-476] Wskaźnik „ND” został użyty przed jego sprawdzeniem pod kątem wartości nullptr. Sprawdź linie: 532, 534. SemaTemplateInstantiateDecl.cpp 532
I tu:
- V595 [CWE-476] Wskaźnik „U” został użyty przed jego zweryfikowaniem względem wartości nullptr. Sprawdź linie: 404, 407. DWARFormValue.cpp 404
- V595 [CWE-476] Wskaźnik „ND” został użyty przed jego sprawdzeniem pod kątem wartości nullptr. Sprawdź linie: 2149, 2151. SemaTemplateInstantiate.cpp 2149
A potem przestałem być zainteresowany studiowaniem ostrzeżeń o numerze V595. Nie wiem więc, czy podobnych błędów jest więcej niż te wymienione tutaj. Najprawdopodobniej istnieje.
Fragment N17, N18: Podejrzana zmiana
static inline bool processLogicalImmediate(uint64_t Imm, unsigned RegSize,
uint64_t &Encoding) {
....
unsigned Size = RegSize;
....
uint64_t NImms = ~(Size-1) << 1;
....
}
Ostrzeżenie PVS-Studio:
Być może nie jest to błąd i kod działa dokładnie tak, jak zamierzono. Ale to zdecydowanie bardzo podejrzane miejsce i należy je sprawdzić.
Powiedzmy, że zmienna Rozmiar jest równe 16, a następnie autor kodu planował uzyskać to w zmiennej NImm oznaczający:
1111111111111111111111111111111111111111111111111111111111100000
Jednak w rzeczywistości wynik będzie następujący:
0000000000000000000000000000000011111111111111111111111111100000
Faktem jest, że wszystkie obliczenia odbywają się przy użyciu 32-bitowego typu bez znaku. I dopiero wtedy ten 32-bitowy typ bez znaku zostanie domyślnie rozszerzony do uint64_t. W tym przypadku najbardziej znaczące bity będą wynosić zero.
Możesz naprawić sytuację w ten sposób:
uint64_t NImms = ~static_cast<uint64_t>(Size-1) << 1;
Podobna sytuacja: V629 [CWE-190] Rozważ sprawdzenie wyrażenia „Immr << 6”. Przesunięcie bitowe wartości 32-bitowej z późniejszą ekspansją do typu 64-bitowego. AArch64AddressingModes.h 269
Fragment N19: Brakujące słowo kluczowe więcej?
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");
}
....
}
Ostrzeżenie PVS-Studio:
Nie ma tu żadnego błędu. Od ówczesnego bloku pierwszego if kończy się kontynuować, to nie ma to znaczenia, jest słowo kluczowe więcej albo nie. Tak czy inaczej, kod będzie działał tak samo. Wciąż brakowało więcej sprawia, że kod jest bardziej niejasny i niebezpieczny. Jeśli w przyszłości kontynuować zniknie, kod zacznie działać zupełnie inaczej. Moim zdaniem lepiej dodać więcej.
Fragment N20: Cztery literówki tego samego typu
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;
}
Ostrzeżenia PVS-Studio:
- V655 [CWE-480] Łańcuchy zostały połączone, ale nie są używane. Rozważ sprawdzenie wyrażenia „Wynik + Nazwa.str()”. Symbol.cpp 32
- V655 [CWE-480] Łańcuchy zostały połączone, ale nie są używane. Rozważ sprawdzenie wyrażenia „Wynik + „(Klasa ObjC)” + Nazwa.str()”. Symbol.cpp 35
- V655 [CWE-480] Łańcuchy zostały połączone, ale nie są używane. Rozważ sprawdzenie wyrażenia „Wynik + „(Klasa ObjC EH)” + Nazwa.str()”. Symbol.cpp 38
- V655 [CWE-480] Łańcuchy zostały połączone, ale nie są używane. Rozważ sprawdzenie wyrażenia „Wynik + „(ObjC IVar)” + Nazwa.str()”. Symbol.cpp 41
Przez przypadek operator + został użyty zamiast operatora +=. Rezultatem są projekty pozbawione znaczenia.
Fragment N21: Niezdefiniowane zachowanie
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();
}
}
}
Spróbuj samodzielnie znaleźć niebezpieczny kod. A to zdjęcie dla odwrócenia uwagi, żeby nie patrzeć od razu na odpowiedź:
Ostrzeżenie PVS-Studio:
Linia problemowa:
FeaturesMap[Op] = FeaturesMap.size();
Jeśli element Op nie zostanie znaleziony, wówczas na mapie tworzony jest nowy element i zapisywana jest tam liczba elementów tej mapy. Nie wiadomo tylko, czy funkcja zostanie wywołana rozmiar przed lub po dodaniu nowego elementu.
Fragment N22-N24: Powtarzane zadania
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;
}
....
}
Ostrzeżenie PVS-Studio:
Nie sądzę, że jest tu jakiś poważny błąd. Po prostu niepotrzebne powtarzanie zadania. Ale nadal błąd.
Podobnie:
- V519 [CWE-563] Zmiennej „B.NDesc” przypisuje się wartości dwukrotnie po sobie. Być może jest to błąd. Sprawdź linie: 1488, 1489. llvm-nm.cpp 1489
- V519 [CWE-563] Zmiennej przypisywane są wartości kolejno dwukrotnie. Być może jest to błąd. Sprawdź linie: 59, 61. coff2yaml.cpp 61
Fragment N25-N27: Więcej ponownych przydziałów
Przyjrzyjmy się teraz nieco innej wersji przeniesienia.
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;
....
}
Ostrzeżenie PVS-Studio: V519 [CWE-563] Do zmiennej „Wyrównanie” przypisywane są wartości dwukrotnie. Być może jest to błąd. Sprawdź linie: 1158, 1160. LoadStoreVectorizer.cpp 1160
To bardzo dziwny kod, który najwyraźniej zawiera błąd logiczny. Na początku zmienne Wyrównanie wartość jest przypisywana w zależności od warunku. A potem przypisanie następuje ponownie, ale teraz bez żadnej kontroli.
Podobne sytuacje można zobaczyć tutaj:
- V519 [CWE-563] Zmiennej „Efekty” przypisuje się wartości dwukrotnie po sobie. Być może jest to błąd. Sprawdź linie: 152, 165. WebAssemblyRegStackify.cpp 165
- V519 [CWE-563] Do zmiennej „ExpectNoDerefChunk” przypisywane są wartości dwukrotnie. Być może jest to błąd. Sprawdź linie: 4970, 4973. SemaType.cpp 4973
Fragment N28: Zawsze warunek prawdziwy
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;
}
....
}
Ostrzeżenie PVS-Studio:
Sprawdzanie nie ma sensu. Zmienny następny bajt zawsze nie równa się wartości 0x90, co wynika z poprzedniego sprawdzenia. To jest jakiś błąd logiczny.
Fragment N29 - N...: Zawsze warunki prawda/fałsz
Analizator generuje wiele ostrzeżeń, że cały warunek (
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;
....
}
Ostrzeżenie PVS-Studio:
Stała 0xE to wartość 14 w systemie dziesiętnym. Badanie Nr rej. == 0xe nie ma sensu, bo jeśli Nr rejestracyjny > 13, wówczas funkcja zakończy swoje wykonanie.
Było wiele innych ostrzeżeń o identyfikatorach V547 i V560, ale jak z
Dam ci przykład, dlaczego studiowanie tych wyzwalaczy jest nudne. Analizator ma całkowitą rację, wydając ostrzeżenie dla następującego kodu. Ale to nie jest błąd.
bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
tok::TokenKind ClosingBraceKind) {
bool HasError = false;
....
HasError = true;
if (!ContinueOnSemicolons)
return !HasError;
....
}
Ostrzeżenie PVS-Studio: V547 [CWE-570] Wyrażenie „!HasError” jest zawsze fałszywe. UnwrappedLineParser.cpp 1635
Fragment N30: Podejrzany powrót
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();
}
....
}
Ostrzeżenie PVS-Studio:
Jest to albo błąd, albo specyficzna technika, która ma na celu wyjaśnienie czegoś programistom czytającym kod. Ten projekt nic mi nie wyjaśnia i wygląda bardzo podejrzanie. Lepiej tak nie pisać :).
Zmęczony? Następnie nadszedł czas na zrobienie herbaty lub kawy.
Usterki zidentyfikowane poprzez nową diagnostykę
Myślę, że 30 aktywacji starej diagnostyki wystarczy. Zobaczmy teraz, jakie ciekawe rzeczy można znaleźć dzięki nowej diagnostyce, która pojawiła się później w analizatorze
Fragment N31: Kod nieosiągalny
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();
}
Ostrzeżenie PVS-Studio:
Jak widać obie gałęzie operatora if kończy się połączeniem do operatora powrót. Odpowiednio pojemnik CtorDtorsWedługPriorytetu nigdy nie zostanie oczyszczone.
Fragment N32: Kod nieosiągalny
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;
}
Ostrzeżenie PVS-Studio: V779 [CWE-561] Wykryto nieosiągalny kod. Możliwe, że wystąpił błąd. LLParser.cpp 835
Ciekawa sytuacja. Przyjrzyjmy się najpierw temu miejscu:
return ParseTypeIdEntry(SummaryID);
break;
Na pierwszy rzut oka wydaje się, że nie ma tu żadnego błędu. Wygląda na operatora złamać jest tu dodatkowy i możesz go po prostu usunąć. Jednak nie wszystko jest takie proste.
Analizator wyświetla ostrzeżenie w liniach:
Lex.setIgnoreColonInIdentifiers(false);
return false;
I rzeczywiście, ten kod jest nieosiągalny. Wszystkie sprawy w wyłącznik kończy się połączeniem od operatora powrót. A teraz bezsensowna samotność złamać nie wygląda tak nieszkodliwie! Być może jedna z gałęzi powinna zakończyć się złamaćale nie na powrót?
Fragment N33: Losowe resetowanie wysokich bitów
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);
....
}
Ostrzeżenie PVS-Studio:
Należy pamiętać, że funkcja getStubAlignment zwraca typ unsigned. Obliczmy wartość wyrażenia zakładając, że funkcja zwraca wartość 8:
~(getStubAlignment() - 1)
~(8u-1)
0xFFFFFFFF8u
Teraz zauważ, że zmienna Rozmiar danych ma 64-bitowy typ bez znaku. Okazuje się, że podczas wykonywania operacji DataSize & 0xFFFFFFF8u wszystkie trzydzieści dwa bity wyższego rzędu zostaną zresetowane do zera. Najprawdopodobniej nie tego chciał programista. Podejrzewam, że chciał obliczyć: DataSize & 0xFFFFFFFFFFFFFF8u.
Aby naprawić błąd, powinieneś napisać tak:
DataSize &= ~(static_cast<uint64_t>(getStubAlignment()) - 1);
Lub tak:
DataSize &= ~(getStubAlignment() - 1ULL);
Fragment N34: Nie powiodło się rzutowanie typu jawnego
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);
....
}
Ostrzeżenie PVS-Studio:
Jawne rzutowanie typu służy do uniknięcia przepełnienia podczas mnożenia zmiennych typu int. Jednak jawne rzutowanie typów nie chroni tutaj przed przepełnieniem. Najpierw zmienne zostaną pomnożone, a dopiero potem 32-bitowy wynik mnożenia zostanie rozwinięty do typu
Fragment N35: Nieudane kopiowanie i wklejanie
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;
}
....
}
Ta nowa, interesująca diagnostyka identyfikuje sytuacje, w których fragment kodu został skopiowany i niektóre nazwy w nim zaczęto zmieniać, ale w jednym miejscu tego nie poprawiono.
Należy pamiętać, że w drugim bloku uległy zmianie Op0 na Op1. Ale w jednym miejscu tego nie naprawili. Najprawdopodobniej należało to napisać w ten sposób:
if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
I.setOperand(1, ConstantFP::getNullValue(Op1->getType()));
return &I;
}
Fragment N36: Zmienne zamieszanie
struct Status {
unsigned Mask;
unsigned Mode;
Status() : Mask(0), Mode(0){};
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
Mode &= Mask;
};
....
};
Ostrzeżenie PVS-Studio:
Nadawanie argumentom funkcji tych samych nazw, co członom klasy, jest bardzo niebezpieczne. Bardzo łatwo się pomylić. Mamy właśnie taki przypadek przed sobą. To wyrażenie nie ma sensu:
Mode &= Mask;
Argument funkcji ulega zmianie. To wszystko. Ten argument nie jest już używany. Najprawdopodobniej powinieneś napisać to w ten sposób:
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
this->Mode &= Mask;
};
Fragment N37: Zmienne zamieszanie
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;
}
Ostrzeżenie PVS-Studio: V1001 [CWE-563] Zmienna „Rozmiar” jest przypisana, ale nie jest używana na końcu funkcji. Obiekt.cpp 424
Sytuacja jest podobna do poprzedniej. Powinno być napisane:
this->Size += this->EntrySize;
Fragment N38-N47: Zapomnieli sprawdzić indeks
Wcześniej przyjrzeliśmy się przykładom wyzwalania diagnostycznego
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()); // <=
....
}
Ostrzeżenie PVS-Studio: V1004 [CWE-476] Wskaźnik „Ptr” został użyty w sposób niebezpieczny po zweryfikowaniu go pod kątem wartości nullptr. Sprawdź linie: 729, 738. TargetTransformInfoImpl.h 738
Zmienna Ptr może być równe nullptro czym świadczy kontrola:
if (Ptr != nullptr)
Jednakże poniżej ten wskaźnik jest wyłuskiwany bez wstępnego sprawdzania:
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());
Rozważmy inny podobny przypadek.
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(); // <=
....
}
Ostrzeżenie PVS-Studio: V1004 [CWE-476] Wskaźnik „FD” został użyty w sposób niebezpieczny po zweryfikowaniu go pod kątem wartości nullptr. Sprawdź linie: 3228, 3231. CGDebugInfo.cpp 3231
Zwróć uwagę na znak FD. Jestem pewien, że problem jest wyraźnie widoczny i nie wymaga specjalnych wyjaśnień.
I dalej:
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()); // <=
....
}
Ostrzeżenie PVS-Studio: V1004 [CWE-476] Wskaźnik „PtrTy” został użyty w sposób niebezpieczny po zweryfikowaniu go pod kątem wartości nullptr. Sprawdź linie: 960, 965. InterleavedLoadCombinePass.cpp 965
Jak uchronić się przed takimi błędami? Bądź bardziej uważny podczas Code-Review i korzystaj z analizatora statycznego PVS-Studio, aby regularnie sprawdzać swój kod.
Nie ma sensu cytować innych fragmentów kodu zawierających tego typu błędy. W artykule pozostawię jedynie listę ostrzeżeń:
- V1004 [CWE-476] Wskaźnik „Expr” został użyty w sposób niebezpieczny po zweryfikowaniu go pod kątem wartości nullptr. Sprawdź linie: 1049, 1078. DebugInfoMetadata.cpp 1078
- V1004 [CWE-476] Wskaźnik „PI” został użyty w sposób niebezpieczny po zweryfikowaniu go pod kątem wartości nullptr. Sprawdź linie: 733, 753. LegacyPassManager.cpp 753
- V1004 [CWE-476] Wskaźnik „StatepointCall” został użyty w sposób niebezpieczny po zweryfikowaniu go pod kątem wartości nullptr. Sprawdź linie: 4371, 4379. Verifier.cpp 4379
- V1004 [CWE-476] Wskaźnik „RV” został użyty w sposób niebezpieczny po zweryfikowaniu go pod kątem wartości nullptr. Sprawdź linie: 2263, 2268. TGParser.cpp 2268
- V1004 [CWE-476] Wskaźnik „CalleeFn” został użyty w sposób niebezpieczny po zweryfikowaniu go pod kątem wartości nullptr. Sprawdź linie: 1081, 1096. SimplifyLibCalls.cpp 1096
- V1004 [CWE-476] Wskaźnik „TC” został użyty w sposób niebezpieczny po zweryfikowaniu go pod kątem wartości nullptr. Sprawdź linie: 1819, 1824. Driver.cpp 1824
Fragment N48-N60: Nie krytyczny, ale wada (możliwy wyciek pamięci)
std::unique_ptr<IRMutator> createISelMutator() {
....
std::vector<std::unique_ptr<IRMutationStrategy>> Strategies;
Strategies.emplace_back(
new InjectorIRStrategy(InjectorIRStrategy::getDefaultOps()));
....
}
Ostrzeżenie PVS-Studio:
Aby dodać element na końcu kontenera, np std::wektor > nie możesz po prostu pisać xxx.push_back(nowy X), ponieważ nie ma ukrytej konwersji z X* в std::unique_ptr.
Powszechnym rozwiązaniem jest pisanie xxx.emplace_back(nowy X)ponieważ się kompiluje: metoda miejsce_z powrotem konstruuje element bezpośrednio na podstawie jego argumentów i dlatego może używać jawnych konstruktorów.
To nie jest bezpieczne. Jeśli wektor jest pełny, pamięć jest ponownie przydzielana. Operacja ponownego przydzielenia pamięci może zakończyć się niepowodzeniem, co spowoduje zgłoszenie wyjątku std::bad_alloc. W takim przypadku wskaźnik zostanie utracony, a utworzony obiekt nigdy nie zostanie usunięty.
Bezpiecznym rozwiązaniem jest tworzenie unikalny_ptrktóry będzie właścicielem wskaźnika, zanim wektor spróbuje ponownie przydzielić pamięć:
xxx.push_back(std::unique_ptr<X>(new X))
Od C++ 14 możesz użyć „std::make_unique”:
xxx.push_back(std::make_unique<X>())
Ten typ wady nie jest krytyczny dla LLVM. Jeśli nie można przydzielić pamięci, kompilator po prostu się zatrzyma. Jednak w przypadku zastosowań z długimi
Tak więc, chociaż ten kod nie stanowi praktycznego zagrożenia dla LLVM, uznałem za przydatne omówienie tego wzorca błędów i fakt, że analizator PVS-Studio nauczył się go identyfikować.
Inne ostrzeżenia tego typu:
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Przepustki” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. PassManager.h 546
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „AA” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. AliasAnalytics.h 324
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Entries” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. DWARFDebugFrame.cpp 519
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „AllEdges” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. CFGMST.h 268
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „VMaps” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. SimpleLoopUnswitch.cpp 2012
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Rekordy” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. FDRLogBuilder.h 30
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „PendingSubmodules” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. Mapa modułu.cpp 810
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Obiekty” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. DebugMap.cpp 88
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Strategie” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. llvm-isel-fuzzer.cpp 60
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Modyfikatory” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. llvm-stres.cpp 685
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Modyfikatory” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. llvm-stres.cpp 686
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Modyfikatory” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. llvm-stres.cpp 688
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Modyfikatory” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. llvm-stres.cpp 689
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Modyfikatory” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. llvm-stres.cpp 690
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Modyfikatory” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. llvm-stres.cpp 691
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Modyfikatory” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. llvm-stres.cpp 692
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Modyfikatory” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. llvm-stres.cpp 693
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Modyfikatory” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. llvm-stres.cpp 694
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Operandy” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. GlobalISelEmitter.cpp 1911
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Stash” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. GlobalISelEmitter.cpp 2100
- V1023 [CWE-460] Wskaźnik bez właściciela jest dodawany do kontenera „Matchers” za pomocą metody „emplace_back”. W przypadku wyjątku nastąpi wyciek pamięci. GlobalISelEmitter.cpp 2702
wniosek
W sumie wydałem 60 ostrzeżeń i przestałem. Czy w LLVM analizator PVS-Studio wykrywa inne defekty? Tak, mam. Kiedy jednak pisałem fragmenty kodu do artykułu, był późny wieczór, a raczej noc, i zdecydowałem, że czas już to zakończyć.
Mam nadzieję, że był dla Ciebie interesujący i będziesz chciał wypróbować analizator PVS-Studio.
Możesz pobrać analizator i uzyskać klucz do trałowca pod adresem
Co najważniejsze, regularnie korzystaj z analizy statycznej. Kontrole jednorazowe, prowadzone przez nas w celu popularyzacji metodologii analizy statycznej i PVS-Studio nie są normalnym scenariuszem.
Powodzenia w poprawie jakości i niezawodności Twojego kodu!
Jeśli chcesz udostępnić ten artykuł anglojęzycznej publiczności, skorzystaj z linku do tłumaczenia: Andrey Karpov.
Źródło: www.habr.com