ProHoster > Blog > Administración > Buscar erros en LLVM 8 usando o analizador PVS-Studio

Buscar erros en LLVM 8 usando o analizador PVS-Studio

Buscar erros en LLVM 8 usando o analizador PVS-Studio
Pasaron máis de dous anos desde a última comprobación de código do proxecto LLVM mediante o noso analizador PVS-Studio. Asegurémonos de que o analizador PVS-Studio aínda é unha ferramenta líder para identificar erros e posibles vulnerabilidades. Para iso, comprobaremos e atoparemos novos erros na versión LLVM 8.0.0.

Artigo por escribir

Para ser sincero, non quería escribir este artigo. Non é interesante escribir sobre un proxecto que xa revisamos varias veces (1, 2, 3). É mellor escribir sobre algo novo, pero non teño opción.

Cada vez que se publica ou actualiza unha nova versión de LLVM Analizador estático Clang, recibimos preguntas do seguinte tipo no noso correo:

Mira, a nova versión de Clang Static Analyzer aprendeu a atopar novos erros. Paréceme que a relevancia do uso de PVS-Studio está a diminuír. Clang atopa máis erros que antes e ponse ao día coas capacidades de PVS-Studio. Que opinas disto?

A isto sempre quero responder algo como:

Nós tampouco quedamos parados! Melloramos significativamente as capacidades do analizador PVS-Studio. Así que non te preocupes, seguimos liderando como antes.

Desafortunadamente, esta é unha mala resposta. Non hai probas nel. E por iso estou escribindo este artigo agora. Entón, o proxecto LLVM foi verificado unha vez máis e atopáronse unha variedade de erros nel. Agora vou demostrar aqueles que me pareceron interesantes. Clang Static Analyzer non pode atopar estes erros (ou é moi inconveniente facelo coa súa axuda). Pero podemos. Ademais, atopei e anotei todos estes erros nunha noite.

Pero escribir o artigo levou varias semanas. Non puiden poñer todo isto en texto :).

Por certo, se estás interesado en que tecnoloxías se usan no analizador PVS-Studio para identificar erros e posibles vulnerabilidades, suxiro que te familiarices con isto. Nota.

Diagnósticos novos e antigos

Como xa se indicou, hai uns dous anos volveuse a comprobar o proxecto LLVM e corrixíronse os erros atopados. Agora este artigo presentará un novo lote de erros. Por que se atoparon novos erros? Hai 3 razóns para iso:

  1. O proxecto LLVM está evolucionando, cambiando o código antigo e engadindo código novo. Por suposto, hai novos erros no código modificado e escrito. Isto demostra claramente que a análise estática debe usarse con regularidade, e non ocasionalmente. Os nosos artigos mostran ben as capacidades do analizador PVS-Studio, pero isto non ten nada que ver coa mellora da calidade do código e na redución do custo de arranxar os erros. Use un analizador de código estático regularmente!
  2. Estamos ultimando e mellorando os diagnósticos existentes. Polo tanto, o analizador pode identificar erros que non detectou durante as exploracións anteriores.
  3. En PVS-Studio apareceron novos diagnósticos que non existían hai 2 anos. Decidín destacalos nunha sección separada para mostrar claramente o desenvolvemento de PVS-Studio.

Defectos identificados por diagnósticos que existían hai 2 anos

Fragmento N1: Copiar-Pegar

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
  ....
}

Aviso de PVS-Studio: V501 [CWE-570] Hai subexpresións idénticas 'Name.startswith("avx512.mask.permvar.")' á esquerda e á dereita do '||' operador. AutoUpgrade.cpp 73

Compróbase dúas veces que o nome comeza coa subcadea "avx512.mask.permvar.". Na segunda comprobación, obviamente querían escribir outra cousa, pero esqueceron corrixir o texto copiado.

Fragmento N2: erro tipográfico

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;
  ....
}

Aviso PVS-Studio: V501 Hai subexpresións idénticas 'CXNameRange_WantQualifier' á esquerda e á dereita do '|' operador. CIndex.cpp 7245

Debido a un erro tipográfico, a mesma constante nomeada úsase dúas veces CXNameRange_WantQualifier.

