Geekly articles weekly

Encontrar errores en LLVM 8 con PVS-Studio

PVS-Studio y LLVM 8.0.0

Han pasado dos años desde la última vez que verificamos el código del proyecto LLVM con PVS-Studio, así que veamos si PVS-Studio sigue siendo el líder entre las herramientas para detectar errores y debilidades de seguridad. Lo haremos escaneando la versión LLVM 8.0.0 en busca de nuevos errores.

El artículo que debe ser escrito.


Francamente, no tenía ganas de escribir este artículo. No es muy divertido hablar sobre el proyecto que ya revisamos más de una vez ( 1 , 2 , 3 ). Preferiría algo nuevo, pero no tenía otra opción.

Cada vez que se lanza una nueva versión de LLVM o se actualiza Clang Static Analyzer , recibimos correos electrónicos que leen en estas líneas:

¡Hola, la nueva versión de Clang Static Analyzer tiene nuevos diagnósticos! PVS-Studio parece ser cada vez menos relevante. Clang puede detectar más errores que antes y ahora se está poniendo al día con PVS-Studio. Que dijiste

A eso con mucho gusto respondería:

¡Tampoco hemos estado holgazaneando! Hemos aumentado significativamente las capacidades de PVS-Studio, así que no se preocupe, seguimos siendo los mejores.

Pero esa es una mala respuesta, me temo. No ofrece pruebas, y esa es la razón por la que estoy escribiendo este artículo. Entonces, revisé LLVM una vez más y encontré toneladas de errores de todo tipo. Los que más me gustaron serán discutidos más a fondo. Clang Static Analyzer no puede detectar estos errores (o hace que el proceso sea muy problemático), y nosotros sí. Y, por cierto, solo me llevó una noche escribir todos esos errores.

Sin embargo, el artículo me tomó varias semanas en completar. Simplemente no pude poner el material reunido en texto :).

Por cierto, si se pregunta qué técnicas emplea PVS-Studio para detectar errores y vulnerabilidades, eche un vistazo a esta publicación .

Diagnósticos nuevos y existentes


Como ya dije, la última de las muchas verificaciones de LLVM se realizó hace dos años, y los errores encontrados fueron corregidos por los autores. Este artículo mostrará una nueva porción de errores. ¿Cómo es que hay nuevos errores? Hay tres razones:

  1. El proyecto LLVM está evolucionando; los autores modifican el código existente y agregan código nuevo. Ambas partes modificadas y nuevas tienen naturalmente nuevos errores. Este hecho es un fuerte argumento para ejecutar análisis estáticos regularmente en lugar de de vez en cuando. El formato de nuestros artículos es perfecto para mostrar las capacidades de PVS-Studio, pero no tiene nada que ver con mejorar la calidad del código o hacer que la corrección de errores sea menos costosa. ¡Use análisis estático regularmente!
  2. Modificamos y mejoramos los diagnósticos existentes, permitiendo que el analizador detecte errores que antes no podía detectar.
  3. PVS-Studio se ha mejorado con nuevos diagnósticos, que no existían hace dos años. Agrupe tales advertencias en una sección separada para que el progreso de PVS-Studio se vea más claramente.

Defectos encontrados por diagnósticos existentes


Fragmento no. 1: copiar y 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 .... }

Mensaje de diagnóstico de PVS-Studio: V501 [CWE-570] Hay subexpresiones idénticas 'Name.startswith ("avx512.mask.permvar.")' A la izquierda y a la derecha de '||' operador AutoUpgrade.cpp 73

La aparición del "avx512.mask.permvar". la subcadena se verifica dos veces. La segunda condición obviamente era verificar otra cosa, pero el programador olvidó cambiar la línea copiada.

Fragmento no. 2: 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; .... }

Mensaje de diagnóstico de PVS-Studio: V501 Hay subexpresiones idénticas 'CXNameRange_WantQualifier' a la izquierda y a la derecha de '|' operador CIndex.cpp 7245

La constante nombrada CXNameRange_WantQualifier se usa dos veces debido a un error tipográfico.

Fragmento no. 3: Confusión sobre la precedencia del operador

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

