ProHoster > Blog > Administración > Encontrando errores en LLVM 8 con el analizador PVS-Studio

Encontrando errores en LLVM 8 con el analizador PVS-Studio

Encontramos errores en LLVM 8 con el analizador PVS-Studio
Han pasado más de dos años desde la última revisión del código del proyecto LLVM con nuestro analizador PVS-Studio. Asegurémonos de que el analizador PVS-Studio sigue siendo la herramienta líder en la detección de errores y vulnerabilidades potenciales. Para ello, revisaremos y encontraremos nuevos errores en la versión LLVM 8.0.0.

Artículo que debe ser escrito

Honestamente, no quería escribir este artículo. No es interesante escribir sobre un proyecto que ya hemos revisado varias veces (1, 2, 3). Prefiero escribir sobre algo nuevo, pero no tengo otra opción.

Cada vez que se lanza una nueva versión de LLVM o se actualiza Clang Static Analyzer, recibimos preguntas del siguiente tipo en nuestro correo:

¡Miren, la nueva versión de Clang Static Analyzer ha aprendido a encontrar nuevos errores! Me parece que la relevancia de usar PVS-Studio está disminuyendo. Clang encuentra más errores que antes y está alcanzando las capacidades de PVS-Studio. ¿Qué piensan al respecto?

Siempre quiero responder algo en el sentido de:

¡Nosotros también estamos en actividad! Hemos mejorado significativamente las capacidades del analizador PVS-Studio. Así que no se preocupen, seguimos liderando como antes.

Lamentablemente, esta es una mala respuesta. No tiene pruebas. Y es precisamente por eso que estoy escribiendo este artículo ahora. Así que el proyecto LLVM ha sido revisado una vez más y se han encontrado diversos errores. Aquellos que me parecieron interesantes los mostraré ahora. Estos errores no pueden ser encontrados por Clang Static Analyzer (o es muy incómodo hacerlo con su ayuda). Pero nosotros podemos. De hecho, encontré y anoté todos estos errores en una sola noche.

Pero la escritura del artículo se ha alargado durante varias semanas. No podía obligarme a poner todo esto en forma de texto :).

Por cierto, si les interesa qué tecnologías se utilizan en el analizador PVS-Studio para detectar errores y vulnerabilidades potenciales, les propongo conocer esta nota.

Diagnósticos nuevos y antiguos

Como ya se mencionó, hace aproximadamente dos años el proyecto LLVM fue revisado nuevamente y los errores encontrados fueron corregidos. Ahora, en este artículo, se presentará una nueva tanda de errores. ¿Por qué se encontraron nuevos errores? Hay 3 razones para esto:

  1. El proyecto LLVM está en desarrollo; el código viejo se modifica y aparece nuevo código. Naturalmente, en el código modificado y nuevo hay errores. Esto demuestra bien que el análisis estático debe aplicarse regularmente, y no de manera ocasional. Nuestros artículos muestran bien las capacidades del analizador PVS-Studio, pero esto no está relacionado con mejorar la calidad del código y reducir el costo de corregir errores. ¡Utilice un analizador de código estático de forma regular!
  2. Estamos mejorando y perfeccionando diagnósticos ya existentes. Por lo tanto, el analizador puede detectar errores que no se notaron en revisiones anteriores.
  3. PVS-Studio ha introducido nuevos diagnósticos que no existían hace 2 años. He decidido destacarlos en una sección separada para mostrar visualmente el desarrollo de PVS-Studio.

Defectos detectados por diagnósticos que existían hace 2 años

Fragmento N1: Copiar y Pegar

static bool ShouldUpgradeX86Intrinsic(Function *F, StringRef Name) {
  if (Name == "addcarryx.u32" || // Agregado en 8.0
    ....
    Name == "avx512.mask.cvtps2pd.128" || // Agregado en 7.0
    Name == "avx512.mask.cvtps2pd.256" || // Agregado en 7.0
    Name == "avx512.cvtusi2sd" || // Agregado en 7.0
    Name.startswith("avx512.mask.permvar.") || // Agregado en 7.0     // <=
    Name.startswith("avx512.mask.permvar.") || // Agregado en 7.0     // <=
    Name == "sse2.pmulu.dq" || // Agregado en 7.0
    Name == "sse41.pmuldq" || // Agregado en 7.0
    Name == "avx2.pmulu.dq" || // Agregado en 7.0
  ....
}

Advertencia PVS-Studio: V501 [CWE-570] Hay subexpresiones idénticas ‘Name.startswith(«avx512.mask.permvar.»)’ a la izquierda y a la derecha del operador ‘||’. AutoUpgrade.cpp 73

Se comprueba dos veces que el nombre comienza con la subcadena «avx512.mask.permvar.» En la segunda verificación, claramente se quería escribir algo más, pero olvidaron corregir el texto copiado.

Fragmento N2: Error 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;
  ....
}