Fragmento N3: confusión coa precedencia do operador

int PPCTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) {
  ....
  if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian() ? 1 : 0)
    return 0;
  ....
}

Aviso de PVS-Studio: V502 [CWE-783] Quizais o operador '?:' funcione dun xeito diferente do que se esperaba. O operador '?:' ten unha prioridade máis baixa que o operador '=='. PPCTargetTransformInfo.cpp 404

Na miña opinión, este é un erro moi bonito. Si, sei que teño ideas estrañas sobre a beleza :).

Agora, segundo prioridades do operador, a expresión avalíase do seguinte xeito:

(ISD == ISD::EXTRACT_VECTOR_ELT && (Index == ST->isLittleEndian())) ? 1 : 0

Desde un punto de vista práctico, tal condición non ten sentido, xa que se pode reducir a:

(ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian())

Este é un erro claro. O máis probable é que quixesen comparar 0/1 cunha variable Índice. Para corrixir o código, cómpre engadir parénteses ao operador ternario:

if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == (ST->isLittleEndian() ? 1 : 0))

Por certo, o operador ternario é moi perigoso e provoca erros lóxicos. Teña moito coidado con el e non sexas cobizoso con parénteses. Observei este tema con máis detalle aquí, no capítulo "Coidado co ?: Operador e enxérrao entre parénteses".

Fragmento N4, N5: punteiro nulo

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;
  }
  ....
}

Aviso de PVS-Studio: V522 [CWE-476] Pode que se produza a desreferenciación do punteiro nulo 'LHS'. TGParser.cpp 2152

Se o punteiro LHS é nulo, debe emitirse unha advertencia. Non obstante, no seu lugar, este mesmo punteiro nulo será desreferenciado: LHS->getAsString().

Esta é unha situación moi típica cando se oculta un erro nun manejador de erros, xa que ninguén o proba. Os analizadores estáticos verifican todo o código accesible, sen importar a frecuencia con que se use. Este é un moi bo exemplo de como a análise estática complementa outras técnicas de proba e protección contra erros.

Erro similar ao manexo do punteiro RHS permitido no código debaixo: V522 [CWE-476] Pode ter lugar a desreferenciación do punteiro nulo 'RHS'. TGParser.cpp 2186

Fragmento N6: Usar o punteiro despois de moverse

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);
  }
  ....
}

Aviso de PVS-Studio: V522 [CWE-476] É posible que se produza a desreferenciación do punteiro nulo 'ProgClone'. Miscompilation.cpp 601

Ao principio un punteiro intelixente ProgClone deixa de ser propietario do obxecto:

BD.setNewProgram(std::move(ProgClone));

De feito, agora ProgClone é un punteiro nulo. Polo tanto, debería producirse unha desreferencia de punteiro nulo xusto debaixo:

Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);

Pero, en realidade, isto non sucederá! Teña en conta que o bucle non se executa realmente.

Ao comezo do recipiente Funcións compiladas incorrectamente despexado:

MiscompiledFunctions.clear();

A continuación, o tamaño deste recipiente úsase na condición de bucle:

for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {

É doado ver que o bucle non comeza. Creo que isto tamén é un erro e que o código debería escribirse doutro xeito.

Parece que nos atopamos con esa famosa paridade de erros! Un erro enmascara outro :).

Fragmento N7: Usar o punteiro despois de moverse

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;
  }
  ....
}

Aviso de PVS-Studio: V522 [CWE-476] É posible que se produza a desreferenciación do punteiro nulo "Test". Compilación incorrecta.cpp 709

A mesma situación de novo. Nun primeiro momento, o contido do obxecto móvese e despois utilízase coma se nada pasase. Vexo esta situación cada vez con máis frecuencia no código do programa despois de que a semántica do movemento apareceu en C++. É por iso que me encanta a linguaxe C++! Cada vez hai máis formas novas de disparar a túa propia perna. O analizador PVS-Studio sempre terá traballo :).

Fragmento N8: punteiro nulo

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);
}