Mensaje de diagnóstico de PVS-Studio: V502 [CWE-783] Quizás el operador '?:' Funciona de una manera diferente a la esperada. El operador '?:' Tiene una prioridad menor que el operador '=='. PPCTargetTransformInfo.cpp 404

Encuentro este error muy lindo. Sí, sé que tengo un sabor extraño :).

Según lo dictado por la precedencia del operador , la expresión original se evalúa de la siguiente manera:

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

Sin embargo, desde el punto de vista práctico, esta condición no tiene sentido, ya que puede reducirse a:

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

Esto obviamente es un error. Debe haber sido la variable de índice que el programador quería comprobar para 0/1. Para corregir el código, el operador ternario debe estar entre paréntesis:

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

El operador ternario es realmente muy complicado y puede conducir a errores lógicos. Úselo con cuidado y no dude en poner paréntesis adicionales a su alrededor. Este tema se trata con más detalle aquí , en la sección "¿Cuidado con el?: Operador y encerrarlo entre paréntesis".

Fragmentos no. 4, 5: puntero 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; } .... }

Mensaje de diagnóstico de PVS-Studio: V522 [CWE-476] Puede producirse una desreferenciación del puntero nulo 'LHS'. TGParser.cpp 2152

Si el puntero LHS es nulo, se espera que el programa genere una advertencia. En cambio, desreferenciará ese puntero muy nulo: LHS-> getAsString () .

Es una situación bastante típica que los controladores de errores contengan errores porque los desarrolladores no los prueban correctamente. Los analizadores estáticos verifican todo el código accesible, sin importar con qué frecuencia se ejecute realmente. Este es un buen ejemplo de cómo el análisis estático complementa otras pruebas de código y medios de protección.

Un manejador defectuoso similar para el puntero RHS se encuentra un poco más lejos: V522 [CWE-476] Puede tener lugar la desreferenciación del puntero nulo 'RHS'. TGParser.cpp 2186

Fragmento no. 6: Usar un puntero después de un movimiento

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

Mensaje de diagnóstico de PVS-Studio: V522 [CWE-476] Puede producirse una desreferenciación del puntero nulo 'ProgClone'. Miscompilation.cpp 601

El puntero inteligente ProgClone primero libera la propiedad del objeto:

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

De hecho, ProgClone se ha convertido en un puntero nulo, por lo que, técnicamente, un puntero nulo se desreferencia un poco más:

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

¡Pero eso no sucederá! Tenga en cuenta que el bucle en realidad no se ejecuta en absoluto.

El contenedor MiscompiledFunctions se borra primero:

 MiscompiledFunctions.clear();

Y luego su tamaño se usa en la condición de bucle:

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

Obviamente, el ciclo simplemente no comenzará. Creo que también es un error, y el código debía verse de alguna manera diferente.

Supongo que lo que vemos aquí es esa notoria paridad de error, donde un error actúa como un disfraz para otro :).

Fragmento no. 7: Usar un puntero después de un movimiento

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

Mensaje de diagnóstico de PVS-Studio: V522 [CWE-476] Puede tener lugar la referencia del puntero nulo 'Prueba'. Miscompilation.cpp 709

Este es similar al caso anterior. El contenido del objeto se mueve primero y luego se usa como si nada hubiera pasado. Este error se ha vuelto cada vez más común después de agregar semántica de movimiento a C ++. ¡Eso es lo que me gusta de este idioma! Te dan nuevas formas de dispararte en el pie, lo que significa que PVS-Studio siempre tendrá trabajo que hacer :).

Fragmento no. 8: puntero 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); }

Mensaje de diagnóstico de PVS-Studio: V522 [CWE-476] Puede producirse una desreferenciación del puntero nulo 'Tipo'. PrettyFunctionDumper.cpp 233

Al igual que los controladores de errores, las funciones de prueba que imprimen datos de depuración tampoco suelen obtener una cobertura de prueba adecuada, y este es un ejemplo de eso. En lugar de ayudar al usuario a resolver sus problemas, la función está esperando que lo solucionen.

Código fijo:

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

Fragmento no. 9: puntero 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()); .... }

Mensaje de diagnóstico de PVS-Studio: V522 [CWE-476] Puede producirse una desreferenciación del puntero nulo 'Ty'. SearchableTableEmitter.cpp 614

No creo que necesites ningún comentario sobre este.