Advertencia PVS-Studio: V501 Hay subexpresiones idénticas ‘CXNameRange_WantQualifier’ a la izquierda y a la derecha del operador ‘|’. CIndex.cpp 7245

Debido a un error tipográfico, se utiliza la misma constante nombrada dos veces CXNameRange_WantQualifier.

Fragmento N3: Confusión con las prioridades de los operadores

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

Advertencia PVS-Studio: V502 [CWE-783] Quizás el operador ‘?:’ funcione de una manera diferente a la esperada. El operador ‘?:’ tiene una prioridad más baja que el operador ‘==’. PPCTargetTransformInfo.cpp 404

En mi opinión, es un error muy bonito. Sí, sé que tengo visiones extrañas sobre la belleza :)

Ahora, según las prioridades de los operadores, la expresión se calcula de la siguiente manera:

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

Desde un punto de vista práctico, tal condición no tiene sentido, ya que se puede simplificar a:

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

Esto es un error evidente. Es probable que se quisiera comparar 0/1 con la variable Index. Para corregir el código, es necesario agregar paréntesis alrededor del operador ternario:

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

Por cierto, el operador ternario es muy peligroso y provoca errores lógicos. Ten mucho cuidado con él y no seas tacaño al poner paréntesis. Más sobre este tema lo traté aquí, en el capítulo 'Tengan cuidado con el operador ?: y enciérrenlo en paréntesis'.

Fragmento N4, N5: Puntero nulo

Init *TGParser::ParseValue(Record *CurRec, RecTy *ItemType, IDParseMode Mode) {
  ....
  TypedInit *LHS = dyn_cast(Result);
  ....
  LHS = dyn_cast(
    UnOpInit::get(UnOpInit::CAST, LHS, StringRecTy::get())
      ->Fold(CurRec));
  if (!LHS) {
    Error(PasteLoc, Twine("no se puede transformar '") + LHS->getAsString() +
                    "' a string");
    return nullptr;
  }
  ....
}

Advertencia PVS-Studio: V522 [CWE-476] Puede ocurrir la desreferencia del puntero nulo ‘LHS’. TGParser.cpp 2152

Si el puntero LHS resulta ser nulo, debería emitirse una advertencia. Sin embargo, en su lugar ocurrirá la desreferencia de ese mismo puntero nulo: LHS->getAsString().

Esta es una situación muy típica, donde el error se oculta en el manejador de errores, ya que nadie los prueba. Los analizadores estáticos verifican todo el código alcanzable, independientemente de cuán a menudo se utiliza. Este es un muy buen ejemplo de cómo el análisis estático complementa otros métodos de prueba y defensa contra errores.

Un error similar en el manejo del puntero RHS se ha cometido en el código un poco más abajo: V522 [CWE-476] Puede ocurrir la desreferencia del puntero nulo ‘RHS’. TGParser.cpp 2186

Fragmento N6: Uso del puntero después del movimiento

static Expected
ExtractBlocks(....)
{
  ....
  std::unique_ptr ProgClone = CloneModule(BD.getProgram(), VMap);
  ....
  BD.setNewProgram(std::move(ProgClone));                                // getFunction(MisCompFunctions[i].first);  // <=
    assert(NewF && "¿Función no encontrada??");
    MiscompiledFunctions.push_back(NewF);
  }
  ....
}

Advertencia PVS-Studio: V522 [CWE-476] Puede ocurrir la desreferencia del puntero nulo ‘ProgClone’. Miscompilation.cpp 601

Al principio, el puntero inteligente ProgClone deja de poseer el objeto:

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

De hecho, ahora ProgClone — es un puntero nulo. Por lo tanto, debe ocurrir la desreferenciación del puntero nulo más abajo:

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

Pero, en realidad, eso no sucederá. Tenga en cuenta que el bucle en realidad no se ejecuta.

Al principio del contenedor MiscompiledFunctions se limpia:

MiscompiledFunctions.clear();

Luego, el tamaño de este contenedor se utiliza en la condición del bucle:

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

Es fácil ver que el bucle no se inicia. Creo que esto también es un error, y el código debería estar escrito de otra manera.

Parece que hemos encontrado esa famosa paridad de errores. Un error oculta a otro :).

Fragmento N7: Uso de un puntero después de moverlo

static Expected TestOptimizer(BugDriver &BD, std::unique_ptr Test,
                                    std::unique_ptr Safe) {
  outs() << "  Optimizando las funciones en prueba: ";
  std::unique_ptr Optimized =
      BD.runPassesOn(Test.get(), BD.getPassesToRun());
  if (!Optimized) {
    errs() << " Error al ejecutar esta secuencia de pasos"
           << " en el programa de entrada!n";
    BD.setNewProgram(std::move(Test));                       // <=
    BD.EmitProgressBitcode(*Test, "pass-error", false);      // <=
    if (Error E = BD.debugOptimizerCrash())
      return std::move(E);
    return false;
  }
  ....
}

Advertencia PVS-Studio: V522 [CWE-476] La desreferenciación del puntero nulo ‘Test’ podría ocurrir. Miscompilation.cpp 709

