Las aventuras con el cliente de correo Mozilla Thunderbird comenzaron con la actualización automática a la versión 68.0. Más características en las notificaciones emergentes y el tema oscuro predeterminado son las características notables de esta versión. Ocasionalmente encontré un error que inmediatamente ansiaba detectar con análisis estático. Esta se convirtió en la razón para realizar otra verificación del código fuente del proyecto utilizando PVS-Studio. Dio la casualidad de que en el momento del análisis, el error ya había sido reparado. Sin embargo, dado que hemos prestado cierta atención al proyecto, no hay razón para no escribir sobre otros defectos encontrados.
Introduccion
El tema oscuro de la nueva versión de Thunderbird se ve bonito. Me gustan los temas oscuros. Ya me he cambiado a ellos en messenger, Windows, macOS. Pronto el iPhone se actualizará a iOS 13 con un tema oscuro. Por esta razón, incluso tuve que cambiar mi iPhone 5S por un modelo más nuevo. En la práctica, resultó que un tema oscuro requiere más esfuerzo por parte de los desarrolladores para elegir los colores de la interfaz. No todos pueden manejarlo la primera vez.
Así es como se veían las etiquetas estándar en Thunderbird después de la actualización:
Normalmente uso 6 etiquetas (5 estándar +1 personalizado) para marcar correos electrónicos. La mitad de ellos se volvió imposible de ver después de la actualización, así que decidí cambiar el color en la configuración a uno más brillante. En este punto me quedé atrapado con un error:
¡No puedes cambiar el color de una etiqueta! Más sinceramente, puede, pero el editor no le permitirá guardarlo, refiriéndose a un nombre ya existente (WTF ???).
Otro síntoma de este error es un botón OK inactivo. Como no podía hacer cambios en la misma etiqueta de nombre, intenté cambiar su nombre. Bueno, resulta que tampoco puedes cambiarle el nombre.
Finalmente, es posible que haya notado que el tema oscuro no funcionó para la configuración, que tampoco es muy agradable.
Después de una larga lucha con el sistema de compilación en Windows, finalmente construí Thunderbird a partir de los archivos de origen. La última versión del cliente de correo resultó ser mucho mejor que la nueva versión. En él, el tema oscuro también llegó a la configuración, y este error con el editor de etiquetas desapareció. Sin embargo, para garantizar que la construcción del proyecto no sea solo una pérdida de tiempo, el analizador de código estático
PVS-Studio se puso a trabajar.
Nota El código fuente de Thunderbird se cruza con la base de código de Firefox de alguna manera. Por lo tanto, el análisis incluye errores de diferentes componentes, que los desarrolladores de estos equipos merecen una mirada atenta.
Nota 2. Mientras escribía el artículo, se lanzó Thunderbird 68.1 y se corrigió 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 cadena del
encabezado se comparó con la constante
HEADER_REPLY_TO dos veces. Tal vez debería haber habido otra constante en su lugar.
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:
encabezados . Como siempre, hay dos posibles explicaciones: un cheque innecesario 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)) {
Las teclas Ctrl + C y Ctrl + V definitivamente ayudaron a acelerar la escritura de esta cascada de expresiones condicionales. Como resultado, una de las ramas nunca se ejecutará.
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 todavía está en proceso de escritura. Se puede decir con seguridad que el error se mostrará después de refinar el código. Un programador puede cambiar el código comentado, pero nunca podrá controlarlo. Por favor, tenga mucho cuidado y atención con este código.
V522 Puede tener lugar la desreferenciación de la 'fila' de puntero nulo. morkRowCellCursor.cpp 175
NS_IMETHODIMP morkRowCellCursor::MakeCell(
Posible desreferencia del puntero nulo de
fila en la siguiente línea:
morkCell* cell = row->CellAt(ev, pos);
Lo más probable es que no se haya inicializado un puntero, por ejemplo, mediante 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. Sus diferentes bits representan diferentes campos de una descripción de error. Debe establecer el código de error utilizando constantes especiales de los archivos de encabezado del sistema.
Un par de fragmentos como este:
- 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, los autores utilizaron la función
memset , pero pasaron el tamaño del puntero como el tamaño del espacio de memoria.
Fragmentos sospechosos similares:
- 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 detecta errores típicos de desreferencia de puntero nulo. En este caso tenemos un ejemplo extremadamente interesante, digno de especial atención.
Técnicamente, el analizador tiene razón en que el puntero
aValues primero se desreferencia y luego se verifica, pero el error real es diferente. Es un puntero doble, por lo que el código correcto debería tener el siguiente aspecto:
*aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!*aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; }
Otro fragmento 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 encuentra utilizando un nuevo diagnóstico que estará disponible en la próxima versión del analizador. Todas las variables utilizadas en la condición del bucle while no cambian, ya que las variables
ptr2 y
cSet se confunden 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) {
Primero pensé que duplicar la variable
connectStarted es solo código redundante. Pero luego examiné toda la función bastante larga y encontré un fragmento similar. Lo más probable es que la variable
connectCalled tenga que estar aquí en lugar de la variable
connectStarted .
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. Se cometió un error en el destructor de clases debido a la falta de paréntesis 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 la variable
pos se reescribe en el bucle para el mismo valor. Parece que el nuevo diagnóstico ha encontrado 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; } }
La variable
mVRSystem aparece en la condición dos veces
. Obviamente, uno de sus casos debe ser reemplazado 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 encontró la asignación de la 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 , uno tiene que liberar la memoria usando 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
SHGetSpecialFolderPathW WinAPI devuelve el valor del tipo
BOOL , no
HRESULT . Uno tiene que reescribir la verificación del resultado de la función al correcto.
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 ha detectado la asignación de una constante numérica al puntero
out_flags . Lo más probable es que uno haya olvidado desreferenciarlo:
if (fd->secret->alreadyConnected) { *out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; }
Conclusión
Todavía no es el final. ¡Que se realicen nuevas revisiones de código! Código Thunderbird y Firefox que comprende dos bibliotecas grandes: Servicios de seguridad de red (NSS) y WebRTC (Comunicaciones en tiempo real web). Encontré allí algunos errores convincentes. En esta revisión, mostraré uno de cada proyecto.
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. Mientras que la clave DES no se borra aquí. El compilador eliminará la llamada de
memset del código, ya que la matriz
newdeskey ya no se usa en ningún lugar del código.
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 bucle, los datos se escriben en la matriz incorrecta, porque el autor copió el código y olvidó cambiar el nombre de la matriz de
estado para
state_low .
Probablemente, todavía hay errores interesantes en estos proyectos, de los cuales se debe contar. Y lo haremos pronto. Mientras tanto, prueba
PVS-Studio en tu proyecto.