Fragmento no. 10: 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; }

Mensaje de diagnóstico de PVS-Studio: V570 La variable 'Identificador-> Tipo' se asigna a sí misma. FormatTokenLexer.cpp 249

Asignar una variable a sí mismo es una operación sin sentido. El programador debe haber querido hacer lo siguiente:

 Identifier->Type = Question->Type;

Fragmento no. 11: descanso 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; .... }

Mensaje de diagnóstico de PVS-Studio: V622 [CWE-478] Considere la posibilidad de inspeccionar la declaración 'switch'. Es posible que falte el primer operador de 'caso'. SystemZAsmParser.cpp 652

Hay una declaración de ruptura muy sospechosa al principio. ¿No debería haber algo más aquí?

Fragmento no. 12: Verificación de un puntero después 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"); .... }

Mensaje de diagnóstico de PVS-Studio: V595 [CWE-476] El puntero 'Callee' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 172, 174. AMDGPUInline.cpp 172

El puntero de Callee se desreferencia primero cuando se llama a la función getTTI .

Y luego resulta que el puntero debe verificarse para nullptr :

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

Demasiado tarde ...

Fragmentos no. 13 - No ...: comprobar un puntero después de desreferenciar

El ejemplo anterior no es único. El mismo problema se encuentra en este fragmento:

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

Mensaje de diagnóstico de PVS-Studio: V595 [CWE-476] El puntero 'CalleeFn' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 1079, 1081. SimplifyLibCalls.cpp 1079

Y este:

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

Mensaje de diagnóstico de PVS-Studio: V595 [CWE-476] El puntero 'ND' se utilizó antes de verificarlo con nullptr. Líneas de verificación: 532, 534. SemaTemplateInstantiateDecl.cpp 532

Y aqui:

  • V595 [CWE-476] El puntero 'U' se utilizó antes de que se verificara contra nullptr. Verifique las líneas: 404, 407. DWARFFormValue.cpp 404
  • V595 [CWE-476] El puntero 'ND' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 2149, 2151. SemaTemplateInstantiate.cpp 2149

Luego perdí interés en rastrear las advertencias V595, por lo que no puedo decirte si hay otros errores de este tipo además de los que se muestran arriba. Apuesto a que hay.

Fragmentos no. 17, 18: cambio sospechoso

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

Mensaje de diagnóstico de PVS-Studio: V629 [CWE-190] Considere inspeccionar la expresión '~ (Tamaño - 1) << 1'. Desplazamiento de bits del valor de 32 bits con una expansión posterior al tipo de 64 bits. AArch64AddressingModes.h 260

Este código podría ser correcto, pero parece extraño y necesita ser examinado.

Suponga que la variable Tamaño tiene el valor 16; entonces se espera que la variable NImms obtenga el siguiente valor:

11111111111111111111111111111111111111111111111111111111111111100000000

Pero en realidad obtendrá el valor:

0000000000000000000000000000000000001111111111111111111111111111100000

Esto sucede porque todos los cálculos se realizan en el tipo sin signo de 32 bits, y solo entonces se promociona implícitamente a uint64_t , con los bits más significativos puestos a cero.

El problema se puede solucionar de la siguiente manera:

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

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

Fragmento no. 19: ¿Falta otra palabra clave ?

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

Mensaje de diagnóstico de 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

Este no es un error. Dado que el bloque de la primera instrucción if termina con continue , no importa si tiene la palabra clave else o no. El comportamiento será el mismo en cualquier caso. Sin embargo, lo que falta hace que el código sea menos legible y, por lo tanto, potencialmente peligroso. Si continuar desaparece un día, el comportamiento cambiará drásticamente. Recomiendo agregar más .

Fragmento no. 20: 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; }

Mensajes de diagnóstico de PVS-Studio:

  • V655 [CWE-480] Las cadenas se concatenaron pero no se utilizan. Considere inspeccionar la expresión 'Result + Name.str ()'. Symbol.cpp 32
  • V655 [CWE-480] Las cadenas se concatenaron pero no se utilizan. Considere inspeccionar la expresión 'Resultado + "(Clase ObjC)" + Nombre.str ()'. Symbol.cpp 35
  • V655 [CWE-480] Las cadenas se concatenaron pero no se utilizan. Considere inspeccionar la expresión 'Resultado + "(ObjC Clase EH)" + Nombre.str ()'. Symbol.cpp 38
  • V655 [CWE-480] Las cadenas se concatenaron pero no se utilizan. Considere inspeccionar la expresión 'Resultado + "(ObjC IVar)" + Nombre.str ()'. Symbol.cpp 41