Una vez más, la misma situación. Al principio, el contenido del objeto se mueve, y luego se utiliza como si nada hubiera pasado. Me estoy encontrando cada vez más esta situación en el código de programas, desde que C++ introdujo la semántica de movimiento. ¡Es por eso que me encanta el lenguaje C++! Surgen cada vez más maneras de dispararse en el pie. El analizador PVS-Studio siempre tendrá trabajo :).

Fragmento N8: Puntero nulo

void FunctionDumper::dump(const PDBSymbolTypeFunctionArg &Symbol) {
  uint32_t TypeId = Symbol.getTypeId();
  auto Type = Symbol.getSession().getSymbolById(TypeId);
  if (Type)
    Printer << "";
  else
    Type->dump(*this);
}

Advertencia PVS-Studio: V522 [CWE-476] La desreferenciación del puntero nulo ‘Type’ podría ocurrir. PrettyFunctionDumper.cpp 233

Además de los controladores de errores, generalmente no se prueban las funciones de impresión de datos de depuración. Este es precisamente un caso de eso. La función espera que el usuario, en lugar de resolver sus problemas, se vea obligado a corregirla.

Correcto:

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

Fragmento N9: Puntero nulo

void SearchableTableEmitter::collectTableEntries(
    GenericTable &Table, const std::vector &Items) {
  ....
  RecTy *Ty = resolveTypes(Field.RecType, TI->getType());
  if (!Ty)                                                              // getAsString() + " vs. " +                       // getType()->getAsString());
   ....
}

Advertencia de PVS-Studio: V522 [CWE-476] Puede ocurrir desreferencia del puntero nulo 'Ty'. SearchableTableEmitter.cpp 614

Creo que esto está claro y no requiere aclaraciones.

Fragmento N10: Error 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;
}

Advertencia PVS-Studio: V570 La variable 'Identifier->Type' se asigna a sí misma. FormatTokenLexer.cpp 249

No tiene sentido asignar una variable a sí misma. Probablemente se quería escribir:

Identifier->Type = Question->Type;

Fragmento N11: Break sospechoso

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

Advertencia PVS-Studio: V622 [CWE-478] Considere inspeccionar la declaración 'switch'. Es posible que falte el primer operador 'case'. SystemZAsmParser.cpp 652

Al principio hay un operador muy sospechoso break. ¿Olvidaron escribir algo más aquí?

Fragmento N12: Verificación del puntero después de desreferenciarlo

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

Advertencia PVS-Studio: V595 [CWE-476] Se utilizó el puntero 'Callee' antes de verificarse contra nullptr. Verifique las líneas: 172, 174. AMDGPUInline.cpp 172

Puntero Callee se desreferencia al principio en el momento de llamar a la función getTTI.

Y luego resulta que este puntero debe ser verificado por igual nullptr:

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

Pero ya es tarde...

Fragmento N13 — N…: Verificación del puntero después de desreferenciarlo

La situación abordada en el fragmento de código anterior no es única. Se encuentra aquí:

