Cualquier aplicación moderna grande consta de numerosas bibliotecas de terceros, y me gustaría hablar sobre el tema de nuestra confianza en estas bibliotecas. En libros y artículos, hay muchos debates sobre la calidad del código, los métodos de prueba, las metodologías de desarrollo, etc. Pero no recuerdo a nadie discutiendo la calidad de los ladrillos con los que se construyen las aplicaciones. Así que hablemos de eso hoy. Por ejemplo, existe el kit de herramientas de registro y segmentación de Medicine Insight (ITK). Me parece que está bastante bien implementado. Al menos, he notado algunos errores en su código. Pero no puedo decir lo mismo sobre el código de las bibliotecas de terceros que se utilizan allí. Entonces la pregunta es:¿cuánto podemos confiar en tales sistemas? Mucho alimento para el pensamiento.
Al desarrollar aplicaciones médicas, todo el mundo habla de estándares de calidad y codificación; se exige a los programadores que sigan estándares como MISRA, etc. A decir verdad, no estoy muy familiarizado con las metodologías utilizadas al escribir aplicaciones críticas para la seguridad. Pero sospecho que la cuestión de la calidad de las bibliotecas de terceros utilizadas en el desarrollo a menudo se ignora. El código de la aplicación y el código de las bibliotecas de terceros viven sus propias vidas por separado.
Esta conclusión se extrae de mis observaciones subjetivas. Muy a menudo me encuentro con aplicaciones de muy alta calidad en las que no puedo encontrar ni media docena de errores graves. Al mismo tiempo, dichas aplicaciones pueden incluir bibliotecas de terceros de muy mala calidad.
Supongamos que un médico hace un diagnóstico incorrecto debido a algunos artefactos en la imagen provocados por un error en el software. En este caso, no importa un poco si este error está en el propio programa o en la biblioteca de manejo de imágenes. Piénsalo.
Lo que me hizo pensar en todo de nuevo fue la comprobación de los códigos fuente del proyecto ITK:
Perspectiva Segmentación y Registro Juego de herramientas (ITK). ITK es un código abierto, multiplataforma sistema eso proporciona desarrolladores con un extenso suite de software herramientas para imagen análisis. Desarrollado a través extremo programación metodologías, ITK emplea vanguardia algoritmos para registrándose y segmentación datos multidimensionales.
Mientras analizaba el proyecto ITK con PVS-Studio, una vez más noté lo siguiente. Hubo algunos fragmentos sospechosos relacionados con el proyecto ITK en sí, pero al mismo tiempo muchos fragmentos sospechosos y errores obvios en los archivos almacenados en la carpeta "ThirdParty".
No es de extrañar. ITK incluye bastantes bibliotecas. Pero eso es bastante triste:algunos errores en esas bibliotecas pueden afectar el funcionamiento de ITK.
No voy a apelar por ningún acto drástico ni dar recomendaciones; mi objetivo es atraer la atención de la gente hacia mis hallazgos para que puedan reflexionar sobre ellos. Para que mis palabras se queden grabadas en tu memoria, te mostraré algunos fragmentos sospechosos que me han llamado la atención.
Comencemos con la biblioteca OpenJPEG
Caso pobre
typedef enum PROG_ORDER {
PROG_UNKNOWN = -1,
LRCP = 0,
RLCP = 1,
RPCL = 2,
PCRL = 3,
CPRL = 4
} OPJ_PROG_ORDER;
OPJ_INT32 pi_check_next_level(....)
{
....
case 'P':
switch(tcp->prg)
{
case LRCP||RLCP:
if(tcp->prc_t == tcp->prcE){
l=pi_check_next_level(i-1,cp,tileno,pino,prog);
....
}Mensaje de diagnóstico de PVS-Studio:V560 Una parte de la expresión condicional siempre es verdadera:RLCP. pic.c 1708
El programador olvidó cómo usar correctamente el operador 'caso'. La declaración "caso LRCP||RLCP:" es equivalente a "caso 1:". Y esto obviamente no es lo que pretendía el programador.
El código correcto debería tener el siguiente aspecto:
case LRCP:
case RLCP:Y eso es exactamente lo que está escrito en otros lugares del programa. Bueno, también agregaría un comentario, algo como esto:
case LRCP: // fall through
case RLCP:
Desreferenciación de puntero nulo
bool j2k_write_rgn(....)
{
OPJ_BYTE * l_current_data = 00;
OPJ_UINT32 l_nb_comp;
OPJ_UINT32 l_rgn_size;
opj_image_t *l_image = 00;
opj_cp_t *l_cp = 00;
opj_tcp_t *l_tcp = 00;
opj_tccp_t *l_tccp = 00;
OPJ_UINT32 l_comp_room;
// preconditions
assert(p_j2k != 00);
assert(p_manager != 00);
assert(p_stream != 00);
l_cp = &(p_j2k->m_cp);
l_tcp = &l_cp->tcps[p_tile_no];
l_tccp = &l_tcp->tccps[p_comp_no];
l_nb_comp = l_image->numcomps;
....
}Mensaje de diagnóstico de PVS-Studio:V522 Es posible que se elimine la referencia del puntero nulo 'l_image'. j2k.c 5205
El puntero 'l_image' se inicializa a cero y no cambia en ningún lugar después de eso. Por lo tanto, al llamar a la función j2k_write_rgn(), se eliminará la referencia al puntero nulo.
Una variable asignada a sí misma
OPJ_SIZE_T opj_stream_write_skip (....)
{
....
if (!l_is_written)
{
p_stream->m_status |= opj_stream_e_error;
p_stream->m_bytes_in_buffer = 0;
p_stream->m_current_data = p_stream->m_current_data;
return (OPJ_SIZE_T) -1;
}
....
}Mensaje de diagnóstico de PVS-Studio:V570 La variable 'p_stream->m_current_data' está asignada a sí misma. cio.c 675
Algo está mal en este código. A una variable se le asigna su propio valor.
Cheque incorrecto
typedef struct opj_stepsize
{
OPJ_UINT32 expn;
OPJ_UINT32 mant;
};
bool j2k_read_SQcd_SQcc(
opj_j2k_t *p_j2k,
OPJ_UINT32 p_comp_no,
OPJ_BYTE* p_header_data,
OPJ_UINT32 * p_header_size,
struct opj_event_mgr * p_manager
)
{
....
OPJ_UINT32 l_band_no;
....
l_tccp->stepsizes[l_band_no].expn =
((l_tccp->stepsizes[0].expn) - ((l_band_no - 1) / 3) > 0) ?
(l_tccp->stepsizes[0].expn) - ((l_band_no - 1) / 3) : 0;
....
}Mensaje de diagnóstico de PVS-Studio:V555 La expresión del tipo 'A - B> 0' funcionará como 'A !=B'. itkopenjpeg j2k.c 3421
No es fácil encontrar rápidamente el error en este fragmento, así que hice un ejemplo artificial simplificado:
unsigned A, B;
....
X = (A - B > 0) ? (A - B) : 0;Según tengo entendido, el programador pretendía hacer lo siguiente. Si la variable A es mayor que B, entonces se debe calcular la diferencia; si no, la expresión debería evaluarse como cero.
Eligió una manera incorrecta de escribir esta comparación. Dado que la expresión (A - B) no tiene signo, siempre será mayor o igual a 0. Por ejemplo, si "A =3, B =5", entonces (A - B) es igual a 0xFFFFFFFE (4294967294).
Entonces parece que esta expresión se puede simplificar:
X = (A != B) ? (A - B) : 0;Si (A ==B), obtendremos 0 como diferencia. Significa que la expresión se puede simplificar aún más:
X = A - B;Algo está obviamente mal. La forma correcta de escribir esta comparación es la siguiente:
X = (A > B) ? (A - B) : 0;GDC
Bueno, basta de JPEG; no queremos que el artículo se convierta en un libro de referencia. Hay otras bibliotecas para discutir, por ejemplo, la biblioteca Grassroots DICOM (GDCM).
Condición de bucle incorrecta
bool Sorter::StableSort(std::vector<std::string> const & filenames)
{
....
std::vector< SmartPointer<FileWithName> >::iterator
it2 = filelist.begin();
for( Directory::FilenamesType::const_iterator it =
filenames.begin();
it != filenames.end(), it2 != filelist.end();
++it, ++it2)
{
....
}Mensaje de diagnóstico de PVS-Studio:V521 Tales expresiones que usan el operador ',' son peligrosas. Asegúrate de que la expresión sea correcta. gdcmsorter.cxx 82
El operador coma ',' en la condición de bucle no tiene sentido. El resultado de este operador es su operando derecho. Por lo tanto, la expresión "it !=filenames.end()" no se tiene en cuenta de ninguna manera.
El bucle probablemente debería verse así:
for(Directory::FilenamesType::const_iterator it = ....;
it != filenames.end() && it2 != filelist.end();
++it, ++it2)Un poco más adelante en el código, hay otro bucle incorrecto similar (gdcmsorter.cxx 123).
Posible desreferenciación de puntero nulo
bool PrivateTag::ReadFromCommaSeparatedString(const char *str)
{
unsigned int group = 0, element = 0;
std::string owner;
owner.resize( strlen(str) );
if( !str || sscanf(str, "%04x,%04x,%s", &group ,
&element, &owner[0] ) != 3 )
{
gdcmDebugMacro( "Problem reading Private Tag: " << str );
return false;
}
....
}Mensaje de diagnóstico de PVS-Studio:V595 El puntero 'str' se utilizó antes de que se verificara contra nullptr. Verifique las líneas:26, 27. gdcmprivatetag.cxx 26
Puede ver en la condición que el puntero 'str' puede ser igual a nullptr. Sin embargo, este puntero es desreferenciado sin ser chequeado en la siguiente línea:
owner.resize( strlen(str) );Comportamiento no especificado
bool ImageCodec::DoOverlayCleanup(
std::istream &is, std::ostream &os)
{
....
// nmask : to propagate sign bit on negative values
int16_t nmask = (int16_t)0x8000;
nmask = nmask >>
( PF.GetBitsAllocated() - PF.GetBitsStored() - 1 );
....
}Mensaje de diagnóstico de PVS-Studio:V610 Comportamiento no especificado. Compruebe el operador de turno '>>. El operando izquierdo 'nmask' es negativo. gdcmimagecodec.cxx 397
Cambiar valores negativos a través del operador ">>" conduce a un comportamiento no especificado. Confiar en la suerte es inaceptable para tales bibliotecas.
Lectura peligrosa del archivo
void LookupTable::Decode(....) const
{
....
while( !is.eof() )
{
unsigned short idx;
unsigned short rgb[3];
is.read( (char*)(&idx), 2);
if( is.eof() ) break;
if( IncompleteLUT )
{
assert( idx < Internal->Length[RED] );
assert( idx < Internal->Length[GREEN] );
assert( idx < Internal->Length[BLUE] );
}
rgb[RED] = rgb16[3*idx+RED];
rgb[GREEN] = rgb16[3*idx+GREEN];
rgb[BLUE] = rgb16[3*idx+BLUE];
os.write((char*)rgb, 3*2);
}
....
}Mensaje de diagnóstico de PVS-Studio:V663 El bucle infinito es posible. La condición 'cin.eof()' es insuficiente para salir del bucle. Considere agregar la llamada de función 'cin.fail()' a la expresión condicional. gdcmMSFF gdcmlookuptable.cxx 280
Verá, el programa puede colgarse en este lugar. Si algo desencadena un error al leer el archivo, la comprobación "is.eof()" no podrá detener el ciclo. En caso de error, el programa no puede leer el archivo. Pero aún no se ha llegado al final del archivo. Y estas son cosas bastante diferentes.
Se debe agregar una verificación adicional que se puede implementar a través de una llamada de la función is.fail().
Hay muchos otros errores peligrosos al leer archivos. Recomiendo a los desarrolladores que revisen todos los fragmentos donde se llama a la función eof(). Estos fragmentos se pueden encontrar tanto en GDCM como en otras bibliotecas.
ITC
Vamos a terminar con las bibliotecas aquí. Creo que he logrado aclararte mi preocupación.
Quizás a los lectores les interese saber si he encontrado algo en la propia biblioteca ITK. Sí, hubo algunas cuestiones interesantes.
El efecto de la última línea
Recientemente escribí un artículo divertido titulado "El efecto de la última línea". Si aún no lo has leído, te lo recomiendo.
Aquí hay otra forma en que este efecto se manifiesta. En la última y tercera línea, el índice debe ser '2' en lugar de '1'.
int itkPointSetToSpatialObjectDemonsRegistrationTest(....)
{
....
// Set its position
EllipseType::TransformType::OffsetType offset;
offset[0]=50;
offset[1]=50;
offset[1]=50;
....
}Mensaje de diagnóstico de PVS-Studio:V519 A la variable 'offset[1]' se le asignan valores dos veces seguidas. Quizás esto sea un error. Verifique las líneas:41, 42. itkpointsettospatialobjectdemonsregistrationtest.cxx 42
Un error tipográfico
Aquí hay un error tipográfico más con un índice de matriz:
template< typename TCoordRepType >
void
VoronoiDiagram2D< TCoordRepType >::SetOrigin(PointType vorsize)
{
m_VoronoiBoundaryOrigin[0] = vorsize[0];
m_VoronoiBoundaryOrigin[0] = vorsize[1];
}Mensaje de diagnóstico de PVS-Studio:V519 A la variable 'm_VoronoiBoundaryOrigin[0]' se le asignan valores dos veces seguidas. Quizás esto sea un error. Verificar líneas:74, 75. itkvoronoidiagram2d.hxx 75
Un índice faltante
void MultiThreader::MultipleMethodExecute()
{
....
HANDLE process_id[ITK_MAX_THREADS];
....
process_id[thread_loop] = (void *) _beginthreadex(0, 0, ....);
if ( process_id == 0 )
{
itkExceptionMacro("Error in thread creation !!!");
}
....
}Mensaje de diagnóstico de PVS-Studio:V600 Considere inspeccionar la condición. El puntero 'process_id' siempre no es igual a NULL. itkmultithreaderwinthreads.cxx 90
La verificación "if (process_id ==0)" no tiene sentido. El programador quería verificar un elemento de la matriz y el código debía verse así:
if ( process_id[thread_loop] == 0 )
Cheques idénticos
template< typename T >
void WriteCellDataBufferAsASCII(....)
{
....
if( this->m_NumberOfCellPixelComponents == 3 )
{
....
}
else if( this->m_NumberOfCellPixelComponents == 3 )
{
....
}
....
}Mensajes de diagnóstico de PVS-Studio:V517 Se detectó el uso del patrón 'if (A) {...} else if (A) {...}'. Hay una probabilidad de presencia de error lógico. Verifique las líneas:948, 968. itkvtkpolydatameshio.h 948
Constructor sospechoso
template<typename LayerType, typename TTargetVector>
QuickPropLearningRule <LayerType,TTargetVector>
::QuickPropLearningRule()
{
m_Momentum = 0.9; //Default
m_Max_Growth_Factor = 1.75;
m_Decay = -0.0001;
m_SplitEpsilon = 1;
m_Epsilon = 0.55;
m_Threshold = 0.0;
m_SigmoidPrimeOffset = 0;
m_SplitEpsilon = 0;
}Mensajes de diagnóstico de PVS-Studio:V519 A la variable 'm_SplitEpsilon' se le asignan valores dos veces seguidas. Quizás esto sea un error. Verifique las líneas:35, 39. itkquickproplearningrule.hxx 39
Observe cómo se inicializa la variable 'm_SplitEpsilon'. Al principio, a este miembro de la clase se le asigna el valor 1 y luego 0. Eso es bastante extraño.
Borrado de caché incorrecto
template <typename TInputImage, typename TOutputImage>
void
PatchBasedDenoisingImageFilter<TInputImage, TOutputImage>
::EmptyCaches()
{
for (unsigned int threadId = 0;
threadId < m_ThreadData.size(); ++threadId)
{
SizeValueType cacheSize =
m_ThreadData[threadId].eigenValsCache.size();
for (SizeValueType c = 0; c < cacheSize; ++c)
{
delete m_ThreadData[threadId].eigenValsCache[c];
delete m_ThreadData[threadId].eigenVecsCache[c];
}
m_ThreadData[threadId].eigenValsCache.empty();
m_ThreadData[threadId].eigenVecsCache.empty();
}
}Mensajes de diagnóstico de PVS-Studio:
- V530 Se requiere utilizar el valor de retorno de la función 'vacío'. itkpatchbaseddenoisingimagefilter.hxx 85
- V530 Se requiere utilizar el valor de retorno de la función 'vacío'. itkpatchbaseddenoisingimagefilter.hxx 86
Debido a la falta de atención, el programador implementó una llamada a la función 'empty()' en lugar de 'clear()'. Conduce a agregar basura al caché, por lo que usarlo se vuelve peligroso. Este error es difícil de encontrar y puede provocar efectos secundarios muy extraños.
Otros errores
Hubo otros errores, tanto en ITK como en las bibliotecas de terceros. Pero me prometí encajar el artículo en 12 páginas, mientras lo escribía en Microsoft Word. Verás, no me gusta que mis artículos tiendan a crecer en tamaño cada vez más. Así que tengo que restringirme. La razón por la que los artículos son cada vez más extensos es que el analizador de PVS-Studio está aprendiendo a encontrar más y más errores.
Está bien que no haya descrito todos los fragmentos sospechosos. Para ser honesto, estaba revisando rápidamente el informe y seguramente me perdí mucho. No trate este artículo como una colección de advertencias; en cambio, quiero que estimule a algunos de ustedes a comenzar a usar analizadores estáticos en su trabajo con regularidad. Será mucho mejor así, ya que no puedo comprobar todos los programas del mundo.
Si los autores de ITK revisan su proyecto ellos mismos, será mucho mejor que hacer arreglos basándose en mi artículo. Lamentablemente, PVS-Studio genera demasiados falsos positivos en ITK. La razón es que el código usa algunas macros especiales. Los resultados del análisis se pueden mejorar significativamente mediante una ligera personalización. Si es necesario, pídeme consejo, estaré encantado de ayudarte.
Conclusión
Estimados lectores, recuerden que las comprobaciones únicas de los analizadores estáticos solo les brindan un pequeño beneficio. Solo usarlos regularmente realmente lo ayudará a ahorrar tiempo. Esta idea se discute en detalle en la publicación "Leo Tolstoy y el análisis de código estático".
¡Que sus programas y bibliotecas permanezcan libres de errores!