Aviso de PVS-Studio: V522 [CWE-476] É posible que se produza a desreferenciación do punteiro nulo "Tipo". PrettyFunctionDumper.cpp 233

Ademais dos controladores de erros, as funcións de impresión de depuración normalmente non se proban. Só temos un caso así ante nós. A función está á espera do usuario, que en vez de resolver os seus problemas, verase obrigado a solucionala.

De forma correcta:

if (Type)
  Type->dump(*this);
else
  Printer << "<unknown-type>";

Fragmento N9: punteiro nulo

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());
   ....
}

Aviso de PVS-Studio: V522 [CWE-476] É posible que se produza a desreferenciación do punteiro nulo 'Ty'. SearchableTableEmitter.cpp 614

Creo que está todo claro e non precisa explicación.

Fragmento N10: erro tipográfico

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;
}

Aviso de PVS-Studio: V570 A variable 'Identificador->Tipo' está asignada a si mesma. FormatTokenLexer.cpp 249

Non ten sentido asignarlle unha variable a si mesma. O máis probable é que quixesen escribir:

Identifier->Type = Question->Type;

Fragmento N11: Rotura sospeitosa

void SystemZOperand::print(raw_ostream &OS) const {
  switch (Kind) {
    break;
  case KindToken:
    OS << "Token:" << getToken();
    break;
  case KindReg:
    OS << "Reg:" << SystemZInstPrinter::getRegisterName(getReg());
    break;
  ....
}

Aviso de PVS-Studio: V622 [CWE-478] Considere inspeccionar a declaración "switch". É posible que falte o primeiro operador de "caso". SystemZAsmParser.cpp 652

Hai un operador moi sospeitoso ao principio break. Esqueciches escribir algo máis aquí?

Fragmento N12: comprobación dun punteiro despois de desreferenciar

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");
  ....
}

Aviso de PVS-Studio: V595 [CWE-476] O punteiro 'Callee' utilizouse antes de verificalo contra nullptr. Liñas de verificación: 172, 174. AMDGPUInline.cpp 172

Punteiro Callee ao principio é desreferenciado no momento en que se chama a función obterTTI.

E entón resulta que este punteiro debería comprobarse a igualdade nullptr:

if (!Callee || Callee->isDeclaration())

Pero é demasiado tarde...

Fragmento N13 - N...: Comprobación dun punteiro despois de desreferenciar

A situación discutida no fragmento de código anterior non é única. Aparece aquí:

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()) {               // <=
  ....
}

Aviso de PVS-Studio: V595 [CWE-476] Utilizouse o punteiro 'CalleeFn' antes de verificalo contra nullptr. Liñas de verificación: 1079, 1081. SimplifyLibCalls.cpp 1079

E aquí:

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());     // <=
  ....
}

Aviso de PVS-Studio: V595 [CWE-476] Utilizouse o punteiro "ND" antes de verificalo contra nullptr. Liñas de verificación: 532, 534. SemaTemplateInstantiateDecl.cpp 532

E aquí:

  • V595 [CWE-476] Utilizouse o punteiro "U" antes de verificalo contra nullptr. Liñas de verificación: 404, 407. DWARFormValue.cpp 404
  • V595 [CWE-476] O punteiro 'ND' utilizouse antes de verificalo contra nullptr. Liñas de verificación: 2149, 2151. SemaTemplateInstantiate.cpp 2149

E entón non me interesou estudar os avisos co número V595. Polo tanto, non sei se hai máis erros similares ademais dos que aquí se enumeran. O máis probable é que o haxa.

Fragmento N17, N18: Quenda sospeitosa

static inline bool processLogicalImmediate(uint64_t Imm, unsigned RegSize,
                                           uint64_t &Encoding) {
  ....
  unsigned Size = RegSize;
  ....
  uint64_t NImms = ~(Size-1) << 1;
  ....
}

Aviso de PVS-Studio: V629 [CWE-190] Considere inspeccionar a expresión '~(Tamaño - 1) << 1'. Cambio de bits do valor de 32 bits cunha expansión posterior ao tipo de 64 bits. AArch64AddressingModes.h 260

Pode non ser un erro e o código funciona exactamente como se pretende. Pero este é claramente un lugar moi sospeitoso e hai que verificalo.