static Value *optimizeDoubleFP(CallInst *CI, IRBuilder &B,
                               bool isBinary, bool isPrecise = false) {
  ....
  Function *CalleeFn = CI->getCalledFunction();
  StringRef CalleeNm = CalleeFn->getName();                 // getAttributes();
  if (CalleeFn && !CalleeFn->isIntrinsic()) {               // <=
  ....
}

Advertencia de PVS-Studio: V595 [CWE-476] Se utilizó el puntero 'CalleeFn' antes de verificarse contra nullptr. Verifique las líneas: 1079, 1081. SimplifyLibCalls.cpp 1079

Y aquí:

void Sema::InstantiateAttrs(const MultiLevelTemplateArgumentList &TemplateArgs,
                            const Decl *Tmpl, Decl *New,
                            LateInstantiatedAttrVec *LateAttrs,
                            LocalInstantiationScope *OuterMostScope) {
  ....
  NamedDecl *ND = dyn_cast(New);
  CXXRecordDecl *ThisContext =
    dyn_cast_or_null(ND->getDeclContext());         // isCXXInstanceMember());     // <=
  ....
}

Advertencia de PVS-Studio: V595 [CWE-476] El puntero ‘ND’ se utilizó antes de ser verificado contra nullptr. Verifique las líneas: 532, 534. SemaTemplateInstantiateDecl.cpp 532

Y aquí:

  • V595 [CWE-476] El puntero ‘U’ se utilizó antes de ser verificado contra nullptr. Verifique las líneas: 404, 407. DWARFFormValue.cpp 404
  • V595 [CWE-476] El puntero ‘ND’ se utilizó antes de ser verificado contra nullptr. Verifique las líneas: 2149, 2151. SemaTemplateInstantiate.cpp 2149

Y luego, me dejó de interesar estudiar las advertencias con el número V595. Así que no sé si hay más errores similares, aparte de los listados aquí. Seguramente sí.

Fragmento N17, N18: Desplazamiento sospechoso

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

Advertencia PVS-Studio: V629 [CWE-190] Considere inspeccionar la expresión ‘~(Size — 1) << 1’. Desplazamiento de bits del valor de 32 bits con una posterior expansión al tipo de 64 bits. AArch64AddressingModes.h 260

Puede que no sea un error y que el código funcione exactamente como se pretendía. Pero es claramente un lugar muy sospechoso y debe ser revisado.

Supongamos que la variable Tamaño es igual a 16, y entonces el autor del código planeaba obtener en la variable NImms el valor:

1111111111111111111111111111111111111111111111111111111111100000

Sin embargo, en realidad, obtendrá el valor:

0000000000000000000000000000000011111111111111111111111111100000

El hecho es que todos los cálculos se realizan utilizando el tipo unsigned de 32 bits. Y solo después, este tipo sin signo de 32 bits se expandirá implícitamente a uint64_t. En este proceso, los bits superiores serán ceros.

La situación se puede corregir así:

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

Situación similar: V629 [CWE-190] Considere inspeccionar la expresión ‘Immr << 6’. Desplazamiento de bits del valor de 32 bits con una posterior expansión al tipo de 64 bits. AArch64AddressingModes.h 269

Fragmento N19: Falta una palabra clave else?

void AMDGPUAsmParser::cvtDPP(MCInst &Inst, const OperandVector &Operands) {
  ....
  if (Op.isReg() && Op.Reg.RegNo == AMDGPU::VCC) {
    // VOP2b (v_add_u32, v_sub_u32 ...) uso de dpp "vcc" token.
    // Saltar esto.
    continue;
  } if (isRegOrImmWithInputMods(Desc, Inst.getNumOperands())) {    // <=
    Op.addRegWithFPInputModsOperands(Inst, 2);
  } else if (Op.isDPPCtrl()) {
    Op.addImmOperands(Inst, 1);
  } else if (Op.isImm()) {
    // Manejar argumentos opcionales
    OptionalIdx[Op.getImmTy()] = I;
  } else {
    llvm_unreachable("Tipo de operando inválido");
  }
  ....
}

Advertencia PVS-Studio: V646 [CWE-670] Considere inspeccionar la lógica de la aplicación. Es posible que falte la palabra clave ‘else’. AMDGPUAsmParser.cpp 5655

No hay error aquí. Dado que el bloque then del primero if termina en continue, por lo que no importa si hay una palabra clave else o no. De todos modos, el código funcionará de la misma manera. Sin embargo, un código omitido else hace que el código sea más confuso y peligroso. Si en el futuro continue se elimina, el código comenzará a funcionar de una manera completamente diferente. En mi opinión, es mejor agregar else.

Fragmento N20: Cuatro errores tipográficos idénticos

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

Advertencias de PVS-Studio:

  • V655 [CWE-480] Las cadenas fueron concatenadas pero no se utilizan. Considere inspeccionar la expresión ‘Result + Name.str()’. Symbol.cpp 32
  • V655 [CWE-480] Las cadenas fueron concatenadas pero no se utilizan. Considere inspeccionar la expresión ‘Result + «(ObjC Class) » + Name.str()’. Symbol.cpp 35
  • V655 [CWE-480] Las cadenas fueron concatenadas pero no se utilizan. Considere inspeccionar la expresión ‘Result + «(ObjC Class EH) » + Name.str()’. Symbol.cpp 38
  • V655 [CWE-480] Las cadenas fueron concatenadas pero no se utilizan. Considere inspeccionar la expresión ‘Result + «(ObjC IVar) » + Name.str()’. Symbol.cpp 41

Por accidente, en lugar del operador += se usa el operador +. Como resultado, se obtienen construcciones sin sentido.

Fragmento N21: Comportamiento 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 no puede estar vacío");

    for (auto &Op : Ops) {
      assert(!Op.empty() && "Operador vacío");
      if (FeaturesMap.find(Op) == FeaturesMap.end())
        FeaturesMap[Op] = FeaturesMap.size();
    }
  }
}

Intenta encontrar código peligroso por ti mismo. Y esta es una imagen para distraer la atención, para no mirar la respuesta de inmediato:

Encontramos errores en LLVM 8 con el analizador PVS-Studio

Advertencia PVS-Studio: V708 [CWE-758] Se está utilizando una construcción peligrosa: ‘FeaturesMap[Op] = FeaturesMap.size()’, donde ‘FeaturesMap’ es de la clase ‘map’. Esto puede llevar a un comportamiento indefinido. RISCVCompressInstEmitter.cpp 490

La línea problemática:

FeaturesMap[Op] = FeaturesMap.size();

Si el elemento Op no se encuentra, se crea un nuevo elemento en el mapa y se registra el número de elementos en este mapa. Pero no se sabe si se llamará a la función tamaño antes o después de añadir un nuevo elemento.

