A “aventura” com o cliente de email Mozilla Thunderbird começou com uma atualização automática para a versão 68.0. Os recursos mais notáveis desta versão foram: mais texto é adicionado às notificações pop-up e um tema sombrio por padrão. Houve um erro que eu queria tentar detectar usando a análise estática. Foi uma ocasião para verificar novamente o código fonte do projeto usando o PVS-Studio. Aconteceu que, no momento da análise, o erro já havia sido corrigido. Mas, como chamamos a atenção para este projeto, podemos escrever sobre outros defeitos encontrados nele.
1. Introdução
O tema sombrio da nova versão do Thunderbird parece bastante bonito. Eu amo temas sombrios. Já mudou para eles em mensageiros instantâneos, Windows, macOS. Em breve, o iPhone será atualizado para o iOS 13, onde um tema sombrio apareceu. Para isso, tive que mudar meu iPhone 5S para um modelo mais novo. Na prática, o tema sombrio exige mais esforço dos desenvolvedores para escolher as cores da interface. Nem todo mundo lida com isso pela primeira vez.
Então, minhas tags padrão no Thunderbird começaram a se parecer com:
Uso seis tags (5 padrão + 1 personalizado) para marcar e-mails. Tornou-se impossível assistir metade deles após a atualização, e decidi mudar a cor para mais brilhante nas configurações. Mas aqui encontrei um bug:
Você não pode mudar a cor da etiqueta !!! Mais precisamente, é possível, mas o editor não permitirá que ele seja salvo, referindo-se a um nome existente (WTF ???).
Outra manifestação do bug será a inação do botão OK, se você tentar alterar o nome, pois não poderá salvar esse nome. Você também não pode renomear.
Finalmente, você pode perceber que o tema sombrio não tocou nas configurações, o que também não é muito bonito.
Após uma longa luta com o sistema de compilação no Windows, ainda era possível montar o Thunderbird a partir da fonte. A versão mais recente do cliente de email acabou sendo muito melhor que a versão mais recente. Nele, um tema sombrio chegou às configurações, e esse bug com o editor de tags também desapareceu. Mas, para que o trabalho de montagem do projeto não fosse desperdiçado, o analisador de código estático
PVS-Studio foi lançado.
Nota O código-fonte do Thunderbird se sobrepõe de alguma forma à base de código do Firefox. Portanto, a análise incluiu erros de diferentes componentes, que valem uma olhada de perto nos desenvolvedores de diferentes equipes.
Nota 2 Enquanto o artigo estava sendo escrito, a atualização do Thunderbird 68.1 foi lançada com uma correção para esse bug:
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 linha do
cabeçalho foi comparada com a constante
HEADER_REPLY_TO duas vezes. Talvez em seu lugar devesse ter havido outra constante.
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 são os
cabeçalhos . Como sempre, existem duas explicações possíveis: um cheque extra 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)) {
A cascata de expressões condicionais foi claramente acelerada pressionando Ctrl + C e Ctrl + V. Como resultado, uma das ramificações nunca é 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 o código está em processo de gravação. É seguro dizer que, com uma alta probabilidade, o erro se manifestará após a finalização do código. Um programador pode alterar o código comentado, mas ele nunca terá controle. Tenha cuidado e cuidado com este código.
V522 A desreferenciação do ponteiro nulo 'linha' pode ocorrer. morkRowCellCursor.cpp 175
NS_IMETHODIMP morkRowCellCursor::MakeCell(
A desreferenciação do ponteiro nulo da
linha é possível na seguinte linha:
morkCell* cell = row->CellAt(ev, pos);
Provavelmente, a inicialização do ponteiro foi ignorada antes desta linha, por exemplo, usando o 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. Diferentes bits de uma variável desse tipo representam diferentes campos de descrição de erro. O código de erro deve ser definido usando constantes especiais dos arquivos de cabeçalho do sistema.
Mais alguns desses lugares:
- 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, eles usaram a função
memset , mas transferiram o tamanho do ponteiro para ele como o tamanho de um pedaço de memória.
Outros locais suspeitos:
- 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 encontra erros típicos de
cancelamento de referência de ponteiro nulo. Mas foi encontrado um caso muito interessante, digno de atenção especial.
Tecnicamente, o analisador está certo de que o ponteiro
aValues é primeiro desreferenciado e depois verificado, mas o erro é diferente. Como é um ponteiro duplo, o código correto deve ficar assim:
*aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!*aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; }
Outro lugar muito 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 para interromper o
loop while não são modificadas, porque as variáveis
ptr2 e
cSet são misturadas 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) {
No começo, pensei que duplicar a variável
connectStarted é apenas um código supérfluo até examinar toda a função suficientemente longa e encontrar um fragmento semelhante. Provavelmente, em vez de uma única variável
connectStarted aqui, também deve ser uma variável
connectCalled .
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. Eles cometeram um erro no destruidor de classes, esquecendo de adicionar colchetes ao 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 de
pos é substituído no loop pela mesma quantidade. 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; } }
Na condição, a variável
mVRSystem está presente duas vezes. Obviamente, um deles deve ser substituído 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 detectou a atribuição de uma 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 memória usando a função
free , não o operador
delete .
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
WinGI SHGetSpecialFolderPathW retorna um valor do tipo
BOOL , não
HRESULT . A verificação do resultado da função deve ser reescrita 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, eles simplesmente esqueceram de desreferê-lo:
if (fd->secret->alreadyConnected) { *out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; }
Conclusão
Este não é o fim. Ser novas revisões de código. O código Thunderbird e Firefox inclui duas grandes bibliotecas: Network Security Services (NSS) e WebRTC (Web Real Time Communications). Houve alguns erros muito interessantes. Nesta revisão, mostrarei uma de cada vez.
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, e aqui a DES Key não é limpa. O compilador removerá a chamada
memset do código, conforme a matriz
newdeskey não
é mais usada no código além desse local.
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 ciclo, os dados são gravados na matriz errada, porque o autor copiou o código e esqueceu de alterar o nome da matriz de
state para
state_low .
Provavelmente existem bugs mais interessantes nesses projetos que vale a pena mencionar. E faremos isso em um futuro próximo. Enquanto isso, tente o
PVS-Studio em seu projeto.
Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Svyatoslav Razmyslov.
Tema sombrio do Thunderbird como um motivo para executar um analisador de código .