El programador usó accidentalmente el operador + en lugar de + = y terminó con cuatro construcciones sin sentido.

Fragmento no. 21: 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 cannot be empty"); for (auto &Op : Ops) { assert(!Op.empty() && "Empty operator"); if (FeaturesMap.find(Op) == FeaturesMap.end()) FeaturesMap[Op] = FeaturesMap.size(); } } }

Intenta detectar el error por tu cuenta primero. Agregué la imagen para que no vea la respuesta de inmediato:

???


Mensaje de diagnóstico de PVS-Studio: V708 [CWE-758] Se utiliza una construcción peligrosa: 'FeaturesMap [Op] = FeaturesMap.size ()', donde 'FeaturesMap' es de la clase 'map'. Esto puede conducir a un comportamiento indefinido. RISCVCompressInstEmitter.cpp 490

La línea defectuosa es esta:

 FeaturesMap[Op] = FeaturesMap.size();

Si no se ha encontrado el elemento Op , el programa crea un nuevo elemento en el mapa y le asigna el número total de elementos en este mapa. Simplemente no sabe si se llamará a la función de tamaño antes o después de agregar el nuevo elemento.

Fragmentos no. 22 - No. 24: tareas duplicadas

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

Mensaje de diagnóstico de PVS-Studio: V519 [CWE-563] A la variable 'NType' se le asignan valores dos veces sucesivamente. Quizás esto sea un error. Líneas de verificación: 1663, 1664. MachOObjectFile.cpp 1664

No creo que sea un verdadero error, sino una tarea duplicada. Pero sigue siendo un defecto.

Otros dos casos:

  • V519 [CWE-563] A la variable 'B.NDesc' se le asignan valores dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 1488, 1489. llvm-nm.cpp 1489
  • V519 [CWE-563] A la variable se le asignan valores dos veces seguidas. Quizás esto sea un error. Verifique las líneas: 59, 61. coff2yaml.cpp 61

Fragmentos no. 25 - No. 27: Más tareas duplicadas

Estos tratan con versiones ligeramente diferentes de tareas duplicadas.

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

Mensaje de diagnóstico de PVS-Studio: V519 [CWE-563] A la variable 'Alineación' se le asignan valores dos veces sucesivamente. Quizás esto sea un error. Líneas de verificación: 1158, 1160. LoadStoreVectorizer.cpp 1160

Este es un fragmento muy extraño, y probablemente contiene un error lógico. Primero se le asigna el valor a la variable Alineamiento en función de la condición, y luego se le asigna el valor una vez más, pero sin ninguna verificación previa.

Defectos similares:

  • V519 [CWE-563] A la variable 'Efectos' se le asignan valores dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 152, 165. WebAssemblyRegStackify.cpp 165
  • V519 [CWE-563] La variable 'ExpectNoDerefChunk' recibe valores asignados dos veces sucesivamente. Quizás esto sea un error. Líneas de verificación: 4970, 4973. SemaType.cpp 4973

Fragmento no. 28: 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) // PAUSE instruction support // <= break; } .... }

Mensaje de diagnóstico de PVS-Studio: V547 [CWE-571] La expresión 'nextByte! = 0x90' siempre es verdadera. X86DisassemblerDecoder.cpp 379

El cheque no tiene sentido. La variable nextByte nunca es igual a 0x90 : solo se deduce lógicamente de la verificación anterior. Esto debe ser un error lógico.

Fragmentos no. 29 - No ....: Siempre condiciones verdaderas / falsas

Hay muchas advertencias sobre una condición completa ( V547 ) o parte de una condición ( V560 ) que siempre es verdadera o falsa. En lugar de errores genuinos, estos a menudo son simplemente códigos incorrectos, los efectos de la expansión de macros, etc. Dicho esto, todas estas advertencias aún deben verificarse porque algunas de ellas pueden señalar errores lógicos genuinos. Por ejemplo, el siguiente fragmento no se ve bien:

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