Fragmento N22-N24: Asignaciones 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;
  }
  ....
}

Advertencia PVS-Studio: V519 [CWE-563] La variable ‘NType’ se asigna valores dos veces sucesivas. Puede que esto sea un error. Verifique líneas: 1663, 1664. MachOObjectFile.cpp 1664

No creo que haya un error real aquí. Simplemente hay una asignación repetida innecesaria. Pero sigue siendo un error.

De manera similar:

  • V519 [CWE-563] La variable ‘B.NDesc’ se asigna valores dos veces sucesivas. Puede que esto sea un error. Verifique líneas: 1488, 1489. llvm-nm.cpp 1489
  • V519 [CWE-563] La variable se asigna valores dos veces sucesivas. Puede que esto sea un error. Verifique líneas: 59, 61. coff2yaml.cpp 61

Fragmento N25-N27: Más asignaciones repetidas

Ahora consideremos una variante un poco diferente de la asignación repetida.

bool Vectorizer::vectorizeLoadChain(
    ArrayRef Chain,
    SmallPtrSet *InstructionsProcessed) {
  ....
  unsigned Alignment = getAlignment(L0);
  ....
  unsigned NewAlign = getOrEnforceKnownAlignment(L0->getPointerOperand(),
                                                 StackAdjustedAlignment,
                                                 DL, L0, nullptr, &DT);
  if (NewAlign != 0)
    Alignment = NewAlign;
  Alignment = NewAlign;
  ....
}

Advertencia PVS-Studio: V519 [CWE-563] La variable ‘Alignment’ se asigna valores dos veces sucesivas. Puede que esto sea un error. Verifique líneas: 1158, 1160. LoadStoreVectorizer.cpp 1160

Este es un código muy extraño, que aparentemente contiene un error lógico. Al principio, la variable Alignment se le asigna un valor dependiendo de una condición. Luego, vuelve a haber una asignación, pero ahora sin ninguna verificación.

Situaciones similares se pueden ver aquí:

  • V519 [CWE-563] La variable ‘Effects’ se asigna valores dos veces sucesivas. Puede que esto sea un error. Verifique líneas: 152, 165. WebAssemblyRegStackify.cpp 165
  • V519 [CWE-563] La variable ‘ExpectNoDerefChunk’ se asigna valores dos veces sucesivas. Puede que esto sea un error. Verifique líneas: 4970, 4973. SemaType.cpp 4973

Fragmento N28: Condición siempre verdadera

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) // Soporte de instrucción PAUSE             // <=
      break;
  }
  ....
}

Advertencia PVS-Studio: V547 [CWE-571] La expresión ‘nextByte != 0x90’ es siempre verdadera. X86DisassemblerDecoder.cpp 379

La verificación no tiene sentido. La variable nextByte siempre no es igual al valor 0x90, lo que se deduce de la verificación anterior. Esto es un error lógico.

Fragmento N29 — N…: Condiciones siempre verdaderas/falsas

El analizador emite muchas advertencias acerca de que toda la condición (V547) o su parte (V560) siempre es verdadero o falso. A menudo, esto no son errores reales, sino simplemente código descuidado, resultado de la implementación de macros, y similar. Sin embargo, tiene sentido revisar todas estas advertencias, ya que de vez en cuando se encuentran errores lógicos reales. Por ejemplo, este fragmento de código es sospechoso:

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

Advertencia PVS-Studio: V560 [CWE-570] Una parte de la expresión condicional siempre es falsa: RegNo == 0xe. ARMDisassembler.cpp 939

La constante 0xE es el valor 14 en el sistema decimal. La comprobación RegNo == 0xe no tiene sentido, ya que si RegNo > 13, la función terminará su ejecución.

Hubo muchas otras advertencias con identificadores V547 y V560, pero, al igual que con V595, estudiar estas advertencias no me interesaba. Ya estaba claro que tenía suficiente material para escribir un artículo :). Por lo tanto, no se sabe cuántos errores de este tipo se pueden identificar en LLVM utilizando PVS-Studio.

Daré un ejemplo de por qué estudiar estas activaciones es aburrido. El analizador tiene toda la razón al emitir una advertencia sobre el siguiente código. Pero eso no es un error.

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

Advertencia de PVS-Studio: V547 [CWE-570] La expresión ‘!HasError’ siempre es falsa. UnwrappedLineParser.cpp 1635

Fragmento N30: retorno sospechoso

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

Advertencia PVS-Studio: V612 [CWE-670] Un ‘return’ incondicional dentro de un bucle. R600OptimizeVectorRegisters.cpp 63

Esto es un error, o una técnica específica destinada a aclarar algo a los programadores que están leyendo el código. Para mí, tal construcción no aclara nada y parece muy sospechosa. Es mejor no escribir así :).

¿Te has cansado? Entonces es hora de preparar un té o café.

Encontramos errores en LLVM 8 con el analizador PVS-Studio

Defectos identificados con los nuevos diagnósticos

