La "aventura" con el cliente de correo electrónico Mozilla Thunderbird comenzó con una actualización automática a la versión 68.0. Las características notables de esta versión fueron las siguientes: se agrega más texto a las notificaciones emergentes y un tema oscuro de forma predeterminada. Hubo un error que quería intentar detectar usando análisis estático. Esta fue una ocasión para verificar una vez más el código fuente del proyecto utilizando PVS-Studio. Resultó que para el momento del análisis el error ya había sido reparado. Pero como llamamos la atención sobre este proyecto, podemos escribir sobre otros defectos encontrados en él.
Introduccion
El tema oscuro de la nueva versión de Thunderbird se ve bastante bonito. Amo los temas oscuros. Ya cambié a ellos en mensajería instantánea, Windows, macOS. Pronto, el iPhone se actualizará a iOS 13, donde ha aparecido un tema oscuro. Para esto, incluso tuve que cambiar mi iPhone 5S a un modelo más nuevo. En la práctica, resultó que el tema oscuro requiere más esfuerzo para que los desarrolladores elijan los colores de la interfaz. No todos lidian con esto la primera vez.
Entonces mis etiquetas estándar en Thunderbird comenzaron a verse así:
Utilizo seis etiquetas (5 estándar + 1 personalizada) para marcar correos electrónicos. Se hizo imposible ver la mitad de ellos después de la actualización, y decidí cambiar el color a más brillante en la configuración. Pero aquí me encontré con un error:
¡No puedes cambiar el color de la etiqueta! Más precisamente, es posible, pero el editor no permitirá que se guarde, haciendo referencia a un nombre existente (WTF ???).
Otra manifestación del error es la inacción del botón Aceptar, si intenta cambiar el nombre, ya que no puede guardar con este nombre. Tampoco puede cambiar el nombre.
Finalmente, puede notar que el tema oscuro no tocó la configuración, que tampoco es muy hermosa.
Después de una larga lucha con el sistema de compilación en Windows, todavía era posible ensamblar Thunderbird desde la fuente. La última versión del cliente de correo resultó ser mucho mejor que la última versión. En él, un tema oscuro llegó a la configuración, y este error con el editor de etiquetas también desapareció. Pero para que el trabajo de ensamblaje del proyecto no se desperdicie, se lanzó el analizador de código estático
PVS-Studio .
Nota El código fuente de Thunderbird se superpone de alguna manera con la base de código de Firefox. Por lo tanto, el análisis incluyó errores de diferentes componentes que los desarrolladores de diferentes equipos deberían analizar cuidadosamente.
Nota 2 Mientras se escribía el artículo, la actualización de Thunderbird 68.1 salió con una solución para este error:
comunicación
comm-central es un repositorio Mercurial del código de extensión Thunderbird, SeaMonkey y Lightning.
V501 Hay
subexpresiones idénticas '(! Strcmp (encabezado, "Reply-To"))' a la izquierda y a la derecha de '||' operador nsEmitterUtils.cpp 28
extern "C" bool EmitThisHeaderForPrefSetting(int32_t dispType, const char *header) { .... if (nsMimeHeaderDisplayTypes::NormalHeaders == dispType) { if ((!strcmp(header, HEADER_DATE)) || (!strcmp(header, HEADER_TO)) || (!strcmp(header, HEADER_SUBJECT)) || (!strcmp(header, HEADER_SENDER)) || (!strcmp(header, HEADER_RESENT_TO)) || (!strcmp(header, HEADER_RESENT_SENDER)) || (!strcmp(header, HEADER_RESENT_FROM)) || (!strcmp(header, HEADER_RESENT_CC)) || (!strcmp(header, HEADER_REPLY_TO)) || (!strcmp(header, HEADER_REFERENCES)) || (!strcmp(header, HEADER_NEWSGROUPS)) || (!strcmp(header, HEADER_MESSAGE_ID)) || (!strcmp(header, HEADER_FROM)) || (!strcmp(header, HEADER_FOLLOWUP_TO)) || (!strcmp(header, HEADER_CC)) || (!strcmp(header, HEADER_ORGANIZATION)) || (!strcmp(header, HEADER_REPLY_TO)) || (!strcmp(header, HEADER_BCC))) return true; else return false; .... }
La línea de
encabezado se comparó con la constante
HEADER_REPLY_TO dos veces. Quizás en su lugar debería haber habido otra constante.
V501 Hay
subexpresiones idénticas 'obj-> options-> headers! = MimeHeadersCitation' a la izquierda y a la derecha del operador '&&'. mimemsig.cpp 536
static int MimeMultipartSigned_emit_child(MimeObject *obj) { .... if (obj->options && obj->options->headers != MimeHeadersCitation && obj->options->write_html_p && obj->options->output_fn && obj->options->headers != MimeHeadersCitation && sig->crypto_closure) { .... } .... }
Otra comparación extraña de una variable con un nombre similar son los
encabezados . Como siempre, hay dos posibles explicaciones: un cheque adicional o un error tipográfico.
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: 1306, 1308. MapiApi.cpp 1306
void CMapiApi::ReportLongProp(const char *pTag, LPSPropValue pVal) { if (pVal && (PROP_TYPE(pVal->ulPropTag) == PT_LONG)) { nsCString num; nsCString num2; num.AppendInt((int32_t)pVal->Value.l); num2.AppendInt((int32_t)pVal->Value.l, 16); MAPI_TRACE3("%s %s, 0x%s\n", pTag, num, num2); } else if (pVal && (PROP_TYPE(pVal->ulPropTag) == PT_NULL)) { MAPI_TRACE1("%s {NULL}\n", pTag); } else if (pVal && (PROP_TYPE(pVal->ulPropTag) == PT_ERROR)) {
La cascada de expresiones condicionales se aceleró claramente presionando Ctrl + C y Ctrl + V. Como resultado, una de las ramas nunca se ejecuta.
V517 El uso del
patrón 'if (A) {...} else if (A) {...}' fue detectado. Hay una probabilidad de presencia de error lógico. Verifique las líneas: 777, 816. nsRDFContentSink.cpp 777
nsresult RDFContentSinkImpl::GetIdAboutAttribute(const char16_t** aAttributes, nsIRDFResource** aResource, bool* aIsAnonymous) { .... if (localName == nsGkAtoms::about) { .... } else if (localName == nsGkAtoms::ID) { .... } else if (localName == nsGkAtoms::nodeID) { nodeID.Assign(aAttributes[1]); } else if (localName == nsGkAtoms::about) {
La primera y la última condición son las mismas. El código muestra que el código está en proceso de escritura. Es seguro decir que con una alta probabilidad el error se manifestará después de finalizar el código. Un programador puede cambiar el código comentado, pero nunca tendrá el control. Tenga cuidado y cuidado con este código.
V522 Puede tener lugar la desreferenciación de la 'fila' de puntero nulo. morkRowCellCursor.cpp 175
NS_IMETHODIMP morkRowCellCursor::MakeCell(
La desreferenciación del puntero nulo de
fila es posible en la siguiente línea:
morkCell* cell = row->CellAt(ev, pos);
Lo más probable es que la inicialización del puntero se omitiera antes de esta línea, por ejemplo, usando el método
GetRow , etc.
V543 Es extraño que el valor '-1' se asigne a la variable 'm_lastError' del tipo HRESULT. MapiApi.cpp 1050
class CMapiApi { .... private: static HRESULT m_lastError; .... }; CMsgStore *CMapiApi::FindMessageStore(ULONG cbEid, LPENTRYID lpEid) { if (!m_lpSession) { MAPI_TRACE0("FindMessageStore called before session is open\n"); m_lastError = -1; return NULL; } .... }
El tipo
HRESULT es un tipo de datos complejo. Diferentes bits de una variable de este tipo representan diferentes campos de descripción de errores. El código de error debe establecerse utilizando constantes especiales de los archivos de encabezado del sistema.
Un par más de estos lugares:
- V543 Es extraño que el valor '-1' se asigne a la variable 'm_lastError' del tipo HRESULT. MapiApi.cpp 817
- V543 Es extraño que el valor '-1' se asigne a la variable 'm_lastError' del tipo HRESULT. MapiApi.cpp 1749
V579 La función memset recibe el puntero y su tamaño como argumentos. Posiblemente sea un error. Inspeccione el tercer argumento. icalmime.c 195
icalcomponent* icalmime_parse(....) { struct sspm_part *parts; int i, last_level=0; icalcomponent *root=0, *parent=0, *comp=0, *last = 0; if ( (parts = (struct sspm_part *) malloc(NUM_PARTS*sizeof(struct sspm_part)))==0) { icalerror_set_errno(ICAL_NEWFAILED_ERROR); return 0; } memset(parts,0,sizeof(parts)); sspm_parse_mime(parts, NUM_PARTS, icalmime_local_action_map, get_string, data, 0 ); .... }
La variable de
partes es un puntero a una matriz de estructuras. Para restablecer los valores de las estructuras, utilizaron la función
memset , pero le transfirieron el tamaño del puntero como el tamaño de una pieza de memoria.
Otros lugares sospechosos:
- V579 La función memset recibe el puntero y su tamaño como argumentos. Posiblemente sea un error. Inspeccione el tercer argumento. icalmime.c 385
- V579 La función memset recibe el puntero y su tamaño como argumentos. Posiblemente sea un error. Inspeccione el tercer argumento. icalparameter.c 114
- V579 La función snprintf recibe el puntero y su tamaño como argumentos. Posiblemente sea un error. Inspeccione el segundo argumento. icaltimezone.c 1908
- V579 La función snprintf recibe el puntero y su tamaño como argumentos. Posiblemente sea un error. Inspeccione el segundo argumento. icaltimezone.c 1910
- V579 La función strncmp recibe el puntero y su tamaño como argumentos. Posiblemente sea un error. Inspeccione el tercer argumento. sspm.c 707
- V579 La función strncmp recibe el puntero y su tamaño como argumentos. Posiblemente sea un error. Inspeccione el tercer argumento. sspm.c 813
V595 El puntero 'aValues' se utilizó antes de que se verificara contra nullptr. Verifique las líneas: 553, 555. nsLDAPMessage.cpp 553
NS_IMETHODIMP nsLDAPMessage::GetBinaryValues(const char *aAttr, uint32_t *aCount, nsILDAPBERValue ***aValues) { .... *aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; } .... }
El diagnóstico
V595 generalmente encuentra errores típicos de
anulación de referencia de puntero nulo. Pero se encontró un caso muy interesante, digno de atención especial.
Técnicamente, el analizador tiene razón en que primero se desreferencia el puntero
aValues , luego se verifica, pero el error es diferente. Este es un puntero doble, por lo que el código correcto debería verse así:
*aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!*aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; }
Otro lugar muy similar:
- V595 El puntero '_retval' se utilizó antes de verificarlo con nullptr. Verifique las líneas: 357, 358. nsLDAPSyncQuery.cpp 357
V1044 Las condiciones de ruptura de bucle no dependen del número de iteraciones. mimemoz2.cpp 1795
void ResetChannelCharset(MimeObject *obj) { .... if (cSet) { char *ptr2 = cSet; while ((*cSet) && (*cSet != ' ') && (*cSet != ';') && (*cSet != '\r') && (*cSet != '\n') && (*cSet != '"')) ptr2++; if (*cSet) { PR_FREEIF(obj->options->default_charset); obj->options->default_charset = strdup(cSet); obj->options->override_charset = true; } PR_FREEIF(cSet); } .... }
Este error se encontró usando un nuevo diagnóstico, que estará disponible en la próxima versión del analizador. Todas las variables utilizadas en la condición para detener el
ciclo while no se modifican, porque las variables
ptr2 y
cSet se mezclan en el cuerpo de la función.
Netwerk
netwerk contiene interfaces C y código para acceso de bajo nivel a la red (usando sockets y cachés de archivos y memoria), así como acceso de nivel superior (usando varios protocolos como http, ftp, gopher, castanet). Este código también se conoce con los nombres "netlib" y "Necko".
V501 Hay
subexpresiones idénticas 'connectStarted' a la izquierda y a la derecha del operador '&&'. nsSocketTransport2.cpp 1693
nsresult nsSocketTransport::InitiateSocket() { .... if (gSocketTransportService->IsTelemetryEnabledAndNotSleepPhase() && connectStarted && connectCalled) {
Al principio, pensé que duplicar la variable
connectStarted es un código superfluo hasta que examiné toda la función lo suficientemente larga y encontré un fragmento similar. Lo más probable es que, en lugar de una sola variable,
connectStarted aquí también sea una variable
connectCalled .
V611 La memoria se asignó usando el operador 'nuevo T []' pero se liberó usando el operador 'borrar'. Considere inspeccionar este código. Probablemente sea mejor usar 'delete [] mData;'. Líneas de verificación: 233, 222. DataChannel.cpp 233
BufferedOutgoingMsg::BufferedOutgoingMsg(OutgoingMsg& msg) { size_t length = msg.GetLeft(); auto* tmp = new uint8_t[length];
El puntero
mData apunta a una matriz, no a un solo objeto. Cometieron un error en el destructor de clase, olvidando agregar corchetes para el operador de
eliminación .
V1044 Las condiciones de ruptura de bucle no dependen del número de iteraciones. ParseFTPList.cpp 691
int ParseFTPList(....) { .... pos = toklen[2]; while (pos > (sizeof(result->fe_size) - 1)) pos = (sizeof(result->fe_size) - 1); memcpy(result->fe_size, tokens[2], pos); result->fe_size[pos] = '\0'; .... }
El valor de
pos se sobrescribe en el bucle por la misma cantidad. Parece que los nuevos diagnósticos encontraron otro error.
gfx
gfx contiene interfaces C y código para dibujo e imágenes independientes de la plataforma. Se puede usar para dibujar rectángulos, líneas, imágenes, etc. Esencialmente, es un conjunto de interfaces para un contexto de dispositivo (dibujo) independiente de la plataforma. No maneja widgets o rutinas de dibujo específicas; solo proporciona las operaciones primitivas para dibujar.
V501 Hay
subexpresiones idénticas a la izquierda y a la derecha de '||' operador: mVRSystem || mVRCompositor || mVRSystem OpenVRSession.cpp 876
void OpenVRSession::Shutdown() { StopHapticTimer(); StopHapticThread(); if (mVRSystem || mVRCompositor || mVRSystem) { ::vr::VR_Shutdown(); mVRCompositor = nullptr; mVRChaperone = nullptr; mVRSystem = nullptr; } }
En esta condición, la variable
mVRSystem está presente dos veces. Obviamente, uno de ellos debe reemplazarse con
mVRChaperona .
dom
dom contiene interfaces C y código para implementar y rastrear objetos DOM (Modelo de objetos de documento) en Javascript. Forma la subestructura C que crea, destruye y manipula objetos incorporados y definidos por el usuario de acuerdo con el script Javascript.
V570 La variable 'clonedDoc-> mPreloadReferrerInfo' se asigna a sí misma. Document.cpp 12049
already_AddRefed<Document> Document::CreateStaticClone( nsIDocShell* aCloneContainer) { .... clonedDoc->mReferrerInfo = static_cast<dom::ReferrerInfo*>(mReferrerInfo.get())->Clone(); clonedDoc->mPreloadReferrerInfo = clonedDoc->mPreloadReferrerInfo; .... }
El analizador detectó la asignación de una variable a sí mismo.
xpcom
xpcom contiene las interfaces C de bajo nivel, el código C, el código C, un poco de código de ensamblaje y las herramientas de línea de comandos para implementar la maquinaria básica de los componentes XPCOM (que significa "Modelo de objetos componentes multiplataforma"). XPCOM es el mecanismo que permite a Mozilla exportar interfaces y tenerlas disponibles automáticamente para los scripts de JavaScript, Microsoft COM y el código regular de Mozilla C.
V611 La memoria se asignó utilizando la función 'malloc / realloc' pero se liberó utilizando el operador 'eliminar'. Considere inspeccionar las lógicas de operación detrás de la variable 'clave'. Verifique las líneas: 143, 140. nsINIParser.h 143
struct INIValue { INIValue(const char* aKey, const char* aValue) : key(strdup(aKey)), value(strdup(aValue)) {} ~INIValue() { delete key; delete value; } void SetValue(const char* aValue) { delete value; value = strdup(aValue); } const char* key; const char* value; mozilla::UniquePtr<INIValue> next; };
Después de llamar a la función
strdup , debe liberar memoria utilizando la función
libre , no el operador de
eliminación .
V716 Conversión de tipo sospechoso en la inicialización: 'HRESULT var = BOOL'. SpecialSystemDirectory.cpp 73
BOOL SHGetSpecialFolderPathW( HWND hwnd, LPWSTR pszPath, int csidl, BOOL fCreate ); static nsresult GetWindowsFolder(int aFolder, nsIFile** aFile) { WCHAR path_orig[MAX_PATH + 3]; WCHAR* path = path_orig + 1; HRESULT result = SHGetSpecialFolderPathW(nullptr, path, aFolder, true); if (!SUCCEEDED(result)) { return NS_ERROR_FAILURE; } .... }
La función
WinGI SHGetSpecialFolderPathW devuelve un valor de tipo
BOOL , no
HRESULT . La comprobación del resultado de la función debe reescribirse en la correcta.
nsprpub
nsprpub contiene código C para la biblioteca de tiempo de ejecución "C" multiplataforma. La biblioteca de tiempo de ejecución "C" contiene funciones básicas C no visuales para asignar y desasignar memoria, obtener la hora y la fecha, leer y escribir archivos, manejar hilos y manejar y comparar cadenas en todas las plataformas
V647 El valor del tipo 'int' se asigna al puntero del tipo 'corto'. Considere inspeccionar la asignación: 'out_flags = 0x2'. prsocket.c 1220
#define PR_POLL_WRITE 0x2 static PRInt16 PR_CALLBACK SocketPoll( PRFileDesc *fd, PRInt16 in_flags, PRInt16 *out_flags) { *out_flags = 0; #if defined(_WIN64) if (in_flags & PR_POLL_WRITE) { if (fd->secret->alreadyConnected) { out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; } } #endif return in_flags; }
El analizador detectó la asignación de una constante numérica al puntero
out_flags . Lo más probable es que simplemente olvidaron desreferenciarlo:
if (fd->secret->alreadyConnected) { *out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; }
Conclusión
Este no es el final. Ser nuevas revisiones de código. El código Thunderbird y Firefox incluye dos bibliotecas principales: Network Security Services (NSS) y WebRTC (Web Real Time Communications). Hubo algunos errores muy interesantes. En esta revisión, mostraré uno a la vez.
NssV597 El compilador podría eliminar la llamada de función 'memset', que se utiliza para vaciar el búfer 'newdeskey'. La función RtlSecureZeroMemory () debe usarse para borrar los datos privados. pkcs11c.c 1033
static CK_RV sftk_CryptInit(....) { .... unsigned char newdeskey[24]; .... context->cipherInfo = DES_CreateContext( useNewKey ? newdeskey : (unsigned char *)att->attrib.pValue, (unsigned char *)pMechanism->pParameter, t, isEncrypt); if (useNewKey) memset(newdeskey, 0, sizeof newdeskey); sftk_FreeAttribute(att); .... }
NSS es una biblioteca para desarrollar aplicaciones seguras de cliente y servidor, y aquí la clave DES no se limpia. El compilador eliminará la llamada de
memset del código, como la matriz
newdeskey ya no se usa en el código más allá de esta ubicación.
WebRTCV519 La
variable 'state [state_length - x_length + i]' tiene valores asignados dos veces sucesivamente. Quizás esto sea un error. Verifique las líneas: 83, 84. filter_ar.c 84
size_t WebRtcSpl_FilterAR(....) { .... for (i = 0; i < state_length - x_length; i++) { state[i] = state[i + x_length]; state_low[i] = state_low[i + x_length]; } for (i = 0; i < x_length; i++) { state[state_length - x_length + i] = filtered[i]; state[state_length - x_length + i] = filtered_low[i];
En el segundo ciclo, los datos se escriben en la matriz incorrecta, porque el autor copió el código y olvidó cambiar el nombre de la matriz de
state a
state_low .
Probablemente hay errores más interesantes en estos proyectos que vale la pena mencionar. Y lo haremos en el futuro cercano. Mientras tanto, prueba
PVS-Studio en tu proyecto.
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Svyatoslav Razmyslov.
Tema oscuro de Thunderbird como razón para ejecutar un analizador de código .