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 corrigiu vários erros, implementou a capacidade de analisar novos protocolos e substituiu o driver WinPcap pelo Npcap. Aqui a citação do anúncio termina e nosso artigo sobre erros no projeto começa. Antes do lançamento, eles eram claramente corrigidos insuficientemente. Vamos fazer algumas correções para que haja uma razão para fazer um novo lançamento :).
1. Introdução
O Wireshark é uma ferramenta bem conhecida para capturar e analisar o tráfego de rede. O programa trabalha com a grande maioria dos protocolos conhecidos, possui uma interface gráfica clara e lógica, um poderoso sistema de filtro. Wireshark - multiplataforma, funciona em sistemas operacionais como: Windows, Linux, macOS, Solaris, FreeBSD, NetBSD e muitos outros.
Para procurar erros, usamos o analisador estático
PVS-Studio . Para analisar o código fonte, você deve primeiro compilar o projeto em algum sistema operacional. A escolha foi ótima, não apenas pela natureza de plataforma cruzada do projeto, mas também pela natureza de plataforma cruzada do analisador. Para a análise do projeto, escolhi o macOS. O analisador também pode ser iniciado no Windows e Linux.
Sobre a qualidade do código que quero contar separadamente. Infelizmente, não posso chamá-lo de bom. Esta é uma avaliação subjetiva, mas, como
verificamos regularmente muitos projetos, tenho algo com o que comparar. Nesse caso, um grande número de avisos do PVS-Studio em uma pequena quantidade de código é impressionante. No total, mais de 3.500 avisos de todos os níveis foram emitidos para este projeto. Isso é típico para projetos que não usam ferramentas de análise estática, mesmo gratuitas. Outro fator que indica a qualidade do projeto são os erros repetidos identificados pelo analisador. Os mesmos exemplos de código não serão fornecidos no artigo, mas alguns erros idênticos estão presentes no código em centenas de lugares.
Além disso, essas inserções não adicionam qualidade ao código:
#line 1 "./asn1/acse/packet-acse-template.c"
Existem mais de 1000 deles em todo o projeto. Tais inserções dificultam para o analisador comparar os avisos gerados com o arquivo desejado. Mas tenho certeza de que desenvolvedores comuns não desfrutam do suporte a 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 dois tipos de estruturas:
mate_cfg_gog e
mate_cfg_gop , eles são muito semelhantes, mas não idênticos. Provavelmente, as funções foram misturadas nesse fragmento de código, repleto de erros em potencial no programa durante o acesso à memória pelo ponteiro.
A seguir, trechos de estruturas de dados confusas:
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 apenas calculado da variável
HDR_TCP.dest_port não é
confirmado .
Erros lógicos
Nesta seção, darei alguns exemplos de erros em declarações condicionais e todos eles serão fundamentalmente 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 . Escrever expressões através do operador AND significa que, quando a condição interna for atingida, o valor da variável de
direção não será exatamente igual à 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 trecho de código é que o resultado da condição depende de apenas 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 da instrução condicional dependerá apenas dessa parte da expressão
(origem [deslocamento] == '') . A verificação
(origem [deslocamento]! = '\ 0') é redundante e pode ser removida com segurança. Este não é um 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 verificações desnecessárias e, possivelmente, com erros de digitação. Em um dos ifs, algo mais deve ser verificado.
Ativos estranhos
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 em
g_assert neste momento é supérflua, pois o ponteiro é verificado antes disso. Talvez nesta função houvesse apenas
g_assert antes , e eles se esqueceram de excluí-la, mas talvez aqui você deva verificar algum campo de estrutura.
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á totalmente claro por que uma
declaração está presente na função, que duplica a condição do loop. O contador de ciclos no corpo não muda.
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 várias linhas antes que seja verificado se é igual a zero ou não.
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. Durante o processamento de uma exceção no programa, essa matriz é limpa primeiro pela função
g_strfreev e, em seguida, uma das linhas dessa matriz é usada na mensagem de erro. Provavelmente, essas linhas no código 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 ,
você precisa chamar a função
g_free em algum momento. No fragmento de código apresentado, isso não é feito e, no loop a cada iteração, uma nova parte da RAM é alocada. Vários vazamentos de memória ocorrem.
Mais alguns avisos para trechos 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, existem muitos outros lugares semelhantes no código em que a memória não é 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 ousadamente o valor do contador de loop e fazem isso várias vezes. É difícil dizer se isso é um erro ou não, mas existem cerca de 10 desses ciclos 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 que a função utiliza é imediatamente desfiado por outro valor. Isso é muito suspeito. Além disso, o código contém várias dezenas de lugares, por isso é difícil dizer se isso é um erro ou não. Eu conheci um código semelhante anteriormente em outro projeto grande, lá estava o código correto, apenas ninguém 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 uma sobrecarga incorreta da função
headerData . As funções têm um valor padrão diferente para o parâmetro
role . Isso pode não levar ao comportamento esperado pelo 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 de idioma.
Em vez disso, o código correto deve ser assim:
if (bitshift == 64) available_bits = 0; else available_bits = G_GUINT64_CONSTANT(0xFFFFFFFFFFFFFFFF) >> bitshift;
Conclusão
Pode parecer que há poucos exemplos de erros na revisão, mas no relatório completo os casos apresentados são repetidos dezenas e centenas de vezes. As visões gerais de aviso do PVS-Studio são apenas para fins de demonstração. Isso é uma contribuição para a qualidade dos projetos de código aberto, mas as verificações únicas são a maneira mais ineficiente de aplicar a 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.
Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Svyatoslav Razmyslov.
Wireshark 3.x: análise de código no macOS e revisão de erros