Digamos a variable tamaño é igual a 16, e entón o autor do código planeou obtelo nunha variable NImms valor:

1111111111111111111111111111111111111111111111111111111111100000

Non obstante, en realidade o resultado será:

0000000000000000000000000000000011111111111111111111111111100000

O feito é que todos os cálculos ocorren usando o tipo de 32 bits sen asinar. E só entón, este tipo de 32 bits sen asinar expandirase implícitamente a uint64_t. Neste caso, os bits máis significativos serán cero.

Podes corrixir a situación así:

uint64_t NImms = ~static_cast<uint64_t>(Size-1) << 1;

Situación semellante: V629 [CWE-190] Considere inspeccionar a expresión 'Immr << 6'. Cambio de bits do valor de 32 bits cunha expansión posterior ao tipo de 64 bits. AArch64AddressingModes.h 269

Fragmento N19: falta a palabra clave outro?

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");
  }
  ....
}

Aviso de PVS-Studio: V646 [CWE-670] Considere inspeccionar a lóxica da aplicación. É posible que falte a palabra clave "else". AMDGPUAsmParser.cpp 5655

Non hai ningún erro aquí. Desde o bloque de entón do primeiro if remata con continuar, entón non importa, hai unha palabra clave outro ou non. De calquera xeito, o código funcionará igual. Aínda faltaba outro fai o código máis pouco claro e perigoso. Se no futuro continuar desaparece, o código comezará a funcionar de forma completamente diferente. Na miña opinión é mellor engadir outro.

Fragmento N20: Catro erros tipográficos do mesmo tipo

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;
}

Avisos de PVS-Studio:

  • V655 [CWE-480] As cadeas foron concatenadas pero non se usan. Considere inspeccionar a expresión "Resultado + Nome.str()". Símbolo.cpp 32
  • V655 [CWE-480] As cadeas foron concatenadas pero non se usan. Considere inspeccionar a expresión 'Resultado + "(Clase ObjC)" + Nome.str()'. Símbolo.cpp 35
  • V655 [CWE-480] As cadeas foron concatenadas pero non se usan. Considere inspeccionar a expresión "Resultado + "(Clase ObjC EH)" + Nome.str()". Símbolo.cpp 38
  • V655 [CWE-480] As cadeas foron concatenadas pero non se usan. Considere inspeccionar a expresión 'Resultado + "(ObjC IVar)" + Nome.str()'. Símbolo.cpp 41

Por accidente, úsase o operador + en lugar do operador +=. O resultado son deseños que carecen de significado.

Fragmento N21: Comportamento indefinido

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();
    }
  }
}

Tenta atopar o código perigoso ti mesmo. E esta é unha imaxe para distraer a atención para non mirar inmediatamente a resposta:

Buscar erros en LLVM 8 usando o analizador PVS-Studio

Aviso de PVS-Studio: V708 [CWE-758] Utilízase unha construción perigosa: 'FeaturesMap[Op] = FeaturesMap.size()', onde 'FeaturesMap' é da clase 'map'. Isto pode levar a un comportamento indefinido. RISCVCompressInstEmitter.cpp 490

Liña do problema:

FeaturesMap[Op] = FeaturesMap.size();

Se elemento Op non se atopa, entón créase un novo elemento no mapa e alí escríbese o número de elementos deste mapa. Só se descoñece se se chamará a función tamaño antes ou despois de engadir un novo elemento.

Fragmento N22-N24: tarefas repetidas

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;
  }
  ....
}

Aviso de PVS-Studio: V519 [CWE-563] Á variable 'NType' asígnaselles valores dúas veces sucesivas. Quizais isto sexa un erro. Liñas de verificación: 1663, 1664. MachOObjectFile.cpp 1664

Non creo que haxa un verdadeiro erro aquí. Só unha tarefa repetida innecesaria. Pero non deixa de ser un erro.