Mensaje de diagnóstico de 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 número decimal 14. La comprobación RegNo == 0xe no tiene sentido porque si RegNo> 13 , la función volverá.

Vi muchas otras advertencias de V547 y V560, pero, al igual que con V595 , no me sentí entusiasmado por revisarlas ya que ya tenía suficiente material para un artículo :). Por lo tanto, no hay cifras para el número total de errores de este tipo en LLVM.

Aquí hay un ejemplo para ilustrar por qué es aburrido verificar esas advertencias. El analizador es totalmente correcto al emitir una advertencia sobre el siguiente código. Pero todavía no es un error.

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

Mensaje de diagnóstico de PVS-Studio: V547 [CWE-570] La expresión '! HasError' siempre es falsa. UnwrappedLineParser.cpp 1635

Fragmento no. 30: regreso 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(); } .... }

Mensaje de diagnóstico de PVS-Studio: V612 [CWE-670] Un 'retorno' incondicional dentro de un bucle. R600OptimizeVectorRegisters.cpp 63

Es un error o una técnica de codificación específica destinada a comunicar alguna idea a otros programadores. Para mí no dice nada, excepto que es un código muy sospechoso. Por favor no escriba código como ese :).

¿Te sientes cansado? Bien, es hora de hacer té o café.

cafe


Defectos encontrados por nuevos diagnósticos


Creo que 30 ejemplos son suficientes para los diagnósticos existentes. Ahora veamos si podemos encontrar algo interesante con los nuevos diagnósticos, que se agregaron después de la verificación anterior . En los últimos dos años, el módulo analizador C ++ se amplió con 66 nuevos diagnósticos.

Fragmento no. 31: 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(); }

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

Como puede ver, ambas ramas de la declaración if terminan con una declaración return , lo que significa que el contenedor CtorDtorsByPriority nunca se borrará.

Fragmento no. 32: 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; }

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

Este es interesante. Echa un vistazo a esta parte primero:

 return ParseTypeIdEntry(SummaryID); break;

Parece que no hay nada extraño en este código; la declaración de ruptura es innecesaria y se puede eliminar de forma segura. Pero no es tan simple.

La advertencia se activa mediante las siguientes líneas:

 Lex.setIgnoreColonInIdentifiers(false); return false;

De hecho, este código es inalcanzable. ¡Todas las etiquetas de caso de la declaración de cambio finalizan con un retorno , y la ruptura solitaria sin sentido ya no parece tan inofensiva! ¿Qué pasa si una de las ramas estaba destinada a terminar con un descanso en lugar de regresar ?

Fragmento no. 33: borrado accidental de los bits más significativos

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

Mensaje de diagnóstico de 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 más altos. RuntimeDyld.cpp 815

Tenga en cuenta que la función getStubAlignment devuelve un valor sin signo . Veamos cómo se evaluará la expresión, suponiendo que la función devolverá el valor 8:

~ (getStubAlignment () - 1)

~ (8u-1)

0xFFFFFFF8u

Tenga en cuenta ahora que el tipo de la variable DataSize es de 64 bits sin signo. Entonces resulta que ejecutar la operación DataSize & 0xFFFFFFF8 dará como resultado la eliminación de los 32 bits más significativos del valor. No creo que el programador quisiera eso. Quizás quisieron decir que era DataSize y 0xFFFFFFFFFFFFFFFFF8u.

Para corregir el error, el código debe reescribirse así:

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

O así:

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

Fragmento no. 34: Mala conversión de tipo explícito

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

Mensaje de diagnóstico de 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 explícita de tipos se utiliza para evitar un desbordamiento al multiplicar variables de tipo int . En este caso, sin embargo, no funciona porque la multiplicación ocurrirá primero y solo entonces el resultado de 32 bits se promocionará a tipo size_t .

Fragmento no. 35: Mal copiar-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] Se encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y se debe usar la variable 'Op1' en lugar de 'Op0'. InstCombineCompares.cpp 5507

Este nuevo diagnóstico fresco detecta situaciones en las que se escribe un fragmento de código usando copiar y pegar, con todos los nombres cambiados, excepto uno.

Tenga en cuenta que todos los Op0 excepto uno se cambiaron a Op1 en el segundo bloque. El código probablemente debería verse así:

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

