Wireshark Foundation a publié la version stable finale de l'analyseur de trafic réseau populaire - Wireshark 3.0.0. La nouvelle version corrige plusieurs bugs, il est désormais possible d'analyser les nouveaux protocoles, à part que le pilote sur Npcap WinPcap est remplacé. C'est ici que se termine la citation de l'annonce et que commence notre note sur les bogues du projet. Les auteurs des projets n'ont certainement pas fait de leur mieux pour corriger les bogues avant la sortie.
Collectons les correctifs dès maintenant pour donner un motif de faire une nouvelle version :).
Présentation
Wireshark est un outil bien connu pour capturer et analyser le trafic réseau. Le programme fonctionne avec la grande majorité des protocoles connus, possède une interface graphique intuitive et logique, un système de filtres tout-puissant. Wireshark est multiplateforme, fonctionne dans des systèmes d'exploitation tels que: Windows, Linux, macOS, Solaris, FreeBSD, NetBSD et bien d'autres.
Pour faire l'analyse du code source, nous avons utilisé l'analyseur de code statique
PVS-Studio . Pour analyser le code source, nous avons d'abord dû compiler le projet dans un OS. Le choix était large non seulement en raison de la nature multiplateforme du projet, mais aussi en raison de celle de l'analyseur. J'ai choisi macOS pour l'analyse. Vous pouvez également exécuter l'analyseur sous Windows et Linux.
Je voudrais attirer une attention particulière sur la qualité du code. Malheureusement, je ne peux pas lui donner de gros points. C'est une évaluation subjective, mais comme nous vérifions régulièrement de nombreux projets, j'ai un cadre de référence. Ce qui ressort dans ce cas, c'est un grand nombre d'avertissements PVS-Studio pour une petite quantité de code. Au total, plus de 3500 avertissements de tous niveaux déclenchés pour ce projet. C'est typique des projets, qui n'utilisent généralement pas d'outils d'analyse statique, même gratuits. Un autre facteur indiquant la qualité du projet est les erreurs répétées détectées par l'analyseur. Je ne citerai pas d'exemples de code de même type, alors que des erreurs similaires se produisent dans des centaines d'endroits.
Ces insertions ne donnent pas non plus un coup de pouce à la qualité du code:
#line 1 "./asn1/acse/packet-acse-template.c"
Il y en a plus de 1000 dans l'ensemble du projet. De telles insertions rendent plus difficile pour l'analyseur de faire correspondre les avertissements émis avec les fichiers appropriés. Eh bien, je pense que les développeurs moyens ne profiteront pas du maintien d'un tel code.
Typos
Avertissement 1V641 La taille de la mémoire tampon allouée n'est pas un multiple de la taille de l'élément. 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)); .... }
Il existe des structures de deux types:
mate_cfg_gog et
mate_cfg_gop, elles sont très similaires, mais pas égales. Très probablement, dans ce fragment de code, les fonctions sont mélangées, ce qui est lourd d'erreurs potentielles dans le programme lors de l'accès à la mémoire par un pointeur.
Voici les fragments de structures de données mélangées:
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;
Avertissement 2V519 La variable 'HDR_TCP.dest_port' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 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); .... }
Dans la dernière ligne, la valeur (qui vient d'être évaluée) de la variable
HDR_TCP.dest_port est réécrite.
Erreurs logiques
Dans cette section, je citerai plusieurs exemples d'erreurs dans les opérateurs conditionnels, et tous seront complètement différents les uns des autres.
Avertissement 1V547 L'expression 'direction == 0' est toujours fausse. 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;
Dans la condition externe, la variable de
direction est comparée à la constante
P2P_DIR_RECV. Selon les expressions écrites avec l'opérateur AND, en arrivant à la condition intérieure, la valeur de la
direction variable sera certainement différente d'une autre constante
P2P_DIR_SENT .
Avertissement 2V590 Envisagez d'inspecter le '(type == 0x1) || (tapez! = 0x4) '. L'expression est excessive ou contient une erreur d'impression. packet-fcsb3.c 686
static int dissect_fc_sbccs (....) { .... else if ((type == FC_SBCCS_IU_CMD_HDR) || (type != FC_SBCCS_IU_CMD_DATA)) { .... }
L'erreur de ce fragment de code est que le résultat de la condition ne dépend que d'une seule expression:
(type != FC_SBCCS_IU_CMD_DATA)
Avertissement 3V590 Envisagez d'inspecter cette expression. L'expression est excessive ou contient une erreur d'impression. 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; }
Le résultat de l'opérateur conditionnel ne dépendra que de cette partie de l'expression
(source [offset] == '') . La vérification
(source [offset]! = '\ 0') est redondante et peut être supprimée en toute sécurité. Ce n'est pas l'erreur réelle, mais le code redondant rend la lecture et la compréhension du programme plus difficiles, il est donc préférable de le simplifier.
Avertissement 4V547 L'expression 'eras_pos! = NULL' est toujours vraie. 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]); } } .... }
Peut-être, nous avons affaire à un contrôle redondant, probablement avec une faute de frappe, et une autre chose doit être vérifiée dans l'une des conditions du bloc
if .
Affirmations étranges
Avertissement 1V547 L'expression 'sub_dissectors! = NULL' est toujours vraie. 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);
La vérification du pointeur
g_assert est redondante ici, car le pointeur a déjà été vérifié auparavant. Peut-être que seul
g_assert était dans cette fonction et qu'un développeur a oublié de la supprimer, mais peut-être qu'un champ de structure aurait dû être vérifié ici.
Avertissement 2V547 L'expression 'i <count' est toujours vraie. packet-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);
On ne sait pas très bien pourquoi
assert , qui duplique la condition de la boucle, a lieu dans la fonction. Le compteur de boucles ne changera pas dans le corps.
Erreurs avec des pointeurs
Avertissement 1V595 Le pointeur 'si-> conv' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 2135, 2144. packet-smb2.c 2135
static int dissect_smb2_fid(....) { .... g_hash_table_insert(si->conv->fids, sfi, sfi);
Le pointeur
si-> conv est déréférencé quelques lignes avant sa vérification de null.
Avertissement 2V774 Le pointeur «protos» a été utilisé après la libération de la mémoire. packet-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 est un tableau de chaînes. Lors de la gestion d'un cas spécial dans le programme, ce tableau est d'abord effacé par la fonction
g_strfreev , puis une chaîne de ce tableau est utilisée dans le message d'erreur. Très probablement, ces lignes devraient être échangées:
*err = g_strdup_printf("Could not find dissector for: '%s'", protos[i]); g_strfreev(protos);
Fuites de mémoire
V773 Le pointeur 'ptmpstr' a reçu des valeurs deux fois sans libérer la mémoire. Une fuite de mémoire est possible. 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); } .... }
Après la fonction
g_strdup , nous devons appeler la fonction
g_free à un moment donné. Cela n'est pas fait dans l'extrait de code donné et une nouvelle partie de la mémoire est allouée dans la boucle à chaque itération. Voici venir plusieurs fuites de mémoire.
Quelques autres avertissements pour des fragments de code similaires:
- V773 Le pointeur 'ptmpstr' a reçu des valeurs deux fois sans libérer la mémoire. Une fuite de mémoire est possible. idl2wrs.c 2447
- V773 Le pointeur 'ptmpstr' a reçu des valeurs deux fois sans libérer la mémoire. Une fuite de mémoire est possible. idl2wrs.c 2713
- V773 Le pointeur 'ptmpstr' a reçu des valeurs deux fois sans libérer la mémoire. Une fuite de mémoire est possible. idl2wrs.c 2728
- V773 Le pointeur 'ptmpstr' a reçu des valeurs deux fois sans libérer la mémoire. Une fuite de mémoire est possible. idl2wrs.c 2732
- V773 Le pointeur 'ptmpstr' a reçu des valeurs deux fois sans libérer la mémoire. Une fuite de mémoire est possible. idl2wrs.c 2745
Malheureusement, dans le code, il existe de nombreux autres cas similaires, où la mémoire est libérée.
Divers
Avertissement 1V535 La variable 'i' est utilisée pour cette boucle et pour la boucle externe. Vérifiez les lignes: 7716, 7798. packet-opa-mad.c 7798
static gint parse_GetVFInfo(....) { .... for (i = 0; i < records; i++) {
Dans une fonction très longue, les développeurs modifient audacieusement la valeur du compteur de boucles, même en le faisant plusieurs fois. Nous ne pouvons pas dire avec certitude s'il s'agit d'une erreur ou non, cependant, il y a environ 10 boucles de ce type dans le projet.
Avertissement 2V763 Le paramètre 'item' est toujours réécrit dans le corps de la fonction avant d'être utilisé. 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, ....);
Le pointeur d'
élément , pris par la fonction, est immédiatement modifié avec une autre valeur. C'est très suspect. De plus, le code contient plusieurs dizaines de ces emplacements, il est donc difficile de décider s'il s'agit d'une erreur ou non. Je suis tombé sur un code similaire dans un autre grand projet, ce code était correct là-bas, personne n'a simplement osé changer l'interface de la fonction.
Avertissement 3V762 Il est possible qu'une fonction virtuelle n'ait pas été remplacée correctement. Voir le troisième argument de la fonction 'headerData' dans la classe dérivée 'PacketListModel' et la classe de base 'QAbstractItemModel'. packet_list_model.h 48
QVariant QAbstractItemModel::headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const
L'analyseur a détecté une surcharge non valide de la fonction
headerData . Les fonctions ont différentes valeurs par défaut du paramètre de
rôle . Cela peut provoquer le mauvais comportement, pas celui attendu par un programmeur.
Avertissement 4V610 Comportement indéfini. Vérifiez l'opérateur de décalage '>>'. L'opérande droit ('bitshift' = [0..64]) est supérieur ou égal à la longueur en bits de l'opérande gauche promu. 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; .... }
Un décalage de 64 bits entraînera un comportement indéfini selon la norme de langue.
Très probablement, le code correct devrait être comme ceci:
if (bitshift == 64) available_bits = 0; else available_bits = G_GUINT64_CONSTANT(0xFFFFFFFFFFFFFFFF) >> bitshift;
Conclusion
Il peut sembler que cet examen montre peu d'erreurs, mais dans le rapport complet, les cas considérés se répètent des dizaines et des centaines de fois. De plus, les révisions des avertissements PVS-Studio sont de nature démonstrative. Ils représentent une contribution à la qualité des projets open source, mais les contrôles ponctuels sont les plus inefficaces en termes de méthodologie d'analyse statique.
Vous pouvez obtenir et analyser le rapport complet vous-même. Pour ce faire, il vous suffit de
télécharger et d'exécuter l'analyseur PVS-Studio.