Durante todo el período de su existencia, las personas han hecho un gran esfuerzo para estudiar casi toda el área del cielo estrellado. Hasta la fecha, hemos examinado cientos de miles de asteroides, cometas, nebulosas y estrellas, galaxias y planetas. Para ver toda esta belleza usted mismo, no es necesario salir de la casa y comprar un telescopio. Puede instalar Stellarium, un planetario virtual en su computadora, y mirar el cielo nocturno, acostado cómodamente en el sofá ... ¿Pero es cómodo? Para encontrar la respuesta a esta pregunta, revisaremos Stellarium en busca de errores en el código de la computadora.
Un poco sobre el proyecto ...
Como se
describe en el sitio web de Wikipedia,
Stellarium es un planetario virtual de código abierto disponible para Linux, Mac OS X, Microsoft Windows, Symbian, Android e iOS, así como MeeGo. El programa utiliza tecnologías OpenGL y Qt para crear un cielo realista en tiempo real. Con Stellarium, puede ver lo que puede ver con un telescopio mediano e incluso grande. El programa también proporciona observaciones de eclipses solares y el movimiento de los cometas.
Stellarium fue creado por el programador francés Fabian Chereau, quien lanzó el proyecto en el verano de 2001. Otros desarrolladores destacados incluyen Robert Spearman, Johannes Gadzhozik, Matthew Gates, Timothy Reeves, Bogdan Marinov y Johan Meeris, quien es responsable de la obra de arte.
... y sobre el analizador
El análisis del proyecto se realizó utilizando el analizador de código estático PVS-Studio. Esta es una herramienta para detectar errores y vulnerabilidades potenciales en el código fuente de programas escritos en C, C ++ y C # (¡pronto en Java!). Se ejecuta en Windows, Linux y macOS. Está diseñado para aquellos que necesitan mejorar la calidad de su código.
El análisis fue bastante simple. Primero,
descargué el proyecto Stellarium de GitHub y luego instalé todos los paquetes necesarios para el ensamblaje. Como el proyecto se creó con Qt Creator, utilicé el sistema de seguimiento del lanzamiento del compilador, que está integrado en la versión
independiente del analizador. Allí puede ver el informe de análisis terminado.
Los nuevos lectores y usuarios de
Stellarium pueden haberse preguntado: ¿por qué aparece el unicornio en el título del artículo y cómo se relaciona con el análisis de código? Respondo: soy uno de los desarrolladores de PVS-Studio, y el unicornio es nuestra mascota traviesa favorita. Así que arriba!
Espero que gracias a este artículo, los lectores aprendan algo nuevo por sí mismos y los desarrolladores de Stellarium puedan eliminar algunos errores y mejorar la calidad del código.
Tómate un café con un croissant de aire y ponte cómodo, porque vamos a la parte más interesante: ¡una descripción general de los resultados del análisis y el análisis de errores!
Condiciones sospechosas
Para un mayor placer de lectura, sugiero que no mire directamente la advertencia del analizador, sino que intente aquí y más lejos para encontrar errores usted mismo.
void QZipReaderPrivate::scanFiles() { ....
Advertencia de PVS-Studio: V654 La condición 'start_of_directory == - 1' del bucle siempre es verdadera. qzip.cpp 617
¿Puedes encontrar un error? Si es así, alabado sea.
El error radica en la condición del
bucle while . Siempre es cierto, ya que la variable
start_of_directory no cambia en el cuerpo del bucle. Lo más probable es que el ciclo no sea eterno, ya que contiene
retorno y
descanso , pero ese código parece extraño.
Me parece que en el código se olvidaron de hacer la asignación
start_of_directory = pos en el lugar donde se verifica la firma. Entonces la declaración de
ruptura es quizás superflua. En este caso, el código se puede reescribir así:
int i = 0; int start_of_directory = -1; EndOfDirectory eod; while (start_of_directory == -1) { const int pos = device->size() - int(sizeof(EndOfDirectory)) - i; if (pos < 0 || i > 65535) { qWarning() << "QZip: EndOfDirectory not found"; return; } device->seek(pos); device->read((char *)&eod, sizeof(EndOfDirectory)); if (readUInt(eod.signature) == 0x06054b50) start_of_directory = pos; ++i; }
Sin embargo, no estoy seguro de cómo debería ser el código así. Es mejor que los desarrolladores del proyecto analicen esta parte del programa y realicen los cambios necesarios.
Otra condición extraña:
class StelProjectorCylinder : public StelProjector { public: .... protected: .... virtual bool intersectViewportDiscontinuityInternal(const Vec3d& capN, double capD) const { static const SphericalCap cap1(1,0,0); static const SphericalCap cap2(-1,0,0); static const SphericalCap cap3(0,0,-1); SphericalCap cap(capN, capD); return cap.intersects(cap1) && cap.intersects(cap2) && cap.intersects(cap2); } };
Advertencia de PVS-Studio: V501 Hay
subexpresiones idénticas 'cap.intersects (cap2)' a la izquierda y a la derecha del operador '&&'. StelProjectorClasses.hpp 175
Como probablemente ya haya adivinado, el error radica en la última línea de la función: el programador cometió un error tipográfico y al final resultó que la función devuelve el resultado independientemente del valor de
cap3 .
Este tipo de error es extremadamente común: en casi todos los proyectos probados, hemos encontrado errores tipográficos relacionados con los nombres del formulario
nombre1 y
nombre2 y similares. Típicamente, tales errores están relacionados con copiar y pegar.
Esta instancia de código es un excelente ejemplo de otro patrón de error común, para el cual incluso realizamos un mini estudio por separado. Mi colega Andrei Karpov lo llamó el "
efecto de última línea ". Si no está familiarizado con este material, le sugiero que abra una pestaña en el navegador para leer más tarde, pero por ahora, continuemos.
void BottomStelBar::updateText(bool updatePos) { .... updatePos = true; .... if (location->text() != newLocation || updatePos) { updatePos = true; .... } .... if (fov->text() != str) { updatePos = true; .... } .... if (fps->text() != str) { updatePos = true; .... } if (updatePos) { .... } }
Advertencias de PVS-Studio:- V560 Una parte de la expresión condicional siempre es verdadera: updatePos. StelGuiItems.cpp 732
- La expresión V547 'updatePos' siempre es verdadera. StelGuiItems.cpp 831
- El parámetro V763 'updatePos' siempre se reescribe en el cuerpo de la función antes de usarse. StelGuiItems.cpp 690
El valor del parámetro
updatePos siempre se sobrescribe antes de usarse, es decir, la función funcionará igual, independientemente del valor que se le pase.
Se ve raro, ¿no? En todos los lugares donde está
involucrado el parámetro
updatePos , es
cierto . Esto significa que las condiciones
if (location-> text ()! = NewLocation || updatePos) y
if (updatePos) siempre serán verdaderas.
Otro fragmento:
void LandscapeMgr::onTargetLocationChanged(StelLocation loc) { .... if (pl && flagEnvironmentAutoEnabling) { QSettings* conf = StelApp::getInstance().getSettings(); setFlagAtmosphere(pl->hasAtmosphere() & conf->value("landscape/flag_atmosphere", true).toBool()); setFlagFog(pl->hasAtmosphere() & conf->value("landscape/flag_fog", true).toBool()); setFlagLandscape(true); } .... }
Advertencias de PVS-Studio:- V792 Se llamará a la función 'toBool' ubicada a la derecha del operador '&' independientemente del valor del operando izquierdo. Quizás, es mejor usar '&&'. LandscapeMgr.cpp 782
- V792 Se llamará a la función 'toBool' ubicada a la derecha del operador '&' independientemente del valor del operando izquierdo. Quizás, es mejor usar '&&'. LandscapeMgr.cpp 783
El analizador detectó una expresión sospechosa en los argumentos de las
funciones setFlagAtmosphere y
setFlagFog . De hecho: en ambos lados del operador de bits
y hay valores de tipo
bool . En lugar del operador
& , debe usar el operador
&& , y ahora explicaré por qué.
Sí, el resultado de esta expresión siempre será correcto. Antes de utilizar el bit a bit "y", ambos operandos serán promovidos a
int . En C ++, dicha conversión es
inequívoca : falso se convierte a 0 y verdadero se convierte a 1. Por lo tanto, el resultado de esta expresión será el mismo que si se utilizara el operador
&& .
Pero hay un matiz. Al calcular el resultado de la operación
&& , se utiliza el llamado "cálculo diferido". Si el valor del operando izquierdo es
falso , entonces el valor derecho ni siquiera se calcula, porque el "y" lógico en cualquier caso devolverá
falso . Esto se hace para ahorrar recursos informáticos y le permite escribir diseños más complejos. Por ejemplo, puede verificar que el puntero no sea nulo y, de ser así, desreferenciarlo para realizar una verificación adicional. Ejemplo:
if (ptr && ptr-> foo ()) .
Este "cálculo
diferido " no se realiza cuando se utiliza el operador bit a bit
& : expresiones
conf-> value ("...", true) .toBool () se evaluará cada vez, independientemente del valor
pl-> hasAtmosphere () .
En casos raros, esto se hace a propósito. Por ejemplo, si calcular el operando correcto tiene "efectos secundarios", el resultado se utilizará más adelante. Tampoco es muy bueno hacerlo, porque complica la comprensión del código y complica el mantenimiento del mismo. Además, el orden de cálculo de los operandos
& no
está definido, por lo que en algunos casos de usar tales "trucos" puede obtener un comportamiento indefinido.
Si necesita guardar efectos secundarios, hágalo en una línea separada y guarde el resultado en una variable separada. Las personas que trabajarán con este código en el futuro te lo agradecerán :)
Pasamos al siguiente tema.
Manejo incorrecto de la memoria
Comencemos el tema de la memoria dinámica con este fragmento:
GLUEShalfEdge* __gl_meshMakeEdge(GLUESmesh* mesh) { GLUESvertex* newVertex1 = allocVertex(); GLUESvertex* newVertex2 = allocVertex(); GLUESface* newFace = allocFace(); GLUEShalfEdge* e; if ( newVertex1 == NULL || newVertex2 == NULL || newFace == NULL) { if (newVertex1 != NULL) { memFree(newVertex1); } if (newVertex2 != NULL) { memFree(newVertex2); } if (newFace != NULL) { memFree(newFace); } return NULL; } e = MakeEdge(&mesh->eHead); if (e == NULL) { return NULL; } MakeVertex(newVertex1, e, &mesh->vHead); MakeVertex(newVertex2, e->Sym, &mesh->vHead); MakeFace(newFace, e, &mesh->fHead); return e; }
Advertencias de PVS-Studio:- V773 Se salió de la función sin liberar el puntero 'newVertex1'. Una pérdida de memoria es posible. mesh.c 312
- V773 Se salió de la función sin liberar el puntero 'newVertex2'. Una pérdida de memoria es posible. mesh.c 312
- V773 La función se salió sin liberar el puntero 'newFace'. Una pérdida de memoria es posible. mesh.c 312
La función asigna memoria para varias estructuras y la pasa a los punteros
newVertex1 ,
newVertex2 (nombres interesantes, ¿verdad?) Y
newFace . Si uno de ellos resulta ser cero, entonces toda la memoria reservada dentro de la función se libera, después de lo cual el flujo de control abandona la función.
¿Qué sucede si la memoria para las tres estructuras se asigna correctamente y la función
MakeEdge (& mesh-> eHead) devuelve
NULL ? El flujo de control alcanzará el segundo
retorno .
Dado que los punteros
newVertex1 ,
newVertex2 y
newFace son variables locales, dejarán de existir después de salir de la función. Pero la liberación de la memoria que les pertenecía no sucederá. Permanecerá reservado, pero ya no tendremos acceso a él.
Tales situaciones se llaman pérdidas de memoria. Un escenario típico con tal error: con el uso prolongado del programa, comienza a consumir más y más RAM, hasta su agotamiento.
Cabe señalar que en este ejemplo, el tercer
retorno no
es erróneo. Las
funciones MakeVertex y
MakeFace transfieren las direcciones de la memoria asignada a otras estructuras de datos, delegando así la responsabilidad de su lanzamiento.
El siguiente defecto está en el método, que toma 90 líneas. Por conveniencia, lo reduje, dejando solo áreas problemáticas.
void AstroCalcDialog::drawAngularDistanceGraph() { .... QVector<double> xs, ys; .... }
Solo queda una línea. Déjame darte una pista: esta es la única mención de los objetos
xs e
ys .
Advertencias de PVS-Studio:- Se creó el objeto V808 'xs' del tipo 'QVector' pero no se utilizó. AstroCalcDialog.cpp 5329
- Se creó el objeto V808 'ys' del tipo 'QVector' pero no se utilizó. AstroCalcDialog.cpp 5329
Los vectores
xs e
ys se crean, pero no se usan en ninguna parte. Resulta que cada vez que usa el método
drawAngularDistanceGraph , se
produce una creación y eliminación adicional de un contenedor vacío. Creo que este anuncio permaneció en el código después de refactorizar. Esto, por supuesto, no es un error, pero debe eliminar el código adicional.
Moldes de tipo extraño
Otro ejemplo después de un poco de formato se ve así:
void SatellitesDialog::updateSatelliteData() { ....
Para entender cuál es el error, deberá observar los prototipos
de los constructores de la clase Qcolor :
Advertencias de PVS-Studio:- V674 El literal '0.4' del tipo 'doble' se está convirtiendo implícitamente al tipo 'int' mientras se llama a la función 'QColor'. Inspecciona el primer argumento. SatellitesDialog.cpp 413
- V674 El literal '0.4' del tipo 'doble' se está convirtiendo implícitamente al tipo 'int' mientras se llama a la función 'QColor'. Inspeccione el segundo argumento. SatellitesDialog.cpp 413
- V674 El literal '0.4' del tipo 'doble' se está convirtiendo implícitamente al tipo 'int' mientras se llama a la función 'QColor'. Inspeccione el tercer argumento. SatellitesDialog.cpp 413
La clase
Qcolor no tiene constructores que acepten el tipo
doble , por lo que los argumentos del ejemplo se convertirán implícitamente a
int . Esto hace que los campos
r ,
g ,
b del objeto
buttonColor tengan un valor de
0 .
Si el programador pretendía crear un objeto a partir de valores de tipo
double , debería usar un constructor diferente.
Por ejemplo, podría usar un constructor que acepte
Qrgb escribiendo:
buttonColor = QColor(QColor::fromRgbF(0.4, 0.4, 0.4));
Podría haberse hecho de manera diferente. Qt utiliza valores reales en el rango [0.0, 1.0] o valores enteros en el rango [0, 255] para indicar colores RGB.
Por lo tanto, el programador podría traducir los valores de real a entero escribiendo así:
buttonColor = QColor((int)(255 * 0.4), (int)(255 * 0.4), (int)(255 * 0.4));
o solo
buttonColor = QColor(102, 102, 102);
Estas aburrido No se preocupe: hay más errores interesantes por delante.
"Unicornio en el espacio". Vista desde el Stellarium.Otros errores
Al final, te dejé un poco más delicioso :) Vamos a ir a uno de ellos.
HipsTile* HipsSurvey::getTile(int order, int pix) { .... if (order == orderMin && !allsky.isNull()) { int nbw = sqrt(12 * 1 << (2 * order)); int x = (pix % nbw) * allsky.width() / nbw; int y = (pix / nbw) * allsky.width() / nbw; int s = allsky.width() / nbw; QImage image = allsky.copy(x, y, s, s); .... } .... }
Advertencia de PVS-Studio: V634 La prioridad de la operación '*' es mayor que la de la operación '<<'. Es posible que se utilicen paréntesis en la expresión. StelHips.cpp 271
Bueno, ¿pudiste detectar el error? Considere la expresión
(12 * 1 << (orden 2 *)) con más detalle. El analizador recuerda que la operación '
* ' tiene una prioridad más alta que la operación de cambio de bit '
<< '. Es fácil ver que multiplicar
12 por
1 no tiene sentido, y los corchetes alrededor del
orden 2 * no
son necesarios.
, : int nbw = sqrt(12 * (1 << 2 * order)); <i>12 </i> .
Nota Además, quiero señalar que si el valor del operando derecho '
<< ' es mayor o igual que el número de bits del operando izquierdo, entonces el resultado no está definido. Dado que los literales numéricos son
int por defecto, que toma
32 bits, el valor del parámetro de
orden no debe exceder
15 . De lo contrario, la evaluación de la expresión puede dar lugar a un comportamiento indefinido.
Continuamos El siguiente método es muy confuso, pero estoy seguro de que un lector sofisticado manejará la detección de errores :)
QCPRange QCPStatisticalBox:: getKeyRange(bool& foundRange, SignDomain inSignDomain) const { foundRange = true; if (inSignDomain == sdBoth) { return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); } else if (inSignDomain == sdNegative) { if (mKey + mWidth * 0.5 < 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey < 0) return QCPRange(mKey - mWidth * 0.5, mKey); else { foundRange = false; return QCPRange(); } } else if (inSignDomain == sdPositive) { if (mKey - mWidth * 0.5 > 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey > 0) return QCPRange(mKey, mKey + mWidth * 0.5); else { foundRange = false; return QCPRange(); } } foundRange = false; return QCPRange(); }
Advertencia de PVS-Studio: V779 Código inalcanzable detectado. Es posible que haya un error presente. qcustomplot.cpp 19512.
El hecho es que todo
si ... de lo contrario las ramas tienen un
retorno . Por lo tanto, el flujo de control nunca alcanza las dos últimas líneas.
En general, este ejemplo se ejecutará normalmente y funcionará correctamente. Pero la presencia de un código inalcanzable solo es una señal. En este caso, indica la estructura incorrecta del método, lo que complica en gran medida la legibilidad y la comprensión del código.
Este fragmento de código debe ser refactorizado, obteniendo una función más ordenada en la salida. Por ejemplo, así:
QCPRange QCPStatisticalBox:: getKeyRange(bool& foundRange, SignDomain inSignDomain) const { foundRange = true; switch (inSignDomain) { case sdBoth: { return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); break; } case sdNegative: { if (mKey + mWidth * 0.5 < 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey < 0) return QCPRange(mKey - mWidth * 0.5, mKey); break; } case sdPositive: { if (mKey - mWidth * 0.5 > 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey > 0) return QCPRange(mKey, mKey + mWidth * 0.5); break; } } foundRange = false; return QCPRange(); }
El último en nuestra revisión será el error que más me gustó. El código de punto de problema es corto y simple:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) : distance(0.0f), sDistance(0.0f) { Plane(v1, v2, v3, SPolygon::CCW); }
¿Has notado algo sospechoso? No todos pueden :)
Advertencia de PVS-Studio: V603 El objeto fue creado pero no se está utilizando. Si desea llamar al constructor, debe usar 'this-> Plane :: Plane (....)'. Plane.cpp 29
El programador esperaba que algunos de los campos del objeto se inicializaran dentro del constructor anidado, pero resultó de esta manera: cuando se
llama al constructor
Plane (Vec3f & v1, Vec3f & v2, Vec3f & v3) , se crea un objeto temporal sin nombre que se elimina de inmediato. Como resultado, una parte del objeto permanece sin inicializar.
Para que el código funcione correctamente, debe usar la característica conveniente y segura de C ++ 11: el constructor delegante:
Plane::Plane(Vec3f& v1, Vec3f& v2, Vec3f& v3) : Plane(v1, v2, v3, SPolygon::CCW) { distance = 0.0f; sDistance = 0.0f; }
Pero si usa el compilador para versiones anteriores del lenguaje, puede escribir así:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) : distance(0.0f), sDistance(0.0f) { this->Plane::Plane(v1, v2, v3, SPolygon::CCW); }
Más o menos:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) : distance(0.0f), sDistance(0.0f) { new (this) Plane(v1, v2, v3, SPolygon::CCW); }
Observo que los dos últimos métodos son
muy peligrosos . Por lo tanto, debe tener mucho cuidado y comprender bien cómo funcionan dichos métodos.
Conclusión
¿Qué conclusiones se pueden hacer sobre la calidad del código Stellarium? Honestamente, no hubo muchos errores. Además, en todo el proyecto, no encontré un solo error en el que el código esté vinculado a un comportamiento indefinido. Para el proyecto de código abierto, la calidad del código resultó ser de alto nivel, por lo que me quito el sombrero ante los desarrolladores. ¡Ustedes son geniales! Me complació e interesó revisar su proyecto.
¿Qué pasa con el planetario mismo? Lo uso con bastante frecuencia. Desafortunadamente, viviendo en la ciudad, rara vez puedo disfrutar de un cielo nocturno despejado, y Stellarium me permite estar en cualquier parte del mundo sin levantarme del sofá. ¡Es realmente cómodo!
Me gusta especialmente el modo "Constellation art". La vista de las enormes figuras que cubren todo el cielo en un extraño baile es impresionante.
"Un baile extraño". Vista desde el Stellarium.Los terrícolas tienden a cometer errores, y no hay nada vergonzoso en el hecho de que estos errores se filtren en el código. Para esto, se desarrollan herramientas de análisis de código, como PVS-Studio. Si eres uno de los terrícolas
, me gusta, te sugiero que lo
descargues y lo pruebes tú mismo .
Espero que haya estado interesado en leer mi artículo y haya aprendido algo nuevo y útil para usted. Y les deseo a los desarrolladores una corrección temprana de los errores encontrados.
¡Suscríbete a nuestros canales y mantente atento a las noticias del mundo de la programación!
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: George Gribkov.
Hacia el espacio de nuevo: cómo el unicornio visitó Stellarium