Fragmento no. 36: Variables mezcladas

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

Mensaje de diagnóstico de PVS-Studio: V1001 [CWE-563] La variable 'Modo' se asigna pero no se usa al final de la función. SIModeRegister.cpp 48

Es muy peligroso tener los mismos nombres para los argumentos de la función que para los miembros de la clase porque corre el riesgo de mezclarlos. Lo que ves aquí es un ejemplo de eso. La siguiente expresión no tiene sentido:

 Mode &= Mask;

El argumento cambia pero nunca se usa después de eso. Este fragmento probablemente debería verse así:

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

Fragmento no. 37: Variables mezcladas

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

Mensaje de diagnóstico de PVS-Studio: V1001 [CWE-563] La variable 'Tamaño' se asigna pero no se usa al final de la función. Object.cpp 424

Este es similar al ejemplo anterior. Versión correcta:

 this->Size += this->EntrySize;

Fragmentos no. 38 - No. 47: Falta la comprobación del puntero

Observamos algunos ejemplos de la advertencia V595 un poco antes. Lo que detecta es una situación en la que primero se desreferencia un puntero y solo luego se verifica. El nuevo diagnóstico V1004 es lo opuesto a eso, y también detecta toneladas de errores. Busca punteros ya probados que no se vuelven a probar cuando es necesario. Aquí hay algunos errores de este tipo que se encuentran en el código 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()); // <= .... }

Mensaje de diagnóstico de PVS-Studio: V1004 [CWE-476] El puntero 'Ptr' se usó de manera insegura después de que se verificó contra nullptr. Líneas de verificación: 729, 738. TargetTransformInfoImpl.h 738

Ptr puede ser nullptr , lo que se indica mediante la comprobación:

 if (Ptr != nullptr)

Sin embargo, el mismo puntero se desreferencia sin tal verificación un poco más:

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

Otro caso similar.

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

Mensaje de diagnóstico de PVS-Studio: V1004 [CWE-476] El puntero 'FD' se usó de manera insegura después de que se verificó contra nullptr. Líneas de verificación: 3228, 3231. CGDebugInfo.cpp 3231

Tenga en cuenta el puntero FD . Este error es sencillo, por lo que no hay comentarios sobre este.

Uno más aquí:

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

Mensaje de diagnóstico de PVS-Studio: V1004 [CWE-476] El puntero 'PtrTy' se usó de manera insegura después de que se verificó contra nullptr. Líneas de verificación: 960, 965. InterleavedLoadCombinePass.cpp 965

¿Cómo evitas errores como ese? Tenga mucho cuidado al revisar su código y verifíquelo regularmente con PVS-Studio.

No creo que debamos examinar otros ejemplos de este tipo, así que aquí hay solo una lista de las advertencias:
  • V1004 [CWE-476] El puntero 'Expr' se usó de forma insegura después de que se verificó contra nullptr. Líneas de verificación: 1049, 1078. DebugInfoMetadata.cpp 1078
  • V1004 [CWE-476] El puntero 'PI' se usó de forma insegura después de que se verificó contra nullptr. Líneas de verificación: 733, 753. LegacyPassManager.cpp 753
  • V1004 [CWE-476] El puntero 'StatepointCall' se usó de manera insegura después de que se verificó contra nullptr. Líneas de verificación: 4371, 4379. Verifier.cpp 4379
  • V1004 [CWE-476] El puntero 'RV' se usó de forma insegura después de que se verificó contra nullptr. Líneas de verificación: 2263, 2268. TGParser.cpp 2268
  • V1004 [CWE-476] El puntero 'CalleeFn' se usó de forma insegura después de que se verificó contra nullptr. Líneas de verificación: 1081, 1096. SimplifyLibCalls.cpp 1096
  • V1004 [CWE-476] El puntero 'TC' se usó de forma insegura después de que se verificó contra nullptr. Líneas de verificación: 1819, 1824. Driver.cpp 1824

Fragmentos no. 48 - No. 60: No es crítico pero sigue siendo un defecto (pérdida potencial de memoria)

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

Mensaje de diagnóstico de PVS-Studio: V1023 [CWE-460] El método 'emplace_back' agrega un puntero sin propietario al contenedor 'Estrategias'. Se producirá una pérdida de memoria en caso de una excepción. llvm-isel-fuzzer.cpp 58

