Mientras se realizaba la 118a Semana Nobel en Estocolmo, se estaba preparando una revisión del código del proyecto ROOT utilizado en la investigación científica para procesar grandes datos en la oficina de desarrollo del analizador de código estático PVS-Studio. Por supuesto, no otorgará una bonificación por dicho código, pero los desarrolladores recibirán una revisión detallada de defectos de código interesantes y una licencia para una verificación completa del proyecto.
Introduccion
ROOT : un conjunto de utilidades para trabajar con datos de investigación científica. Proporciona toda la funcionalidad necesaria para el procesamiento de big data, análisis estadístico, visualización y almacenamiento. Está escrito principalmente en C ++. El desarrollo comenzó en el
CERN (Organización Europea para la Investigación Nuclear) para la investigación en física de alta energía. Todos los días, miles de físicos utilizan aplicaciones ROOT para analizar sus datos o simular.
PVS-Studio es una herramienta para detectar errores y vulnerabilidades potenciales en el código fuente de programas escritos en C, C ++, C # y Java. Funciona en sistemas de 64 bits en Windows, Linux y macOS y puede analizar código diseñado para plataformas ARM integradas y de 32 bits.
Nuevo debut de diagnóstico
V1046 Uso inseguro de los tipos bool 'e' int 'juntos en la operación' & = '. GSLMultiRootFinder.h 175
int AddFunction(const ROOT::Math::IMultiGenFunction & func) { ROOT::Math::IMultiGenFunction * f = func.Clone(); if (!f) return 0; fFunctions.push_back(f); return fFunctions.size(); } template<class FuncIterator> bool SetFunctionList( FuncIterator begin, FuncIterator end) { bool ret = true; for (FuncIterator itr = begin; itr != end; ++itr) { const ROOT::Math::IMultiGenFunction * f = *itr; ret &= AddFunction(*f); } return ret; }
La versión beta del analizador que se utilizó durante la verificación encontró un error tan terrible.
Esperando La función
SetFunctionList omite la lista de iteradores. Si al menos uno de ellos no es válido, el valor de retorno será
falso , de lo contrario será
verdadero .
Realidad La función
SetFunctionList puede devolver
false incluso para iteradores válidos.
Echaremos un
vistazo a la situación:
AddFunction devuelve el número de iteradores válidos en la lista de
fFunctions . Es decir al agregar iteradores distintos de cero, el tamaño de esta lista aumentará secuencialmente: 1, 2, 3, 4, etc. Aquí es donde el error en el código comienza a manifestarse:
ret &= AddFunction(*f);
Porque Como la función devuelve un resultado de tipo
int , no
bool , la operación '& =' con números pares dará el valor
falso . Después de todo, el bit menos significativo de los números pares siempre será cero. Por lo tanto, un error tan obvio estropeará el resultado de la función
SetFunctionsList incluso para argumentos válidos.
Errores en expresiones condicionales
V501 Hay
subexpresiones idénticas a la izquierda y a la derecha del operador '&&': módulo && module rootcling_impl.cxx 3650
virtual void HandleDiagnostic(....) override { .... bool isROOTSystemModuleDiag = module && ....; bool isSystemModuleDiag = module && module && module->IsSystem; if (!isROOTSystemModuleDiag && !isSystemModuleDiag) fChild->HandleDiagnostic(DiagLevel, Info); .... }
Comencemos con el ejemplo más inofensivo. El puntero del módulo se verifica dos veces. Lo más probable es que un cheque sea innecesario. Pero es mejor arreglar el código para que no haya preguntas innecesarias.
V501 Hay
subexpresiones idénticas 'strchr (fHostAuth-> GetHost (),' * ')' a la izquierda y a la derecha de '||' operador TAuthenticate.cxx 300
TAuthenticate::TAuthenticate(TSocket *sock, const char *remote, const char *proto, const char *user) { ....
Aquí en la línea
fHostAuth-> GetHost () se busca el mismo carácter - '*'. Quizás uno de ellos debería ser el símbolo '?'. Por lo general, se utilizan para establecer diferentes máscaras.
V517 El uso del
patrón 'if (A) {...} else if (A) {...}' fue detectado. Hay una probabilidad de presencia de error lógico. Líneas de verificación: 163, 165. TProofMonSenderML.cxx 163
Int_t TProofMonSenderML::SendSummary(TList *recs, const char *id) { .... if (fSummaryVrs == 0) { if ((dsn = recs->FindObject("dataset"))) recs->Remove(dsn); } else if (fSummaryVrs == 0) {
La variable
fSummaryVrs se compara dos veces a cero. Esto hace que el código en la rama else-if nunca se ejecute. Allí se escribe mucho código ...
V523 La declaración 'then' es equivalente a la declaración 'else'. TKDTree.cxx 805
template <typename Index, typename Value> void TKDTree<Index, Value>::UpdateRange(....) { .... if (point[fAxis[inode]]<=fValue[inode]){
El mismo código de copiar y pegar se ejecuta bajo cualquier condición. Quizás haya un error tipográfico en las palabras
izquierda y
derecha .
Hay muchos códigos sospechosos similares en el proyecto:
- V523 La declaración 'then' es equivalente a la declaración 'else'. TContainerConverters.cxx 51
- V523 La declaración 'then' es equivalente a la declaración 'else'. TWebFile.cxx 1310
- V523 La declaración 'then' es equivalente a la declaración 'else'. Método MLP.cxx 423
- V523 La declaración 'then' es equivalente a la declaración 'else'. RooAbsCategory.cxx 394
La expresión
V547 '! File_name_value.empty ()' siempre es falsa. SelectionRules.cxx 1423
bool SelectionRules::AreAllSelectionRulesUsed() const { for(auto&& rule : fClassSelectionRules){ .... std::string file_name_value; if (!rule.GetAttributeValue("file_name", file_name_value)) file_name_value.clear(); if (!file_name_value.empty()) {
Lo más probable es que no haya error. El analizador ha detectado código que puede acortarse. Porque
Dado que el valor de
file_name_value.empty () se verifica al comienzo del ciclo, luego, en la parte inferior del código, esta verificación se puede eliminar, lo que reduce significativamente la cantidad de código innecesario.
V590 Considere inspeccionar el '! File1 || c <= 0 || c == '*' || c! = '(' 'expresión. La expresión es excesiva o contiene un error de imprenta. TTabCom.cxx 840
TString TTabCom::DetermineClass(const char varName[]) { .... c = file1.get(); if (!file1 || c <= 0 || c == '*' || c != '(') { Error("TTabCom::DetermineClass", "variable \"%s\" not defined?", varName); goto cleanup; } .... }
Considere la parte abreviada de la expresión condicional señalada por el analizador:
if (.... || c == '*' || c != '(') { .... }
La condición no depende de si el símbolo "asterisco" es igual o no. Esta parte de la expresión condicional siempre será verdadera para cualquier carácter que no sea '('. Esto es fácil de ver si construye una tabla de verdad.
Dos lugares más con lógica extraña en expresiones condicionales:
- V590 Considere inspeccionar esta expresión. La expresión es excesiva o contiene un error de imprenta. TFile.cxx 3963
- V590 Considere inspeccionar esta expresión. La expresión es excesiva o contiene un error de imprenta. TStreamerInfoActions.cxx 3084
V593 Considere revisar la expresión del tipo 'A = B <C'. La expresión se calcula de la siguiente manera: 'A = (B <C)'. TProofServ.cxx 1903
Int_t TProofServ::HandleSocketInput(TMessage *mess, Bool_t all) { .... if (Int_t ret = fProof->AddWorkers(workerList) < 0) { Error("HandleSocketInput:kPROOF_GETSLAVEINFO", "adding a list of worker nodes returned: %d", ret); } .... }
El error detectado por el analizador solo se manifiesta cuando el programa no funciona correctamente. La variable
ret debe almacenar el código de retorno de la función
AddWorkers y, en caso de emergencia, mostrar este valor en el registro. Pero el código no funciona así. La condición carece de corchetes adicionales que especifiquen la prioridad de las operaciones. No es el código de retorno que se almacena en la variable
ret , sino el resultado de una comparación lógica, es decir. solo 0 o 1.
Hay otro lugar con un defecto similar:
- V593 Considere revisar la expresión del tipo 'A = B <C'. La expresión se calcula de la siguiente manera: 'A = (B <C)'. TProofServ.cxx 3897
V768 La constante de enumeración 'kCostComplexityPruning' se usa como una variable de tipo booleano. MethodDT.cxx 283
enum EPruneMethod {kExpectedErrorPruning=0, kCostComplexityPruning, kNoPruning}; void TMVA::MethodDT::ProcessOptions() { .... if (fPruneStrength < 0) fAutomatic = kTRUE; else fAutomatic = kFALSE; if (fAutomatic && fPruneMethod==!DecisionTree::kCostComplexityPruning){ Log() << kFATAL << "Sorry automatic pruning strength determination is ...." << Endl; } .... }
Hmm ... ¿Por qué la negación del valor constante de
kCostComplexityPruning ? Lo más probable es que el símbolo de negación se haya agregado accidentalmente y ahora conduce a una lógica incorrecta de ejecución del código.
Código inválido con punteros
V522 Puede tener lugar la desreferenciación del puntero nulo 'pre'. TSynapse.cxx 61
void TSynapse::SetPre(TNeuron * pre) { if (pre) { Error("SetPre","this synapse is already assigned to a pre-neuron."); return; } fpre = pre; pre->AddPost(this); }
Traté de descubrir este código extraño. Parece que la idea no es establecer un nuevo valor para el campo
fpre . Luego cometieron un error al confundir el puntero, que debe verificarse en la condición. En la implementación actual, si pasa el valor
nullptr a la función
SetPre , el puntero nulo será desreferenciado.
Lo más probable es que esto sea correcto:
void TSynapse::SetPre(TNeuron * pre) { if (fpre) { Error("SetPre","this synapse is already assigned to a pre-neuron."); return; } fpre = pre; pre->AddPost(this); }
Es cierto que esto aún no salvará la función de pasar un puntero nulo. Pero al menos este código parece más lógico que la versión original.
Aquí hay otro lugar que se ha copiado desde aquí con ligeras modificaciones:
- V522 Puede tener lugar la desreferenciación del puntero nulo 'post'. TSynapse.cxx 74
V595 El puntero 'N' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 484, 488. Scanner.cxx 484
bool RScanner::shouldVisitDecl(clang::NamedDecl *D) { if (auto M = D->getOwningModule()) {
¡El analizador detectó un código muy peligroso! En el primer caso, el puntero
N está desreferenciado sin verificar un valor cero. Y ni siquiera puede ver la llamada al puntero no verificado. Esto sucede dentro de la función
shouldVisitDecl .
Tradicionalmente, esta regla de diagnóstico genera muchas advertencias útiles. Aquí hay algunos de ellos:
- V595 El puntero 'archivo' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 141, 153. TFileCacheRead.cxx 141
- V595 El puntero 'fFree' se utilizó antes de verificarlo con nullptr. Líneas de verificación: 2029, 2038. TFile.cxx 2029
- V595 El puntero 'tbuf' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 586, 591. TGText.cxx 586
- V595 El puntero 'fPlayer' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 3425, 3430. TProof.cxx 3425
- V595 El puntero 'gProofServ' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 1192, 1194. TProofPlayer.cxx 1192
- V595 El puntero 'projDataTmp' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 791, 804. RooSim simultánea.cxx 791
El siguiente ejemplo no es un error, pero una vez más demuestra que las macros
fomentan la escritura de código incorrecto o redundante.
V571 Verificación recurrente. La condición 'if (fCanvasImp)' ya se verificó en la línea 799. TCanvas.cxx 800
#define SafeDelete(p) { if (p) { delete p; p = 0; } } void TCanvas::Close(Option_t *option) { .... if (fCanvasImp) SafeDelete(fCanvasImp); .... }
El puntero
fCanvasImp se verifica dos veces. Una de las comprobaciones ya está implementada en la macro
SafeDelete . Uno de los problemas con las macros es que a menudo son difíciles de navegar desde el código, por lo que muchos no estudian sus contenidos antes de usarlos.
Errores al trabajar con matrices
V519 A la
variable 'Línea [Cursor]' se le asignan valores dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 352, 353. Editor.cpp 353
size_t find_last_non_alnum(const std::string &str, std::string::size_type index = std::string::npos) { .... char tmp = Line.GetText()[Cursor]; Line[Cursor] = Line[Cursor - 1]; Line[Cursor] = tmp; .... }
El nuevo valor del elemento
Line [Cursor] se sobrescribe inmediatamente. Algo está mal aquí ...
V557 Array overrun es posible. El índice 'ivar' apunta más allá del límite de la matriz. BasicMinimizer.cxx 130
bool BasicMinimizer::SetVariableValue(unsigned int ivar, double val) { if (ivar > fValues.size() ) return false; fValues[ivar] = val; return true; }
Entonces, cometer errores al verificar el índice de una matriz es un problema enorme últimamente. Casi cada tercer proyecto se encuentra. Si todo es simple al indexar una matriz en un bucle: el operador '<' casi siempre se usa para comparar el índice con el tamaño de la matriz, entonces con tal verificación, como aquí, debe usar el operador '> =', y no '>'. De lo contrario, es posible ir más allá del límite de la matriz por un elemento.
Este error se propagó varias veces sobre el archivo:
- V557 Array overrun es posible. El índice 'ivar' apunta más allá del límite de la matriz. BasicMinimizer.cxx 186
- V557 Array overrun es posible. El índice 'ivar' apunta más allá del límite de la matriz. BasicMinimizer.cxx 194
- V557 Array overrun es posible. El índice 'ivar' apunta más allá del límite de la matriz. BasicMinimizer.cxx 209
- V557 Array overrun es posible. El índice 'ivar' apunta más allá del límite de la matriz. BasicMinimizer.cxx 215
- V557 Array overrun es posible. El índice 'ivar' apunta más allá del límite de la matriz. BasicMinimizer.cxx 230
V621 Considere inspeccionar el operador 'para'. Es posible que el bucle se ejecute incorrectamente o no se ejecute en absoluto. TDataMember.cxx 554
Int_t TDataMember::GetArrayDim() const { if (fArrayDim<0 && fInfo) { R__LOCKGUARD(gInterpreterMutex); TDataMember *dm = const_cast<TDataMember*>(this); dm->fArrayDim = gCling->DataMemberInfo_ArrayDim(fInfo);
Lo más probable es que en el bucle for quisieran comparar la variable
dim con
dm-> fArrayDim , y no con
fArrayDim . El valor de la variable utilizada es negativo, debido a la condición al comienzo de la función. Tal ciclo nunca se ejecuta.
V767 Acceso sospechoso al elemento de la matriz 'actual' por un índice constante dentro de un bucle. TClingUtils.cxx 3082
llvm::StringRef ROOT::TMetaUtils::DataMemberInfo__ValidArrayIndex(....) { .... while (current!=0) {
Este fragmento de código analiza una determinada línea y verifica su corrección. Después de que el carácter nulo de la
corriente de cadena se define como un número, todos los demás caracteres se atraviesan para asegurarse de que todos los caracteres sean números hasta el final de la cadena. Bueno, cómo se hace ... el contador de bucles no se usa en el bucle. Era necesario escribir
actual [i] , y no
actual [0] en la condición.
Pérdida de memoria
V773 Se salió de la función sin soltar el puntero 'lista de opciones'. Una pérdida de memoria es posible. TDataMember.cxx 355
void TDataMember::Init(bool afterReading) { .... TList *optionlist = new TList();
Al salir de una función, el puntero
optionList no proporciona memoria. Si esto es necesario en este caso particular es difícil para mí decirlo. Pero por lo general, tales errores se corrigen en proyectos en informes PVS-Studio. Todo depende de si el programa debe intentar continuar trabajando en caso de emergencia o no. Hay muchas advertencias de este tipo en el proyecto, es mejor que los desarrolladores verifiquen el proyecto por su cuenta y vean el informe completo del analizador.
De nuevo sobre la función memset
V597 El compilador podría eliminar la llamada de función 'memset', que se utiliza para vaciar el búfer 'x'. La función memset_s () debe usarse para borrar los datos privados. TMD5.cxx 366
void TMD5::Transform(UInt_t buf[4], const UChar_t in[64]) { UInt_t a, b, c, d, x[16]; ....
Muchos pensarán que cuando se compila el código, este comentario no entrará en el archivo binario, y tendrán razón: D. Pero la función
memset también será eliminada por el compilador, no todos lo adivinan. Y sucederá. Si el búfer vaciado ya no se utilizará en el código, el compilador optimizará el código y eliminará la llamada a la función. Técnicamente, tiene razón, pero si había datos secretos en el búfer, entonces permanecerán allí. Este es un clásico error de seguridad
CWE-14 .
Advertencias varias
V591 La función no nula debería devolver un valor. LogLikelihoodFCN.h 108
LogLikelihoodFCN & operator = (const LogLikelihoodFCN & rhs) { SetData(rhs.DataPtr() ); SetModelFunction(rhs.ModelFunctionPtr() ); fNEffPoints = rhs.fNEffPoints; fGrad = rhs.fGrad; fIsExtended = rhs.fIsExtended; fWeight = rhs.fWeight; fExecutionPolicy = rhs.fExecutionPolicy; }
La declaración sobrecargada no tiene un valor de retorno. También un problema muy común últimamente.
V596 El objeto fue creado pero no se está utilizando. Podría faltar la palabra clave 'throw': throw runtime_error (FOO); RTensor.hxx 363
template <typename Value_t, typename Container_t> inline RTensor<Value_t, Container_t> RTensor<Value_t, Container_t>::Transpose() { if (fLayout == MemoryLayout::RowMajor) { fLayout = MemoryLayout::ColumnMajor; } else if (fLayout == MemoryLayout::ColumnMajor) { fLayout = MemoryLayout::RowMajor; } else { std::runtime_error("Memory layout is not known."); } .... }
El error es que la palabra clave
throw se olvida accidentalmente. Como resultado, este código no arroja una excepción en caso de error.
En total, había dos de esos lugares. El segundo:
- V596 El objeto fue creado pero no se está utilizando. Podría faltar la palabra clave 'throw': throw runtime_error (FOO); Forest.hxx 137
V609 Divide por cero. Rango del denominador [0..100]. TGHtmlImage.cxx 340
const char *TGHtml::GetPctWidth(TGHtmlElement *p, char *opt, char *ret) { int n, m, val; .... if (n < 0 || n > 100) return z; if (opt[0] == 'h') { val = fCanvas->GetHeight() * 100; } else { val = fCanvas->GetWidth() * 100; } if (!fInTd) { snprintf(ret, 15, "%d", val / n);
Un caso similar a los descritos anteriormente sobre las matrices. Aquí, la variable
n está limitada a un rango de valores de 0 a 100. En este caso, hay una rama de código en la que se produce la división por la variable n con un valor de 0. Lo más probable es que la restricción del valor de n se reescriba de esta manera:
if (n <= 0 || n > 100) return z;
V646 Considere inspeccionar la lógica de la aplicación. Es posible que falte la palabra clave 'else'. TProofServ.cxx 729
TProofServ::TProofServ(Int_t *argc, char **argv, FILE *flog) : TApplication("proofserv", argc, argv, 0, -1) { .... if (!logmx.IsDigit()) { if (logmx.EndsWith("K")) { xf = 1024; logmx.Remove(TString::kTrailing, 'K'); } else if (logmx.EndsWith("M")) { xf = 1024*1024; logmx.Remove(TString::kTrailing, 'M'); } if (logmx.EndsWith("G")) { xf = 1024*1024*1024; logmx.Remove(TString::kTrailing, 'G'); } } .... }
El analizador detectó un formato extraño. En un lugar, falta la palabra clave
else . Del código podemos suponer que vale la pena arreglar el código.
Y un par de lugares para arreglar al mismo tiempo:
- V646 Considere inspeccionar la lógica de la aplicación. Es posible que falte la palabra clave 'else'. TFormula_v5.cxx 3702
- V646 Considere inspeccionar la lógica de la aplicación. Es posible que falte la palabra clave 'else'. RooAbsCategory.cxx 604
V663 Infinite loop es posible. La condición 'cin.eof ()' es insuficiente para romper el bucle. Considere agregar la llamada a la función 'cin.fail ()' a la expresión condicional. MétodoKNN.cxx 602
void TMVA::MethodKNN::ReadWeightsFromStream(std::istream& is) { .... while (!is.eof()) { std::string line; std::getline(is, line); if (line.empty() || line.find("#") != std::string::npos) { continue; } .... } .... }
Cuando se trabaja con la clase
std :: istream , llamar a la función
eof () no es suficiente para completar el ciclo. En el caso de una falla al leer los datos, llamar a la función
eof () siempre devolverá
falso , y no hay otros puntos de salida del bucle en este código. Para completar el ciclo en este caso, se requiere una verificación adicional del valor devuelto por la función
fail () :
while (!is.eof() && !is.fail()) { .... }
O simplemente escribe:
while (is) { .... }
V678 Un objeto se utiliza como argumento de su propio método. Considere verificar el primer argumento real de la función 'Copiar'. TFormLeafInfo.cxx 2414
TFormLeafInfoMultiVarDim::TFormLeafInfoMultiVarDim( const TFormLeafInfoMultiVarDim& orig) : TFormLeafInfo(orig) { fNsize = orig.fNsize; fSizes.Copy(fSizes);
Finalmente, aquí hay tal error. En lugar de
fSizes, tenía que pasar
orig.fSizes a la función
Copiar .
Conclusión
Hace aproximadamente un año, se proporcionó una descripción general del código del proyecto
NCBI Genome Workbench . Este proyecto también se utiliza en investigación científica, pero en el genoma. A lo que dirijo, el software en esta área es muy importante, pero su calidad no recibe la debida atención.
Por cierto, el otro día salió MacOS 10.15 Catalina, en el que se negaron a admitir aplicaciones de 32 bits. Y en PVS-Studio hay un gran conjunto de reglas para identificar problemas al portar programas a sistemas de 64 bits. Lea más sobre esto en la
publicación del
blog del analizador.
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Svyatoslav Razmyslov.
Analizando el Código de ROOT, Marco de Análisis de Datos Científicos .