Así mesmo:

  • V519 [CWE-563] A variable 'B.NDesc' asígnaselle valores dúas veces consecutivas. Quizais isto sexa un erro. Liñas de verificación: 1488, 1489. llvm-nm.cpp 1489
  • V519 [CWE-563] Á variable asígnaselle valores dúas veces sucesivas. Quizais isto sexa un erro. Liñas de verificación: 59, 61. coff2yaml.cpp 61

Fragmento N25-N27: Máis reasignacións

Agora vexamos unha versión lixeiramente diferente da reasignación.

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;
  ....
}

Aviso de PVS-Studio: V519 [CWE-563] A variable "Aliñamento" atribúeselles valores dúas veces sucesivas. Quizais isto sexa un erro. Liñas de verificación: 1158, 1160. LoadStoreVectorizer.cpp 1160

Este é un código moi estraño que aparentemente contén un erro lóxico. Ao principio, variable Aliñación asígnase un valor dependendo da condición. E entón a asignación ocorre de novo, pero agora sen ningunha comprobación.

Situacións similares pódense ver aquí:

  • V519 [CWE-563] Á variable 'Efectos' asígnaselles valores dúas veces consecutivas. Quizais isto sexa un erro. Liñas de verificación: 152, 165. WebAssemblyRegStackify.cpp 165
  • V519 [CWE-563] A variable 'ExpectNoDerefChunk' asígnaselle valores dúas veces consecutivas. Quizais isto sexa un erro. Liñas de verificación: 4970, 4973. SemaType.cpp 4973

Fragmento N28: condición sempre verdadeira

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;
  }
  ....
}

Aviso de PVS-Studio: V547 [CWE-571] A expresión 'nextByte != 0x90' sempre é verdadeira. X86DisassemblerDecoder.cpp 379

Comprobar non ten sentido. Variable nextByte sempre non igual ao valor 0x90, que se desprende da comprobación anterior. Este é algún tipo de erro lóxico.

Fragmento N29 - N...: Condicións sempre verdadeiro/falso

O analizador emite moitas advertencias de que toda a condición (V547) ou parte dela (V560) sempre é verdadeiro ou falso. Moitas veces, estes non son erros reais, senón simplemente código descoidado, o resultado da expansión de macros e similares. Non obstante, ten sentido mirar todas estas advertencias, xa que de cando en vez ocorren auténticos erros lóxicos. Por exemplo, esta sección de código é sospeitosa:

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;
  ....
}

Aviso de PVS-Studio: V560 [CWE-570] Unha parte da expresión condicional é sempre falsa: RegNo == 0xe. ARMDisassembler.cpp 939

A constante 0xE é o valor 14 en decimal. Exame RegNo == 0xe non ten sentido porque se Número de rexistro > 13, entón a función completará a súa execución.

Houbo moitas outras advertencias cos ID V547 e V560, pero igual V595, non me interesaba estudar estas advertencias. Xa estaba claro que tiña material suficiente para escribir un artigo :). Polo tanto, descoñécese cantos erros deste tipo se poden identificar en LLVM mediante PVS-Studio.

Vouche dar un exemplo de por que é aburrido estudar estes desencadenantes. O analizador ten toda a razón ao emitir unha advertencia para o seguinte código. Pero isto non é un erro.

bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
                                          tok::TokenKind ClosingBraceKind) {
  bool HasError = false;
  ....
  HasError = true;
  if (!ContinueOnSemicolons)
    return !HasError;
  ....
}

Aviso de PVS-Studio: V547 [CWE-570] A expresión '!HasError' sempre é falsa. UnwrappedLineParser.cpp 1635

Fragmento N30: ​​Retorno sospeitoso

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();
  }
  ....
}

Aviso de PVS-Studio: V612 [CWE-670] Un "retorno" incondicional dentro dun bucle. R600OptimizeVectorRegisters.cpp 63

Este é un erro ou unha técnica específica que pretende explicar algo aos programadores que lean o código. Este deseño non me explica nada e parece moi sospeitoso. É mellor non escribir así :).

Canso? Entón é o momento de facer té ou café.

Buscar erros en LLVM 8 usando o analizador PVS-Studio

Defectos identificados mediante novos diagnósticos

