A Wireshark Foundation lançou a versão estável final do popular analisador de tráfego de rede - Wireshark 3.0.0. A nova versão corrige vários bugs, agora é possível analisar os novos protocolos, além de que o driver no Npcap WinPcap é substituído. Aqui é onde a citação do anúncio termina e nossa nota sobre erros no projeto começa. Os autores dos projetos definitivamente não fizeram o melhor para corrigir os erros antes do lançamento.
Vamos coletar hotfixes agora mesmo para dar um motivo para fazer um novo lançamento :).
1. Introdução
O Wireshark é uma ferramenta conhecida para capturar e analisar o tráfego de rede. O programa trabalha com a grande maioria dos protocolos conhecidos, possui interface gráfica intuitiva e lógica, um poderoso sistema de filtros. O Wireshark é multiplataforma, funciona em sistemas operacionais como: Windows, Linux, macOS, Solaris, FreeBSD, NetBSD e muitos outros.
Para fazer a análise do código fonte, usamos o analisador de código estático
PVS-Studio . Para analisar o código-fonte, primeiro precisamos compilar o projeto em um sistema operacional. A escolha foi ampla, não apenas devido à natureza de plataforma cruzada do projeto, mas também à do analisador. Eu escolhi o macOS para a análise. Você também pode executar o analisador no Windows e Linux.
Gostaria de chamar atenção especial para a qualidade do código. Infelizmente, não posso dar grandes pontos a isso. É uma avaliação subjetiva, mas, como verificamos regularmente muitos projetos, tenho um quadro de referência. O que se destaca nesse caso é um grande número de avisos do PVS-Studio para uma pequena quantidade de código. No total, mais de 3500 avisos de todos os níveis foram acionados para este projeto. É típico para os projetos, que geralmente não usam ferramentas de análise estática, mesmo as gratuitas. Outro fator que aponta para a qualidade do projeto são os erros repetidos detectados pelo analisador. Não citarei exemplos de código do mesmo tipo, enquanto alguns erros semelhantes ocorrem em centenas de lugares.
Essas inserções também não aumentam a qualidade do código:
#line 1 "./asn1/acse/packet-acse-template.c"
Existem mais de 1000 deles em todo o projeto. Essas inserções dificultam a correspondência do analisador com os arquivos apropriados. Bem, acho que os desenvolvedores comuns não vão se interessar em manter esse código.
Erros de digitação
Aviso 1V641 O tamanho do buffer de memória alocado não é um múltiplo do tamanho do elemento. mate_setup.c 100
extern mate_cfg_gog* new_gogcfg(mate_config* mc, gchar* name) { mate_cfg_gog* cfg = (mate_cfg_gog *)g_malloc(sizeof(mate_cfg_gop)); .... }
Existem estruturas de dois tipos:
mate_cfg_gog e
mate_cfg_gop, elas são muito semelhantes, mas não iguais. Provavelmente, nesse código, as funções de fragmento são misturadas, o que é repleto de erros em potencial no programa ao acessar a memória por um ponteiro.
Aqui estão os fragmentos de estruturas de dados misturadas:
typedef struct _mate_cfg_gog { gchar* name; GHashTable* items; guint last_id; GPtrArray* transforms; LoAL* keys; AVPL* extra; float expiration; gop_tree_mode_t gop_tree_mode; gboolean show_times; .... } mate_cfg_gog; typedef struct _mate_cfg_gop { gchar* name; guint last_id; GHashTable* items; GPtrArray* transforms; gchar* on_pdu; AVPL* key; AVPL* start; AVPL* stop; AVPL* extra; float expiration; float idle_timeout; float lifetime; gboolean drop_unassigned; gop_pdu_tree_t pdu_tree_mode; gboolean show_times; .... } mate_cfg_gop;
Aviso 2V519 A variável 'HDR_TCP.dest_port' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 495, 496. text_import.c 496
void write_current_packet (void) { .... HDR_TCP.source_port =isOutbound ? g_htons(hdr_dest_port):g_htons(hdr_src_port); HDR_TCP.dest_port = isOutbound ? g_htons(hdr_src_port) :g_htons(hdr_dest_port); HDR_TCP.dest_port = g_htons(hdr_dest_port); .... }
Na última linha, o valor (que acaba de ser avaliado) da variável
HDR_TCP.dest_port é reescrito.
Erros lógicos
Nesta seção, citarei vários exemplos de erros em operadores condicionais, e todos eles serão completamente diferentes um do outro.
Aviso 1V547 A expressão 'direction == 0' é sempre falsa. packet-adb.c 291
#define P2P_DIR_RECV 1 #define P2P_DIR_SENT 0 static void save_command(....) { .... if ( service_data && service_data->remote_id == 0 && direction == P2P_DIR_RECV) { if (direction == P2P_DIR_SENT) { service_data->remote_id = arg1;
Na condição externa, a variável de
direção é comparada com a constante
P2P_DIR_RECV. De acordo com as expressões escritas com o operador AND, ao chegar à condição interna, o valor da
direção da variável será definitivamente diferente de outra constante
P2P_DIR_SENT .
Aviso 2V590 Considere inspecionar o '(tipo == 0x1) || (tipo! = 0x4) 'expressão. A expressão é excessiva ou contém uma impressão incorreta. packet-fcsb3.c 686
static int dissect_fc_sbccs (....) { .... else if ((type == FC_SBCCS_IU_CMD_HDR) || (type != FC_SBCCS_IU_CMD_DATA)) { .... }
O erro desse fragmento de código é que o resultado da condição depende apenas de uma expressão:
(type != FC_SBCCS_IU_CMD_DATA)
Aviso 3V590 Considere inspecionar esta expressão. A expressão é excessiva ou contém uma impressão incorreta. snort-config.c 40
static char *skipWhiteSpace(char *source, int *accumulated_offset) { int offset = 0; while (source[offset] != '\0' && source[offset] == ' ') { offset++; } *accumulated_offset += offset; return source + offset; }
O resultado do operador condicional dependerá apenas dessa parte da expressão
(origem [deslocamento] == '') . A verificação
(origem [deslocamento]! = '\ 0') é redundante e pode ser removida com segurança. Não é o erro real, mas o código redundante dificulta a leitura e a compreensão do programa, por isso é melhor simplificá-lo.
Aviso 4A expressão V547 'eras_pos! = NULL' sempre é verdadeira. reedsolomon.c 659
int eras_dec_rs(dtype data[NN], int eras_pos[NN-KK], int no_eras) { .... if(eras_pos != NULL){ for(i=0;i<count;i++){ if(eras_pos!= NULL) eras_pos[i] = INDEX_TO_POS(loc[i]); } } .... }
Talvez estejamos lidando com uma verificação redundante, provavelmente com um erro de digitação, e outra coisa deve ser verificada em uma das condições do bloco
if .
Afirmações estranhas
Aviso 1A expressão V547 'sub_dissectors! = NULL' sempre é verdadeira. capture_dissectors.c 129
void capture_dissector_add_uint(....) { .... sub_dissectors = (struct capture_dissector_table*)g_hash_table_lookup(....); if (sub_dissectors == NULL) { fprintf(stderr, "OOPS: Subdissector \"%s\" not found ... \n", name); if (getenv("WIRESHARK_ABORT_ON_DISSECTOR_BUG") != NULL) abort(); return; } g_assert(sub_dissectors != NULL);
A verificação do ponteiro
g_assert é redundante aqui, pois o ponteiro já foi verificado antes disso. Talvez apenas
g_assert estivesse nessa função e um desenvolvedor esqueceu de removê-la, mas talvez um campo de estrutura devesse ter sido verificado aqui.
Aviso 2A expressão V547 'i <count' sempre é verdadeira. pacote-netflow.c 10363
static int dissect_v9_v10_template_fields(....) { .... count = tmplt_p->field_count[fields_type]; for(i=0; i<count; i++) { .... if (tmplt_p->fields_p[fields_type] != NULL) { DISSECTOR_ASSERT (i < count);
Não está claro por que a
afirmação , que duplica a condição do loop, ocorre na função. O contador de loop não muda no corpo.
Erros com ponteiros
Aviso 1V595 O ponteiro 'si-> conv' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 2135, 2144. packet-smb2.c 2135
static int dissect_smb2_fid(....) { .... g_hash_table_insert(si->conv->fids, sfi, sfi);
O ponteiro
si-> conv é desreferenciado algumas linhas antes da verificação de nulo.
Aviso 2V774 O ponteiro 'protos' foi usado após o lançamento da memória. pacote-k12.c 311
static gboolean k12_update_cb(void* r, char** err) { gchar** protos; .... for (i = 0; i < num_protos; i++) { if ( ! (h->handles[i] = find_dissector(protos[i])) ) { h->handles[i] = data_handle; h->handles[i+1] = NULL; g_strfreev(protos); *err = g_strdup_printf("Could not find dissector for: '%s'", protos[i]); return FALSE; } } .... }
protos é uma variedade de strings. Ao manipular um caso especial no programa, essa matriz é limpa primeiro pela função
g_strfreev e, em seguida, uma sequência dessa matriz é usada na mensagem de erro. Muito provavelmente, essas linhas devem ser trocadas:
*err = g_strdup_printf("Could not find dissector for: '%s'", protos[i]); g_strfreev(protos);
Vazamentos de memória
V773 O ponteiro 'ptmpstr' recebeu valores duas vezes sem liberar a memória. É possível um vazamento de memória. idl2wrs.c 2436
static void parsetypedefunion(int pass) { char tmpstr[BASE_BUFFER_SIZE], *ptmpstr; .... while(num_pointers--){ g_snprintf(tmpstr, BASE_BUFFER_SIZE, "%s_%s", ptmpstr, "unique"); FPRINTF(eth_code, "static int\n"); FPRINTF(eth_code, "....", tmpstr); FPRINTF(eth_code, "{\n"); FPRINTF(eth_code, " ....", ptmpstr, ti->str); FPRINTF(eth_code, " return offset;\n"); FPRINTF(eth_code, "}\n"); FPRINTF(eth_code, "\n"); ptmpstr=g_strdup(tmpstr); } .... }
Após a função
g_strdup , precisamos chamar a função
g_free em algum momento. Isso não é feito no trecho de código fornecido e uma nova parte da memória é alocada no loop em cada iteração. Aí vêm vários vazamentos de memória.
Alguns outros avisos para fragmentos de código semelhantes:
- V773 O ponteiro 'ptmpstr' recebeu valores duas vezes sem liberar a memória. É possível um vazamento de memória. idl2wrs.c 2447
- V773 O ponteiro 'ptmpstr' recebeu valores duas vezes sem liberar a memória. É possível um vazamento de memória. idl2wrs.c 2713
- V773 O ponteiro 'ptmpstr' recebeu valores duas vezes sem liberar a memória. É possível um vazamento de memória. idl2wrs.c 2728
- V773 O ponteiro 'ptmpstr' recebeu valores duas vezes sem liberar a memória. É possível um vazamento de memória. idl2wrs.c 2732
- V773 O ponteiro 'ptmpstr' recebeu valores duas vezes sem liberar a memória. É possível um vazamento de memória. idl2wrs.c 2745
Infelizmente, no código, existem muitos outros casos semelhantes, nos quais a memória é liberada.
Diversos
Aviso 1V535 A variável 'i' está sendo usada para esse loop e para o loop externo. Verifique as linhas: 7716, 7798. packet-opa-mad.c 7798
static gint parse_GetVFInfo(....) { .... for (i = 0; i < records; i++) {
Em uma função muito longa, os desenvolvedores alteram com ousadia o valor do contador de loop, mesmo fazendo isso algumas vezes. Não podemos dizer com certeza se é um erro ou não; no entanto, existem cerca de 10 desses loops no projeto.
Aviso 2V763 O parâmetro 'item' é sempre reescrito no corpo da função antes de ser usado. packet-cdma2k.c 1324
static void cdma2k_message_ORDER_IND(proto_item *item, ....) { guint16 addRecLen = -1, ordq = -1, rejectedtype = -1; guint16 l_offset = -1, rsc_mode_ind = -1, ordertype = -1; proto_tree *subtree = NULL, *subtree1 = NULL; item = proto_tree_add_item(tree,hf_cdma2k_OrderIndMsg, tvb, ....);
O ponteiro do
item , obtido pela função, é imediatamente alterado com outro valor. É muito suspeito. Além disso, o código contém várias dezenas de lugares, por isso é difícil decidir se é um erro ou não. Me deparei com código semelhante em outro projeto grande, esse código estava correto lá, ninguém simplesmente se atreveu a mudar a interface da função.
Aviso 3V762 É possível que uma função virtual tenha sido substituída incorretamente. Consulte o terceiro argumento da função 'headerData' na classe derivada 'PacketListModel' e na classe base 'QAbstractItemModel'. packet_list_model.h 48
QVariant QAbstractItemModel::headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const
O analisador detectou a sobrecarga inválida da função
headerData . As funções têm diferentes valores padrão do parâmetro
role . Isso pode causar o comportamento errado, não o esperado por um programador.
Aviso 4V610 Comportamento indefinido. Verifique o operador de turno '>>'. O operando direito ('deslocamento de bits' = [0..64]) é maior ou igual ao comprimento em bits do operando esquerdo promovido. proto.c 10941
static gboolean proto_item_add_bitmask_tree(...., const int len, ....) { .... if (len < 0 || len > 8) g_assert_not_reached(); bitshift = (8 - (guint)len)*8; available_bits = G_GUINT64_CONSTANT(0xFFFFFFFFFFFFFFFF) >> bitshift; .... }
Uma mudança de 64 bits resultará em um comportamento indefinido de acordo com o padrão do idioma.
Provavelmente, o código correto deve ser assim:
if (bitshift == 64) available_bits = 0; else available_bits = G_GUINT64_CONSTANT(0xFFFFFFFFFFFFFFFF) >> bitshift;
Conclusão
Parece que esta revisão mostra poucos erros, mas no relatório completo os casos considerados se repetem dezenas e centenas de vezes. Além disso, as análises de avisos do PVS-Studio são de natureza demonstrativa. Eles representam uma contribuição para a qualidade dos projetos de código aberto, mas as verificações únicas são as mais ineficientes em termos de metodologia de análise estática.
Você pode obter e analisar o relatório completo por conta própria. Para fazer isso, basta
baixar e executar o analisador PVS-Studio.