Creo que 30 activaciones de diagnósticos antiguos son suficientes. Ahora veamos qué cosas interesantes se pueden encontrar con los nuevos diagnósticos que se han agregado al analizador después de la anterior comprobación. En total, durante este tiempo se han añadido 66 diagnósticos de propósito general al analizador de 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();
}

Advertencia PVS-Studio: V779 [CWE-561] Código inalcanzable detectado. Es posible que haya un error presente. ExecutionUtils.cpp 146

Como pueden ver, ambas ramas del operador if terminan con la llamada al operador return. Por lo tanto, el contenedor CtorDtorsByPriority nunca será limpiado.

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(), "tipo de resumen inesperado");
  }
  Lex.setIgnoreColonInIdentifiers(false);                      // <=
  return false;
}

Advertencia PVS-Studio: V779 [CWE-561] Código inalcanzable detectado. Es posible que haya un error presente. LLParser.cpp 835

Situación interesante. Comencemos a revisar este lugar:

return ParseTypeIdEntry(SummaryID);
break;

A primera vista, parece que no hay errores aquí. Parece que el operador break es redundante y se puede eliminar. Sin embargo, no es tan simple.

El analizador emite una advertencia en las líneas:

Lex.setIgnoreColonInIdentifiers(false);
return false;

Y de hecho, este código es inalcanzable. Todos los casos en switch terminan con una llamada al operador return. ¡Y ahora, el solitario break no parece tan inofensivo! Quizás una de las ramas deba terminar en break, y no en return?

Fragmento N33: Restablecimiento aleatorio de bits superiores

unsigned getStubAlignment() override {
  if (Arch == Triple::systemz)
    return 8;
  else
    return 1;
}

Expected
RuntimeDyldImpl::emitSection(const ObjectFile &Obj,
                             const SectionRef &Section,
                             bool IsCode) {
  ....
  uint64_t DataSize = Section.getSize();
  ....
  if (StubBufSize > 0)
    DataSize &= ~(getStubAlignment() - 1);
  ....
}

Advertencia PVS-Studio: V784 El tamaño de la máscara de bits es menor que el tamaño del primer operando. Esto causará la pérdida de bits superiores. RuntimeDyld.cpp 815

Observe que la función getStubAlignment devuelve el tipo unsigned. Calculemos el valor de la expresión, suponiendo que la función retorne el valor 8:

~(getStubAlignment() - 1)

~(8u-1)

0xFFFFFFF8‬u

Ahora, observe que la variable DataSize tiene un tipo sin signo de 64 bits. Por lo tanto, al realizar la operación DataSize & 0xFFFFFFF8‬u, los treinta y dos bits superiores serán reiniciados. Lo más probable es que esto no es lo que el programador quería. Sospecho que quería calcular: DataSize & 0xFFFFFFFFFFFFFFF8‬u.

Para corregir el error, debería escribirse así:

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

O así:

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

Fragmento N34: Conversión de tipo explícita fallida

template <typename T>
void scaleShuffleMask(int Scale, ArrayRef<T> Mask,
                      SmallVectorImpl<T> &ScaledMask) {
  assert(0 < Scale && "Factor de escala inesperado");
  int NumElts = Mask.size();
  ScaledMask.assign(static_cast<size_t>(NumElts * Scale), -1);
  ....
}

Advertencia PVS-Studio: V1028 [CWE-190] Posible desbordamiento. Considere convertir los operandos del operador ‘NumElts * Scale’ al tipo ‘size_t’, no el resultado. X86ISelLowering.h 1577

La conversión de tipo explícita se utiliza para evitar el desbordamiento al multiplicar variables del tipo int. Sin embargo, aquí la conversión de tipo explícita no protege contra el desbordamiento. Primero se multiplicarán las variables y luego el resultado de 32 bits se ampliará al tipo size_t.

Fragmento N35: Copy-Paste fallido

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] Se encontraron dos fragmentos de código similares. Quizás, esto es un error tipográfico y se debería usar la variable ‘Op1’ en lugar de ‘Op0’. InstCombineCompares.cpp 5507

Este nuevo diagnóstico interesante detecta situaciones en las que un fragmento de código fue copiado y algunos nombres comenzaron a cambiarse, pero en un lugar no se corrigió.

Tenga en cuenta que en el segundo bloque se cambió Op0 en Op1. Pero en un lugar no se corrigió. Lo más probable es que debería haberse escrito así:

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

Fragmento N36: Confusión en las variables

struct Status {
  unsigned Mask;
  unsigned Mode;

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

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

Advertencia PVS-Studio: V1001 [CWE-563] La variable ‘Mode’ se asigna pero no se utiliza al final de la función. SIModeRegister.cpp 48

Es muy peligroso dar a los argumentos de las funciones los mismos nombres que a los miembros de la clase. Es muy fácil confundirse. Este es precisamente el caso. Esta expresión no tiene sentido:

Mode &= Mask;

Se cambia el argumento de la función. Y eso es todo. Este argumento no se utiliza más. Lo más probable es que se debería haber escrito así:

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

Fragmento N37: Confusión en las variables

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(Sym));
  Size += this->EntrySize;
}