Creo que 30 activacións de diagnósticos antigos son suficientes. Vexamos agora que cousas interesantes se poden atopar cos novos diagnósticos que apareceron despois no analizador anterior cheques. Durante este tempo, engadíronse un total de 66 diagnósticos de propósito xeral ao analizador C++.

Fragmento N31: código inalcanzable

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();
}

Aviso de PVS-Studio: V779 [CWE-561] Detectouse un código inalcanzable. É posible que exista un erro. ExecutionUtils.cpp 146

Como podes ver, ambas ramas do operador if remata cunha chamada ao operador retorno. En consecuencia, o recipiente CtorDtorsByPriority nunca se borrará.

Fragmento N32: código inalcanzable

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;
}

Aviso de PVS-Studio: V779 [CWE-561] Detectouse un código inalcanzable. É posible que exista un erro. LLParser.cpp 835

Interesante situación. Vexamos primeiro este lugar:

return ParseTypeIdEntry(SummaryID);
break;

A primeira vista, parece que aquí non hai ningún erro. Parece o operador break hai un extra aquí, e simplemente podes eliminalo. Non obstante, non todo é tan sinxelo.

O analizador emite un aviso nas liñas:

Lex.setIgnoreColonInIdentifiers(false);
return false;

E de feito, este código é inalcanzable. Todos os casos en cambiar remata cunha chamada do operador retorno. E agora sen sentido só break non parece tan inofensivo! Quizais unha das ramas debería rematar con break, non activado retorno?

Fragmento N33: reinicio aleatorio de bits altos

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);
  ....
}

Aviso de PVS-Studio: V784 O tamaño da máscara de bits é menor que o tamaño do primeiro operando. Isto provocará a perda de bits máis altos. RuntimeDyld.cpp 815

Teña en conta que a función getStubAlignment devolve tipo sen asinar. Calculemos o valor da expresión, supoñendo que a función devolve o valor 8:

~(getStubAlignment() - 1)

~(8u-1)

0xFFFFFFFF8u

Agora observe que a variable DataSize ten un tipo de 64 bits sen asinar. Resulta que ao realizar a operación DataSize & 0xFFFFFFF8u, os trinta e dous bits de orde superior restableceranse a cero. O máis probable é que isto non sexa o que quería o programador. Sospeito que quería calcular: DataSize & 0xFFFFFFFFFFFFFFF8u.

Para corrixir o erro, debes escribir isto:

DataSize &= ~(static_cast<uint64_t>(getStubAlignment()) - 1);

Ou así:

DataSize &= ~(getStubAlignment() - 1ULL);

Fragmento N34: emitir un tipo explícito fallido

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);
  ....
}

Aviso de PVS-Studio: V1028 [CWE-190] Posible desbordamento. Considere lanzar operandos do operador 'NumElts * Scale' ao tipo 'size_t', non ao resultado. X86ISelLowering.h 1577

A conversión de tipos explícitos úsase para evitar o desbordamento ao multiplicar as variables de tipo int. Non obstante, a emisión de tipos explícitos aquí non protexe contra o desbordamento. Primeiro, multiplicaranse as variables e só entón o resultado de 32 bits da multiplicación expandirase ao tipo tamaño_t.

Fragmento N35: Fallou o copiar e pegar

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;
  }
  ....
}

V778 [CWE-682] Atopáronse dous fragmentos de código similares. Quizais este sexa un erro de tipografía e debería usarse a variable "Op1" en lugar de "Op0". InstCombineCompares.cpp 5507

Este novo e interesante diagnóstico identifica situacións nas que se copiou un anaco de código e se comezaron a cambiar algúns nomes nel, pero nun lugar non o corrixiron.

Teña en conta que no segundo bloque cambiaron Op0 en Op1. Pero nun lugar non o arranxaron. O máis probable é que debería escribirse así:

if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
  I.setOperand(1, ConstantFP::getNullValue(Op1->getType()));
  return &I;
}

Fragmento N36: Confusión variable

struct Status {
  unsigned Mask;
  unsigned Mode;

  Status() : Mask(0), Mode(0){};

  Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
    Mode &= Mask;
  };
  ....
};

