As aventuras com o cliente de email Mozilla Thunderbird começaram com a atualização automática para a versão 68.0. Mais texto em notificações pop-up e tema escuro padrão são os recursos notáveis desta versão. Ocasionalmente, encontrava um erro que imediatamente ansiava por detectar com a análise estática. Esse foi o motivo para outra verificação do código-fonte do projeto usando o PVS-Studio. Aconteceu que, no momento da análise, o bug já havia sido corrigido. No entanto, como prestamos atenção ao projeto, não há razão para não escrever sobre outros defeitos encontrados.
1. Introdução
O tema sombrio da nova versão do Thunderbird parece bonito. Eu gosto de temas sombrios. Eu já mudei para eles em mensageiros, Windows, macOS. Em breve, o iPhone será atualizado para o iOS 13 com um tema sombrio. Por esse motivo, tive que trocar meu iPhone 5S por um modelo mais recente. Na prática, descobriu-se que um tema sombrio exige mais esforço para que os desenvolvedores percebam as cores da interface. Nem todo mundo consegue lidar com isso pela primeira vez.
Esta é a aparência das tags padrão no Thunderbird após a atualização:
Normalmente, uso 6 tags (5 padrão +1 personalizadas) para marcar emails. Metade deles ficou impossível de olhar após a atualização, então decidi mudar a cor das configurações para uma mais brilhante. Neste ponto, fiquei preso com um bug:
Você não pode alterar a cor de uma tag !!! Mais sinceramente, você pode, mas o editor não permitirá que você o salve, referindo-se a um nome já existente (WTF ???).
Outro sintoma desse bug é um botão OK inativo. Como não pude fazer alterações no mesmo nome, tentei mudar o nome. Bem, acontece que você também não pode renomeá-lo.
Finalmente, você deve ter notado que o tema sombrio não funcionou nas configurações, o que também não é muito bom.
Após uma longa luta com o sistema de compilação no Windows, acabei criando o Thunderbird a partir dos arquivos de origem. A versão mais recente do cliente de email acabou sendo muito melhor que a nova versão. Nele, o tema escuro também chegou às configurações e esse bug com o editor de tags desapareceu. No entanto, para garantir que a construção do projeto não seja apenas uma perda de tempo, o analisador de código estático do
PVS-Studio começou a funcionar.
Nota O código fonte do Thunderbird cruza com a base de código do Firefox de alguma maneira. Portanto, a análise inclui erros de diferentes componentes, que valem uma olhada mais de perto pelos desenvolvedores dessas equipes.
Nota 2. Enquanto eu escrevia o artigo, o Thunderbird 68.1 foi lançado e esse bug foi corrigido:
comm
comm-central é um repositório Mercurial do código de extensão Thunderbird, SeaMonkey e Lightning.
V501 Existem
subexpressões idênticas '(! Strcmp (cabeçalho, "Responder para"))' à esquerda e à direita do '||' 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; .... }
A cadeia de
cabeçalho foi comparada com a constante
HEADER_REPLY_TO duas vezes. Talvez devesse ter havido outra constante em seu lugar.
V501 Existem
subexpressões idênticas 'obj-> options-> headers! = MimeHeadersCitation' à esquerda e à direita do 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) { .... } .... }
Outra comparação estranha de uma variável com um nome semelhante -
cabeçalhos . Como sempre, existem duas explicações possíveis: uma verificação desnecessária ou um erro de digitação.
V517 O uso do
padrão 'if (A) {...} else if (A) {...}' foi detectado. Há uma probabilidade de presença de erro lógico. Verifique as linhas: 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)) {
As teclas Ctrl + C e Ctrl + V definitivamente ajudaram a acelerar a gravação dessa cascata de expressões condicionais. Como resultado, uma das ramificações nunca será executada.
V517 O uso do
padrão 'if (A) {...} else if (A) {...}' foi detectado. Há uma probabilidade de presença de erro lógico. Verifique as linhas: 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) {
A primeira e a última condição são as mesmas. O código mostra que ainda está em processo de escrita. Pode-se dizer com segurança que o erro será exibido após o código ser refinado. Um programador pode alterar o código comentado, mas nunca terá controle sobre ele. Por favor, tenha muito cuidado e atenção com este código.
V522 A desreferenciação do ponteiro nulo 'linha' pode ocorrer. morkRowCellCursor.cpp 175
NS_IMETHODIMP morkRowCellCursor::MakeCell(
Possível desreferência do ponteiro nulo da
linha na seguinte linha:
morkCell* cell = row->CellAt(ev, pos);
Provavelmente, um ponteiro não foi inicializado, por exemplo, pelo método
GetRow , etc.
V543 É estranho que o valor '-1' seja designado à variável 'm_lastError' do 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; } .... }
O tipo
HRESULT é um tipo de dados complexo. Seus diferentes bits representam diferentes campos de uma descrição de erro. Você precisa definir o código de erro usando constantes especiais dos arquivos de cabeçalho do sistema.
Alguns fragmentos como este:
- V543 É estranho que o valor '-1' seja designado à variável 'm_lastError' do tipo HRESULT. MapiApi.cpp 817
- V543 É estranho que o valor '-1' seja designado à variável 'm_lastError' do tipo HRESULT. MapiApi.cpp 1749
V579 A função memset recebe o ponteiro e seu tamanho como argumentos. É possivelmente um erro. Inspecione o terceiro 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 ); .... }
A variável
parts é um ponteiro para uma matriz de estruturas. Para redefinir os valores das estruturas, os autores usaram a função
memset , mas passaram o tamanho do ponteiro como o tamanho do espaço da memória.
Fragmentos suspeitos semelhantes:
- V579 A função memset recebe o ponteiro e seu tamanho como argumentos. É possivelmente um erro. Inspecione o terceiro argumento. icalmime.c 385
- V579 A função memset recebe o ponteiro e seu tamanho como argumentos. É possivelmente um erro. Inspecione o terceiro argumento. icalparameter.c 114
- V579 A função snprintf recebe o ponteiro e seu tamanho como argumentos. É possivelmente um erro. Inspecione o segundo argumento. icaltimezone.c 1908
- V579 A função snprintf recebe o ponteiro e seu tamanho como argumentos. É possivelmente um erro. Inspecione o segundo argumento. icaltimezone.c 1910
- V579 A função strncmp recebe o ponteiro e seu tamanho como argumentos. É possivelmente um erro. Inspecione o terceiro argumento. sspm.c 707
- V579 A função strncmp recebe o ponteiro e seu tamanho como argumentos. É possivelmente um erro. Inspecione o terceiro argumento. sspm.c 813
V595 O ponteiro 'aValues' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 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; } .... }
O diagnóstico
V595 geralmente detecta erros típicos de desreferência de ponteiro nulo. Nesse caso, temos um exemplo extremamente interessante, digno de atenção especial.
Tecnicamente, o analisador está certo de que o ponteiro
aValues é primeiro desreferenciado e depois verificado, mas o erro real é diferente. É um ponteiro duplo, portanto, o código correto deve ter a seguinte aparência:
*aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!*aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; }
Outro fragmento semelhante:
- V595 O ponteiro '_retval' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 357, 358. nsLDAPSyncQuery.cpp 357
V1044 As condições de interrupção do loop não dependem do número de iterações. 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 erro foi encontrado usando um novo diagnóstico que estará disponível na próxima versão do analisador. Todas as variáveis usadas na condição do loop while não mudam, pois as variáveis
ptr2 e
cSet são confusas no corpo da função.
netwerk
O netwerk contém interfaces e código C para acesso de baixo nível à rede (usando soquetes e caches de arquivos e memória), bem como acesso de nível superior (usando vários protocolos como http, ftp, gopher, castanet). Esse código também é conhecido pelos nomes "netlib" e "Necko".
V501 Existem
subexpressões idênticas 'connectStarted' à esquerda e à direita do operador '&&'. nsSocketTransport2.cpp 1693
nsresult nsSocketTransport::InitiateSocket() { .... if (gSocketTransportService->IsTelemetryEnabledAndNotSleepPhase() && connectStarted && connectCalled) {
Primeiro, pensei que duplicar a variável
connectStarted é apenas um código redundante. Mas então examinei toda a função bastante longa e encontrei um fragmento semelhante. Provavelmente, a variável
connectCalled precisa estar aqui em vez da variável
connectStarted .
V611 A memória foi alocada usando o operador 'new T []', mas foi liberada usando o operador 'delete'. Considere inspecionar este código. Provavelmente é melhor usar 'delete [] mData;'. Verifique as linhas: 233, 222. DataChannel.cpp 233
BufferedOutgoingMsg::BufferedOutgoingMsg(OutgoingMsg& msg) { size_t length = msg.GetLeft(); auto* tmp = new uint8_t[length];
O ponteiro
mData aponta para uma matriz, não para um único objeto. Um erro foi cometido no destruidor de classe devido à falta de colchetes para o operador de
exclusão .
V1044 As condições de interrupção do loop não dependem do número de iterações. 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'; .... }
O valor da variável
pos é reescrito no loop para o mesmo valor. Parece que o novo diagnóstico encontrou outro erro.
gfx
O gfx contém interfaces C e código para desenho e imagem independentes da plataforma. Pode ser usado para desenhar retângulos, linhas, imagens, etc. Essencialmente, é um conjunto de interfaces para um contexto de dispositivo independente de plataforma (desenho). Ele não suporta widgets ou rotinas de desenho específicas; apenas fornece as operações primitivas para o desenho.
V501 Existem
subexpressões idênticas à esquerda e à direita do '||' 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; } }
A variável
mVRSystem aparece na condição duas vezes
. Obviamente, uma de suas ocorrências deve ser substituída por
mVRChaperone.dom
dom contém interfaces e código C para implementar e rastrear objetos DOM (Document Object Model) em Javascript. Ele forma a subestrutura C que cria, destrói e manipula objetos internos e definidos pelo usuário de acordo com o script Javascript.
V570 A variável 'clonedDoc-> mPreloadReferrerInfo' é atribuída a si mesma. Document.cpp 12049
already_AddRefed<Document> Document::CreateStaticClone( nsIDocShell* aCloneContainer) { .... clonedDoc->mReferrerInfo = static_cast<dom::ReferrerInfo*>(mReferrerInfo.get())->Clone(); clonedDoc->mPreloadReferrerInfo = clonedDoc->mPreloadReferrerInfo; .... }
O analisador encontrou a atribuição da variável a si mesma.
xpcom
O xpcom contém as interfaces C de baixo nível, código C, código C, um pouco de código de montagem e ferramentas de linha de comando para implementar o mecanismo básico dos componentes XPCOM (que significa "Modelo de objeto de componente de plataforma cruzada"). XPCOM é o mecanismo que permite ao Mozilla exportar interfaces e disponibilizá-las automaticamente para scripts JavaScript, Microsoft COM e código Mozilla C regular.
V611 A memória foi alocada usando a função 'malloc / realloc', mas foi liberada usando o operador 'delete'. Considere inspecionar as lógicas de operação por trás da variável 'key'. Verifique as linhas: 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; };
Depois de chamar a função
strdup , é necessário liberar a memória usando a função
livre , não o operador de
exclusão .
V716 Conversão de tipo suspeita na inicialização: '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; } .... }
A função
SHGetSpecialFolderPathW WinAPI retorna o valor do tipo
BOOL , não
HRESULT . É preciso reescrever a verificação do resultado da função para a correta.
nsprpub
O nsprpub contém código C para a biblioteca de tempo de execução "C" de plataforma cruzada. A biblioteca de tempo de execução “C” contém funções C não visuais básicas para alocar e desalocar memória, obter hora e data, ler e gravar arquivos, manipular threads e manipular e comparar strings em todas as plataformas
V647 O valor do tipo 'int' é atribuído ao ponteiro do tipo 'curto'. Considere inspecionar a atribuição: '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; }
O analisador detectou a atribuição de uma constante numérica ao ponteiro
out_flags . Provavelmente, esquecemos de desreferê-lo:
if (fd->secret->alreadyConnected) { *out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; }
Conclusão
Ainda não é o fim. Que novas revisões de código sejam! O código Thunderbird e Firefox inclui duas grandes bibliotecas: Network Security Services (NSS) e WebRTC (Web Real Time Communications). Eu encontrei alguns erros convincentes. Nesta revisão, mostrarei um de cada projeto.
NssV597 O compilador pode excluir a chamada de função 'memset', usada para liberar o buffer 'newdeskey'. A função RtlSecureZeroMemory () deve ser usada para apagar os dados particulares. 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); .... }
O NSS é uma biblioteca para o desenvolvimento de aplicativos seguros para clientes e servidores. Enquanto a chave DES não está sendo limpa aqui. O compilador excluirá a chamada
memset do código, pois a matriz
newdeskey não é usada em nenhum outro local do código.
WebRTCV519 A
variável 'state [state_length - x_length + i]' recebe valores duas vezes sucessivos. Talvez isso seja um erro. Verifique as linhas: 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];
No segundo loop, os dados são gravados na matriz incorreta, porque o autor copiou o código e esqueceu de alterar o nome da matriz de
estado para
state_low .
Provavelmente, ainda existem bugs interessantes nesses projetos, sobre os quais devemos falar. E faremos isso em breve. Enquanto isso, tente o
PVS-Studio em seu projeto.