Advertencia PVS-Studio: V1001 [CWE-563] La variable 'Size' se asigna pero no se utiliza al final de la función. Object.cpp 424

La situación es similar a la anterior. Debe escribirse:

this->Size += this->EntrySize;

Fragmento N38-N47: Se olvidó verificar el puntero

Antes hemos revisado ejemplos de activación de diagnósticos V595. Su esencia es que el puntero se desreferencia al principio y luego se verifica. Diagnóstico nuevo V1004 es opuesta en significado, pero también detecta muchos errores. Detecta situaciones en las que se verificó el puntero al principio, pero luego se olvidaron de hacerlo. Consideremos tales casos encontrados dentro de LLVM.

int getGEPCost(Type *PointeeType, const Value *Ptr,
               ArrayRef Operands) {
  ....
  if (Ptr != nullptr) {                                            // <=
    assert(....);
    BaseGV = dyn_cast(Ptr->stripPointerCasts());
  }
  bool HasBaseReg = (BaseGV == nullptr);

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

Advertencia PVS-Studio: V1004 [CWE-476] El puntero 'Ptr' se utilizó de manera insegura después de haber sido verificado contra nullptr. Verifique las líneas: 729, 738. TargetTransformInfoImpl.h 738

Variable Ptr puede ser igual a nullptr, como lo indica la verificación:

if (Ptr != nullptr)

Sin embargo, abajo este puntero se desreferencia ya sin una verificación previa:

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

Consideremos otro caso similar.

llvm::DISubprogram *CGDebugInfo::getFunctionFwdDeclOrStub(GlobalDecl GD,
                                                          bool Stub) {
  ....
  auto *FD = dyn_cast(GD.getDecl());
  SmallVector ArgTypes;
  if (FD)                                                                // parameters())
      ArgTypes.push_back(Parm->getType());
  CallingConv CC = FD->getType()->castAs()->getCallConv(); // <=
  ....
}

Advertencia PVS-Studio: V1004 [CWE-476] El puntero 'FD' se utilizó de manera insegura después de haber sido verificado contra nullptr. Verifique las líneas: 3228, 3231. CGDebugInfo.cpp 3231

Preste atención al puntero FD. Estoy seguro de que el problema se ve claramente y no se requieren explicaciones especiales.

Y además:

static void computePolynomialFromPointer(Value &Ptr, Polynomial &Result,
                                         Value *&BasePtr,
                                         const DataLayout &DL) {
  PointerType *PtrTy = dyn_cast(Ptr.getType());
  if (!PtrTy) {                                                   // getPointerAddressSpace());     // <=
  ....
}

Advertencia de PVS-Studio: V1004 [CWE-476] El puntero 'PtrTy' se utilizó de manera insegura después de haber sido verificado en relación con nullptr. Revisa las líneas: 960, 965. InterleavedLoadCombinePass.cpp 965

¿Cómo protegerse de tales errores? Presta más atención durante la revisión de código y utiliza el analizador estático PVS-Studio para revisiones regulares del código.

No tiene sentido presentar otros fragmentos de código con errores de este tipo. Dejaré en el artículo solo la lista de advertencias:

  • V1004 [CWE-476] El puntero 'Expr' se utilizó de manera insegura después de haber sido verificado en relación con nullptr. Revisa las líneas: 1049, 1078. DebugInfoMetadata.cpp 1078
  • V1004 [CWE-476] El puntero 'PI' se utilizó de manera insegura después de haber sido verificado en relación con nullptr. Revisa las líneas: 733, 753. LegacyPassManager.cpp 753
  • V1004 [CWE-476] El puntero 'StatepointCall' se utilizó de manera insegura después de haber sido verificado en relación con nullptr. Revisa las líneas: 4371, 4379. Verifier.cpp 4379
  • V1004 [CWE-476] El puntero 'RV' se utilizó de manera insegura después de haber sido verificado en relación con nullptr. Revisa las líneas: 2263, 2268. TGParser.cpp 2268
  • V1004 [CWE-476] El puntero 'CalleeFn' se utilizó de manera insegura después de haber sido verificado en relación con nullptr. Revisa las líneas: 1081, 1096. SimplifyLibCalls.cpp 1096
  • V1004 [CWE-476] El puntero 'TC' se utilizó de manera insegura después de haber sido verificado en relación con nullptr. Revisa las líneas: 1819, 1824. Driver.cpp 1824

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

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

Advertencia PVS-Studio: V1023 [CWE-460] Un puntero sin propietario se añade al contenedor 'Strategies' mediante el método 'emplace_back'. Se producirá una fuga de memoria en caso de una excepción. llvm-isel-fuzzer.cpp 58

Para añadir un elemento al final de un contenedor de tipo std::vector<std::unique_ptr> no se puede simplemente escribir xxx.push_back(new X), ya que no hay una conversión implícita de X* en std::unique_ptr.

Una solución común es escribir xxx.emplace_back(new X), ya que compila: el método emplace_back construye el elemento directamente desde los argumentos y, por ende, puede utilizar constructores explícitos.

Esto no es seguro. Si el vector está lleno, ocurrirá una realocación de memoria. La operación de realocación de memoria puede fallar, lo que generaría una excepción std::bad_alloc. En este caso, el puntero se perderá y el objeto creado nunca será eliminado.

Una solución segura es crear un unique_ptr, que poseerá el puntero hasta que el vector intente realocar memoria:

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

A partir de C++14, se puede utilizar ‘std::make_unique’:

xxx.push_back(std::make_unique())

Este tipo de defecto no es crítico para LLVM. Si no se puede asignar memoria, el funcionamiento del compilador simplemente se detendrá. Sin embargo, para aplicaciones con largo tiempo de actividad, que no pueden simplemente finalizar si no se ha podido asignar memoria, esto puede ser un verdadero error desagradable.

Por lo tanto, aunque este código no representa un peligro práctico para LLVM, consideré útil hablar sobre este patrón de errores y cómo el analizador PVS-Studio ha aprendido a detectarlo.

Otras advertencias de este tipo:

  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘Passes’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. PassManager.h 546
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘AAs’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. AliasAnalysis.h 324
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘Entries’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. DWARFDebugFrame.cpp 519
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘AllEdges’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. CFGMST.h 268
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘VMaps’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. SimpleLoopUnswitch.cpp 2012
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘Records’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. FDRLogBuilder.h 30
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘PendingSubmodules’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. ModuleMap.cpp 810
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘Objects’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. DebugMap.cpp 88
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘Strategies’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. llvm-isel-fuzzer.cpp 60
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘Modifiers’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. llvm-stress.cpp 685
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘Modifiers’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. llvm-stress.cpp 686
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘Modifiers’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. llvm-stress.cpp 688
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘Modifiers’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. llvm-stress.cpp 689
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘Modifiers’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. llvm-stress.cpp 690
  • V1023 [CWE-460] Un puntero sin propietario se agrega al contenedor ‘Modifiers’ mediante el método ‘emplace_back’. Se producirá una fuga de memoria en caso de una excepción. llvm-stress.cpp 691
  • V1023 [CWE-460] Se añade un puntero sin propietario al contenedor de 'Modifiers' mediante el método 'emplace_back'. Ocurrirá una pérdida de memoria en caso de una excepción. llvm-stress.cpp 692
  • V1023 [CWE-460] Se añade un puntero sin propietario al contenedor de 'Modifiers' mediante el método 'emplace_back'. Ocurrirá una pérdida de memoria en caso de una excepción. llvm-stress.cpp 693
  • V1023 [CWE-460] Se añade un puntero sin propietario al contenedor de 'Modifiers' mediante el método 'emplace_back'. Ocurrirá una pérdida de memoria en caso de una excepción. llvm-stress.cpp 694
  • V1023 [CWE-460] Se añade un puntero sin propietario al contenedor de 'Operands' mediante el método 'emplace_back'. Ocurrirá una pérdida de memoria en caso de una excepción. GlobalISelEmitter.cpp 1911
  • V1023 [CWE-460] Se añade un puntero sin propietario al contenedor de 'Stash' mediante el método 'emplace_back'. Ocurrirá una pérdida de memoria en caso de una excepción. GlobalISelEmitter.cpp 2100
  • V1023 [CWE-460] Se añade un puntero sin propietario al contenedor de 'Matchers' mediante el método 'emplace_back'. Ocurrirá una pérdida de memoria en caso de una excepción. GlobalISelEmitter.cpp 2702

Conclusión

En total, he emitido 60 advertencias, después de lo cual me detuve. ¿Hay otros defectos que detecta el analizador PVS-Studio en LLVM? Sí, los hay. Sin embargo, cuando escribía fragmentos de código para el artículo, ya era tarde en la noche y decidí que era hora de concluir.

Espero que haya sido interesante y que deseen probar el analizador PVS-Studio.

Pueden descargar el analizador y obtener una clave de prueba en esta página.

Lo más importante, utilicen el análisis estático de forma regular. Verificaciones puntuales, realizadas por nosotros con el fin de popularizar la metodología de análisis estático y PVS-Studio, no son un escenario normal.

¡Buena suerte en la mejora de la calidad y fiabilidad del código!

Encontramos errores en LLVM 8 con el analizador PVS-Studio

Si desean compartir este artículo con una audiencia angloparlante, les pido que utilicen el enlace a la traducción: Andrey Karpov. Encontrando errores en LLVM 8 con PVS-Studio.

Fuente: habr.com

Compra un hosting fiable para sitios web con protección contra DDoS, servidores VPS VDS 🔥 Compra un hosting fiable para sitios web con protección contra DDoS, servidores VPS VDS | ProHoster