Aviso de PVS-Studio: V1001 [CWE-563] A variable 'Mode' está asignada pero non se usa ao final da función. SIModeRegister.cpp 48

É moi perigoso dar os mesmos nomes aos argumentos das funcións que aos membros da clase. É moi doado confundirse. Só temos un caso así ante nós. Esta expresión non ten sentido:

Mode &= Mask;

O argumento da función cambia. Iso é todo. Este argumento xa non se usa. Probablemente deberías telo escrito así:

Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
  this->Mode &= Mask;
};

Fragmento N37: Confusión variable

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;
}

Aviso PVS-Studio: V1001 [CWE-563] A variable 'Tamaño' está asignada pero non se usa ao final da función. Obxecto.cpp 424

A situación é semellante á anterior. Debería escribirse:

this->Size += this->EntrySize;

Fragmento N38-N47: esquecéronse de revisar o índice

Anteriormente, analizamos exemplos de desencadenamento diagnóstico V595. A súa esencia é que o punteiro é desreferenciado ao principio, e só despois comprobado. Diagnóstico novo V1004 ten o sentido contrario, pero tamén revela moitos erros. Identifica situacións nas que se comprobou o punteiro ao principio e despois esqueceuse de facelo. Vexamos estes casos atopados dentro de 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());  // <=
  ....
}

Aviso de PVS-Studio: V1004 [CWE-476] O punteiro "Ptr" utilizouse de forma insegura despois de que se verificou contra nullptr. Liñas de verificación: 729, 738. TargetTransformInfoImpl.h 738

Variable Ptr pode ser igual nullptr, tal e como demostra o cheque:

if (Ptr != nullptr)

Non obstante, debaixo deste indicador desreferenciarase sen unha comprobación preliminar:

auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());

Consideremos outro caso semellante.

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(); // <=
  ....
}

Aviso de PVS-Studio: V1004 [CWE-476] O punteiro "FD" utilizouse de forma insegura despois de que se verificou contra nullptr. Liñas de verificación: 3228, 3231. CGDebugInfo.cpp 3231

Preste atención ao sinal FD. Estou seguro de que o problema é claramente visible e non se precisa ningunha explicación especial.

E ademais:

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());     // <=
  ....
}

Aviso de PVS-Studio: V1004 [CWE-476] O punteiro "PtrTy" utilizouse de forma insegura despois de que se verificou contra nullptr. Liñas de verificación: 960, 965. InterleavedLoadCombinePass.cpp 965

Como protexerse de tales erros? Estea máis atento ao Code-Review e use o analizador estático de PVS-Studio para comprobar regularmente o seu código.

De nada vale citar outros fragmentos de código con erros deste tipo. Deixarei só unha lista de avisos no artigo:

  • V1004 [CWE-476] O punteiro "Expr" utilizouse de forma insegura despois de que se verificou contra nullptr. Liñas de verificación: 1049, 1078. DebugInfoMetadata.cpp 1078
  • V1004 [CWE-476] O punteiro "PI" utilizouse de forma insegura despois de que se verificou contra nullptr. Liñas de verificación: 733, 753. LegacyPassManager.cpp 753
  • V1004 [CWE-476] O punteiro 'StatepointCall' utilizouse de forma insegura despois de que se verificou contra nullptr. Liñas de verificación: 4371, 4379. Verifier.cpp 4379
  • V1004 [CWE-476] O punteiro "RV" utilizouse de forma insegura despois de que se verificou contra nullptr. Liñas de verificación: 2263, 2268. TGParser.cpp 2268
  • V1004 [CWE-476] O punteiro 'CalleeFn' utilizouse de forma insegura despois de que se verificou contra nullptr. Liñas de verificación: 1081, 1096. SimplifyLibCalls.cpp 1096
  • V1004 [CWE-476] O punteiro "TC" utilizouse de forma insegura despois de que se verificou contra nullptr. Liñas de verificación: 1819, 1824. Driver.cpp 1824

Fragmento N48-N60: non é crítico, pero é un defecto (posible fuga de memoria)

