0 AD es un juego tridimensional en el género de la estrategia histórica en tiempo real, desarrollado por la comunidad de voluntarios. El tamaño de la base del código es pequeño y decidí comprobar el juego como un descanso de grandes proyectos como Android y XNU Kernel. Entonces, ante nosotros hay un proyecto que contiene 165,000 líneas de código en C ++. Veamos qué es lo interesante que se puede encontrar usando el analizador estático PVS-Studio.
0 AD
0 AD (0 A.D.) es un juego tridimensional gratuito en el género de la estrategia histórica en tiempo real, desarrollado por una comunidad de voluntarios (los principales desarrolladores están unidos en el equipo de Wildfire Games). El juego te permite controlar civilizaciones que existieron en el período 500 AC. e.- 1 año antes de Cristo. e. A partir del verano de 2018, el proyecto está en versión alfa. [
Descripción tomada de Wikipedia].
¿Por qué exactamente 0 AD?
Le pedí a un colega de
Egor Bredikhin (
EgorBredikhin ) que eligiera y verificara para mí algún pequeño proyecto abierto con el que pudiera trabajar entre otras tareas. Me envió un registro para el proyecto AD 0. A la pregunta: "¿Por qué este proyecto?" - hubo una respuesta: "Sí, acabo de jugar este juego, una buena estrategia en tiempo real". Ok, entonces será 0 AD :).
Densidad de error
Quiero elogiar a los autores de 0 AD por la buena calidad del código C ++. Bien hecho, rara vez logras cumplir con una densidad tan baja de errores. Por supuesto, estos no son todos los errores, sino aquellos que se pueden detectar con PVS-Studio. Como dije, aunque PVS-Studio no encuentra todos los errores, sin embargo, podemos hablar con seguridad sobre la relación entre la densidad de errores y la calidad del código en su conjunto.
Unos cuantos números. El número total de líneas de código no vacías es 231270. De estas, el 28.7% son comentarios. En total, 165,000 líneas de código C ++ puro.
El número de mensajes emitidos por el analizador era pequeño, y después de mirarlos a todos, escribí 19 errores. Consideraré todos estos errores más adelante en el artículo. Tal vez me perdí algo, considerando que el error es un código descuidado inofensivo. Sin embargo, en general, esto no cambia la imagen.
Entonces, encontré 19 errores en 165,000 líneas de código. Calculamos la densidad del error: 19 * 1000/165000 = 0.115.
Para simplificar, redondeamos y suponemos que el analizador PVS-Studio detecta 0.1 errores por 1000 líneas de código en el código del juego.
Gran resultado! A modo de comparación, en mi
artículo reciente
sobre Android, calculé que detecté al menos 0.25 errores por 1000 líneas de código. De hecho, la densidad de errores allí es aún mayor, simplemente no encontré la fuerza para analizar todo el informe cuidadosamente.
O tome las bibliotecas principales de EFL como ejemplo, que
estudié cuidadosamente y calculé el número de defectos. En él, PVS-Studio detecta 0.71 errores por 1000 líneas de código.
Por lo tanto, los autores de 0 AD son buenos compañeros, sin embargo, en aras de la equidad, debe tenerse en cuenta que una pequeña cantidad de código escrito en C ++ les ayuda. Desafortunadamente, cuanto más grande es el proyecto, más rápido crece su complejidad y la densidad de error aumenta de forma no lineal (
más ).
Errores
Veamos ahora los 19 errores que encontré en el juego. Para analizar el proyecto, utilicé
PVS-Studio versión 6.24. Le sugiero que intente descargar la versión demo y verifique los proyectos en los que está trabajando.
Nota Posicionamos PVS-Studio como una solución B2B. Para proyectos pequeños y desarrolladores individuales, tenemos una opción de licencia gratuita: "
Cómo usar PVS-Studio de forma gratuita ".
Error N1Comencemos mirando un error complejo. De hecho, no es complicado, pero tendrá que familiarizarse con un código bastante grande.
void WaterManager::CreateWaveMeshes() { .... int nbNeighb = 0; .... bool found = false; nbNeighb = 0; for (int p = 0; p < 8; ++p) { if (CoastalPointsSet.count(xx+around[p][0] + (yy + around[p][1])*SideSize)) { if (nbNeighb >= 2) { CoastalPointsSet.erase(xx + yy*SideSize); continue; } ++nbNeighb;
Advertencia de PVS-Studio:
V547 CWE-570 La expresión 'nbNeighb> = 2' siempre es falsa. WaterManager.cpp 581
A primera vista, el mensaje del analizador parece extraño. ¿Por qué la condición
nbNeighb> = 2 siempre es falsa? De hecho, en el cuerpo del bucle hay un incremento de la variable
nbNeighb !
Mire a continuación y verá una declaración de interrupción que interrumpe la ejecución del bucle. Por lo tanto, si aumenta la variable
nbNeighb , el ciclo se detendrá. Por lo tanto, el valor de la variable
nbNeighb nunca alcanzará un valor mayor que 1.
El código claramente contiene algún tipo de error lógico.
Error N2 void CmpRallyPointRenderer::MergeVisibilitySegments( std::deque<SVisibilitySegment>& segments) { .... segments.erase(segments.end()); .... }
Advertencia de PVS-Studio:
V783 CWE-119 Puede tener lugar la desreferenciación del iterador inválido 'segmentos.end ()'. CCmpRallyPointRenderer.cpp 1290
Muy, muy extraño código. Quizás el programador quería eliminar el último elemento del contenedor. En este caso, el código debería ser así:
segments.erase(segments.end() - 1);
Aunque, entonces sería más fácil escribir:
segments.pop_back();
Para ser honesto, no está del todo claro para mí qué se debería haber escrito exactamente aquí.
Error N3, N4Decidí considerar dos errores juntos, ya que están relacionados con una pérdida de recursos y requieren primero mostrar qué es la macro
WARN_RETURN .
#define WARN_RETURN(status)\ do\ {\ DEBUG_WARN_ERR(status);\ return status;\ }\ while(0)
Entonces, como puede ver, la macro
WARN_RETURN hace que la función salga del cuerpo. Ahora veamos formas inexactas de usar esta macro.
El primer fragmento.
Status sys_generate_random_bytes(u8* buf, size_t count) { FILE* f = fopen("/dev/urandom", "rb"); if (!f) WARN_RETURN(ERR::FAIL); while (count) { size_t numread = fread(buf, 1, count, f); if (numread == 0) WARN_RETURN(ERR::FAIL); buf += numread; count -= numread; } fclose(f); return INFO::OK; }
Advertencia de PVS-Studio:
V773 CWE-401 La función se cerró sin soltar el controlador 'f'. Una fuga de recursos es posible. unix.cpp 332
Si la función
fread no puede leer los datos, la función
sys_generate_random_bytes se cerrará sin liberar el descriptor de archivo. En la práctica, esto es casi imposible. Es dudoso que no pueda leer datos de "/ dev / urandom". Sin embargo, el código es descuidado.
El segundo fragmento.
Status sys_cursor_create(....) { .... sys_cursor_impl* impl = new sys_cursor_impl; impl->image = image; impl->cursor = XcursorImageLoadCursor(wminfo.info.x11.display, image); if(impl->cursor == None) WARN_RETURN(ERR::FAIL); *cursor = static_cast<sys_cursor>(impl); return INFO::OK; }
Advertencia de PVS-Studio: V773 CWE-401 La función se cerró sin soltar el puntero 'impl'. Una pérdida de memoria es posible. x.cpp 421
Si el cursor no se carga, se produce una pérdida de memoria.
Error N5 Status LoadHeightmapImageOs(....) { .... shared_ptr<u8> fileData = shared_ptr<u8>(new u8[fileSize]); .... }
Advertencia de PVS-Studio:
V554 CWE-762 Uso incorrecto de shared_ptr. La memoria asignada con 'nuevo []' se limpiará con 'eliminar'. MapIO.cpp 54
La opción correcta:
shared_ptr<u8[]> fileData = shared_ptr<u8>(new u8[fileSize]);
Error N6 FUTrackedPtr(ObjectClass* _ptr = NULL) : ptr(_ptr) { if (ptr != NULL) FUTracker::TrackObject((FUTrackable*) ptr); ptr = ptr; }
Advertencia de PVS-Studio:
V570 La variable 'ptr' se asigna a sí misma. FUTracker.h 122
Error N7, N8 std::wstring TraceEntry::EncodeAsText() const { const wchar_t action = (wchar_t)m_action; wchar_t buf[1000]; swprintf_s(buf, ARRAY_SIZE(buf), L"%#010f: %c \"%ls\" %lu\n", m_timestamp, action, m_pathname.string().c_str(), (unsigned long)m_size); return buf; }
Advertencia de PVS-Studio:
V576 CWE-628 Formato incorrecto. Considere verificar el quinto argumento real de la función 'swprintf_s'. Se espera el argumento de tipo char. trace.cpp 93
Aquí nos encontramos con un historial confuso e indistinto de una implementación alternativa de la función
swprintf en Visual C ++. No lo volveré a
contar , pero consulte la documentación para los diagnósticos del
V576 (consulte la sección "Líneas anchas").
En este caso, lo más probable es que este código funcione correctamente al compilar en Visual C ++ para Windows e incorrectamente al compilar para Linux o macOS.
Error similar: V576 CWE-628 Formato incorrecto. Considere verificar el cuarto argumento real de la función 'swprintf_s'. Se espera el argumento de tipo char. vfs_tree.cpp 211
Error N9, N10, N11Clásico Al principio, se usa el puntero, y solo luego se verifica.
static void TEST_CAT2(char* dst, ....) { strcpy(dst, dst_val);
Advertencia de PVS-Studio:
V595 CWE-476 El puntero 'dst' se utilizó antes de verificarlo con nullptr. Líneas de verificación: 140, 143. test_secure_crt.h 140
Creo que el error no requiere explicación. Advertencias similares:
- V595 CWE-476 El puntero 'dst' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 150, 153. test_secure_crt.h 150
- V595 CWE-476 El puntero 'dst' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 314, 317. test_secure_crt.h 314
Error N12 typedef int tbool; void MikkTSpace::setTSpace(...., const tbool bIsOrientationPreserving, ....) { .... m_NewVertices.push_back(bIsOrientationPreserving > 0.5 ? 1.0f : (-1.0f)); .... }
V674 CWE-682 El literal '0.5' del tipo 'doble' se compara con un valor del tipo 'int'. Considere inspeccionar la expresión 'bIsOrientationPreserving> 0.5'. MikktspaceWrap.cpp 137
No tiene sentido comparar una variable de tipo
int con una constante de 0.5. Además, en términos de significado, esta es generalmente una variable de tipo booleano, lo que significa que compararlo con 0.5 parece muy extraño. Suponga que en lugar de
bIsOrientationPreserving se debe
usar otra variable aquí.
Error N13 virtual Status ReplaceFile(const VfsPath& pathname, const shared_ptr<u8>& fileContents, size_t size) { ScopedLock s; VfsDirectory* directory; VfsFile* file; Status st; st = vfs_Lookup(pathname, &m_rootDirectory, directory, &file, VFS_LOOKUP_ADD|VFS_LOOKUP_CREATE);
Advertencia de PVS-Studio:
V749 CWE-675 Destructor del objeto 's' se invocará por segunda vez después de abandonar el alcance del objeto. vfs.cpp 165
Antes de crear un archivo, es necesario que un objeto de tipo
ScopedLock "desbloquee" un objeto. Para esto, se llama explícitamente a un destructor. El problema es que el destructor para el objeto
s se volverá a llamar automáticamente cuando salga la función. Es decir el destructor se llamará dos veces. No he estudiado el dispositivo de la clase
ScopedLock , pero en cualquier caso, no debes hacer esto. A menudo, esta doble llamada al destructor conduce a un comportamiento indefinido u otros errores desagradables. Incluso si el código funciona bien ahora, es muy fácil romper todo cambiando la implementación de la clase
ScopedLock .
Error N14, N15, N16, N17 CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { .... pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; .... }
Advertencia de PVS-Studio:
V668 CWE-570 No tiene sentido probar el puntero 'pEvent' contra nulo, ya que la memoria se asignó utilizando el operador 'nuevo'. La excepción se generará en caso de error de asignación de memoria. fsm.cpp 259
Comprobar el puntero no tiene sentido, porque en caso de un error de asignación de memoria, se
lanzará una excepción
std :: bad_alloc .
Entonces, la verificación es superflua, pero el error no es grave. Sin embargo, todo es mucho peor cuando se ejecuta algo de lógica en el cuerpo de la
instrucción if . Considere el siguiente caso:
CFsmTransition* CFsm::AddTransition(....) { .... CFsmEvent* pEvent = AddEvent( eventType ); if ( !pEvent ) return NULL;
Advertencia del analizador: V668 CWE-570 No tiene sentido probar el puntero 'pNewTransition' contra nulo, ya que la memoria se asignó utilizando el operador 'nuevo'. La excepción se generará en caso de error de asignación de memoria. fsm.cpp 289
Se intenta liberar memoria cuya dirección se almacena en el puntero
pEvent . Naturalmente, esto no sucederá y se producirá una pérdida de memoria.
De hecho, cuando comencé a lidiar con este código, resultó que todo era más complicado y tal vez no hubo un error, sino dos. Ahora explicaré lo que está mal con este código. Para hacer esto, necesitamos familiarizarnos con el dispositivo de función
AddEvent .
CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { CFsmEvent* pEvent = NULL;
Tenga en cuenta que la función no siempre devuelve un puntero a un nuevo objeto creado con el
nuevo operador. Algunas veces toma un objeto existente del contenedor
m_Events . Y el puntero al objeto recién creado, por cierto, también se colocará en
m_Events .
Y aquí surge la pregunta: ¿quién posee y debe destruir los objetos cuyos punteros están almacenados en el contenedor
m_Events ? No estoy familiarizado con el proyecto, pero lo más probable es que en algún lugar haya un código que destruya todos estos objetos. Luego, eliminar un objeto dentro de la función
CFsm :: AddTransition es generalmente superfluo.
Tengo la impresión de que simplemente puede eliminar el siguiente fragmento de código:
if ( !pNewTransition ) { delete pEvent; return NULL; }
Otros errores:
- V668 CWE-571 No tiene sentido probar el puntero 'ret' contra nulo, ya que la memoria se asignó utilizando el operador 'nuevo'. La excepción se generará en caso de error de asignación de memoria. TerrainTextureEntry.cpp 120
- V668 CWE-571 No tiene sentido probar el puntero de 'respuesta' contra nulo, ya que la memoria se asignó utilizando el operador 'nuevo'. La excepción se generará en caso de error de asignación de memoria. SoundManager.cpp 542
Error N18, N19 static void dir_scan_callback(struct de *de, void *data) { struct dir_scan_data *dsd = (struct dir_scan_data *) data; if (dsd->entries == NULL || dsd->num_entries >= dsd->arr_size) { dsd->arr_size *= 2; dsd->entries = (struct de *) realloc(dsd->entries, dsd->arr_size * sizeof(dsd->entries[0])); } if (dsd->entries == NULL) {
Advertencia de PVS-Studio:
V701 CWE-401 realloc () posible fuga: cuando realloc () falla en la asignación de memoria, el puntero original 'dsd-> entradas' se pierde. Considere asignar realloc () a un puntero temporal. mongoose.cpp 2462
Si el tamaño de la matriz se vuelve insuficiente, la memoria se asigna utilizando la función
realloc . El error es que el valor del puntero al bloque de memoria original se sobrescribe inmediatamente con el nuevo valor devuelto por la función
realloc .
Si la asignación de memoria falla, la función
realloc devolverá NULL, y este NULL se escribirá en la variable
dsd-> entradas . Después de lo cual será imposible liberar el bloque de memoria cuya dirección se almacenó previamente en
entradas dsd-> . Se producirá una pérdida de memoria.
Otro error: V701 CWE-401 realloc () posible fuga: cuando realloc () falla en la asignación de memoria, se pierde el puntero original 'Buffer'. Considere asignar realloc () a un puntero temporal. Preprocessor.cpp 84
Conclusión
No puedo decir que esta vez el artículo resultó ser fascinante, o que logré mostrar muchos errores terribles. Qué hacer, una vez a la vez, no es necesario.
Lo que veo, luego lo escribo .
Gracias por su atencion Para variar, terminaré el artículo con una invitación para seguirme en Instagram
@pvs_studio_unicorn y en Twitter
@Code_Analysis .
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Andrey Karpov.
Buen trabajo, autores del juego 0 AD!