No puede simplemente escribir xxx.push_back (nueva X) para agregar un elemento a un contenedor de tipo std :: vector <std :: unique_ptr <X>> porque no hay conversión implícita de X * a std :: unique_ptr < X> .

La solución popular es escribir xxx.emplace_back (nueva X) ya que es compilable: el método emplace_back construye el elemento directamente a partir de los argumentos y, por lo tanto, puede usar constructores explícitos.

Pero esa solución no es segura. Si el vector está lleno, la memoria se reasignará. Esta operación puede fallar y terminar generando una excepción std :: bad_alloc . En este caso, el puntero se perderá y el programa no podrá eliminar el objeto creado.

Una solución más segura es crear un unique_ptr , que retendrá el puntero hasta que el vector intente reasignar la memoria:

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

El estándar C ++ 14 le permite usar 'std :: make_unique':

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

Este tipo de defecto no tiene efecto en LLVM. La compilación simplemente terminará si falla la asignación de memoria. Dicho esto, puede ser bastante crítico en aplicaciones con un tiempo de actividad prolongado , que no puede terminar simplemente cuando ocurre una falla de asignación de memoria.

Entonces, a pesar de que este código no es peligroso para LLVM, pensé que aún debería informarle sobre este patrón de error y el hecho de que PVS-Studio ahora puede detectarlo.

Otros casos similares:

  • V1023 [CWE-460] El método 'emplace_back' agrega un puntero sin propietario al contenedor 'Passes'. Se producirá una pérdida de memoria en caso de una excepción. PassManager.h 546
  • V1023 [CWE-460] A pointer without owner is added to the 'AAs' container by the 'emplace_back' method. A memory leak will occur in case of an exception. AliasAnalysis.h 324
  • V1023 [CWE-460] A pointer without owner is added to the 'Entries' container by the 'emplace_back' method. A memory leak will occur in case of an exception. DWARFDebugFrame.cpp 519
  • V1023 [CWE-460] A pointer without owner is added to the 'AllEdges' container by the 'emplace_back' method. A memory leak will occur in case of an exception. CFGMST.h 268
  • V1023 [CWE-460] A pointer without owner is added to the 'VMaps' container by the 'emplace_back' method. A memory leak will occur in case of an exception. SimpleLoopUnswitch.cpp 2012
  • V1023 [CWE-460] A pointer without owner is added to the 'Records' container by the 'emplace_back' method. A memory leak will occur in case of an exception. FDRLogBuilder.h 30
  • V1023 [CWE-460] A pointer without owner is added to the 'PendingSubmodules' container by the 'emplace_back' method. A memory leak will occur in case of an exception. ModuleMap.cpp 810
  • V1023 [CWE-460] A pointer without owner is added to the 'Objects' container by the 'emplace_back' method. A memory leak will occur in case of an exception. DebugMap.cpp 88
  • V1023 [CWE-460] A pointer without owner is added to the 'Strategies' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-isel-fuzzer.cpp 60
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 685
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 686
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 688
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 689
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 690
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 691
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 692
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 693
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 694
  • V1023 [CWE-460] A pointer without owner is added to the 'Operands' container by the 'emplace_back' method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 1911
  • V1023 [CWE-460] A pointer without owner is added to the 'Stash' container by the 'emplace_back' method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 2100
  • V1023 [CWE-460] A pointer without owner is added to the 'Matchers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 2702

Conclusión


Escribí 60 advertencias y me detuve en eso. ¿PVS-Studio encontró otros errores en LLVM? Sí lo hizo. Pero mientras escribía los ejemplos, cayó la noche, así que decidí dejarlo.

Espero que hayas disfrutado leyendo este artículo y te haya animado a probar el analizador PVS-Studio por ti mismo.

Visite esta página para descargar el analizador y obtener una clave de prueba.

Lo más importante, utilice análisis estático regularmente. Los controles únicos , como los que hacemos para popularizar el análisis estático y promover PVS-Studio, no son el escenario normal.

¡Buena suerte con la mejora de la calidad y fiabilidad de su código!

Source: https://habr.com/ru/post/450002/

More articles:


All Articles