std::unique_ptr<IRMutator> createISelMutator() {
  ....
  std::vector<std::unique_ptr<IRMutationStrategy>> Strategies;
  Strategies.emplace_back(
      new InjectorIRStrategy(InjectorIRStrategy::getDefaultOps()));
  ....
}

Aviso de PVS-Studio: V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Estratexias" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. llvm-isel-fuzzer.cpp 58

Para engadir un elemento ao final dun recipiente como std::vector > non podes simplemente escribir xxx.push_back(nova X), xa que non hai conversión implícita de X* в std::unique_ptr.

Unha solución común é escribir xxx.emplace_back(novo X)xa que compila: método emplace_back constrúe un elemento directamente a partir dos seus argumentos e, polo tanto, pode usar construtores explícitos.

Non é seguro. Se o vector está cheo, a memoria reasignarase. A operación de reasignación de memoria pode fallar, provocando que se lance unha excepción std::bad_alloc. Neste caso, o punteiro perderase e nunca se eliminará o obxecto creado.

Unha solución segura é crear único_ptrque será o propietario do punteiro antes de que o vector intente reasignar memoria:

xxx.push_back(std::unique_ptr<X>(new X))

Desde C++14, podes usar 'std::make_unique':

xxx.push_back(std::make_unique<X>())

Este tipo de defecto non é crítico para LLVM. Se non se pode asignar memoria, o compilador simplemente parará. Non obstante, para aplicacións con longo tempo de actividade, que non pode rematar se a asignación de memoria falla, isto pode ser un verdadeiro erro desagradable.

Entón, aínda que este código non supón unha ameaza práctica para LLVM, pareceume útil falar deste patrón de erro e de que o analizador PVS-Studio aprendeu a identificalo.

Outros avisos deste tipo:

  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contedor "Pasos" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. PassManager.h 546
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "AAs" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. AliasAnalysis.h 324
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Entradas" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. DWARFDebugFrame.cpp 519
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "AllEdges" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. CFGMST.h 268
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "VMaps" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. SimpleLoopUnswitch.cpp 2012
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Rexistros" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. FDRLogBuilder.h 30
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor 'PendingSubmodules' co método 'emplace_back'. En caso de excepción, producirase unha fuga de memoria. ModuleMap.cpp 810
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Obxectos" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. DebugMap.cpp 88
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Estratexias" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. llvm-isel-fuzzer.cpp 60
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Modificadores" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. llvm-stress.cpp 685
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Modificadores" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. llvm-stress.cpp 686
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Modificadores" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. llvm-stress.cpp 688
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Modificadores" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. llvm-stress.cpp 689
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Modificadores" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. llvm-stress.cpp 690
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Modificadores" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. llvm-stress.cpp 691
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Modificadores" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. llvm-stress.cpp 692
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Modificadores" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. llvm-stress.cpp 693
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Modificadores" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. llvm-stress.cpp 694
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Operandos" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. GlobalISelEmitter.cpp 1911
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Alixo" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. GlobalISelEmitter.cpp 2100
  • V1023 [CWE-460] Engádese un punteiro sen propietario ao contenedor "Matchers" polo método "emplace_back". En caso de excepción, producirase unha fuga de memoria. GlobalISelEmitter.cpp 2702

Conclusión

En total publiquei 60 avisos e despois parei. Hai outros defectos que o analizador PVS-Studio detecta en LLVM? Si teño. Non obstante, cando estaba a escribir fragmentos de código para o artigo, era tarde, ou mellor dito, de noite, e decidín que era hora de chamalo día.

Espero que vos resulte interesante e queredes probar o analizador PVS-Studio.

Podes descargar o analizador e obter a clave do buscaminas en Esta páxina.

O máis importante é que use a análise estática regularmente. Controles únicos, realizado por nós co fin de popularizar a metodoloxía da análise estática e PVS-Studio non son un escenario normal.

Moita sorte na mellora da calidade e fiabilidade do teu código!

Buscar erros en LLVM 8 usando o analizador PVS-Studio

Se queres compartir este artigo cun público de fala inglesa, utiliza a ligazón de tradución: Andrey Karpov. Buscando erros en LLVM 8 con PVS-Studio.

Fonte: www.habr.com

Engadir un comentario