La Wireshark Foundation a publié la dernière version stable de l'analyseur de trafic réseau populaire - Wireshark 3.0.0. La nouvelle version a corrigé plusieurs bogues, implémenté la possibilité d'analyser de nouveaux protocoles et remplacé le pilote WinPcap par Npcap. Ici, la citation de l'annonce se termine et notre article sur les bogues dans le projet commence. Avant la sortie, ils ont été clairement corrigés de manière insuffisante. Obtenons quelques correctifs pour qu'il y ait une raison 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 claire et logique, un puissant système de filtrage. Wireshark - multiplateforme, fonctionne dans les systèmes d'exploitation tels que: Windows, Linux, macOS, Solaris, FreeBSD, NetBSD et bien d'autres.
Pour rechercher des erreurs, nous avons utilisé l'analyseur statique
PVS-Studio . Pour analyser le code source, vous devez d'abord compiler le projet dans un système d'exploitation. Le choix était grand non seulement en raison de la nature multiplateforme du projet, mais également en raison de la nature multiplateforme de l'analyseur. Pour l'analyse du projet, j'ai choisi macOS. L'analyseur peut également être lancé sur Windows et Linux.
Je voudrais parler de la qualité du code séparément. Malheureusement, je ne peux pas appeler ça bien. Il s'agit d'une évaluation subjective, mais comme nous
vérifions régulièrement de nombreux projets, j'ai quelque chose à comparer. Dans ce cas, un grand nombre d'avertissements PVS-Studio sur une petite quantité de code sont frappants. Au total, plus de 3 500 avertissements de tous niveaux ont été émis pour ce projet. Ceci est typique pour les projets qui n'utilisent pas du tout d'outils d'analyse statique, même gratuits. Un autre facteur indiquant la qualité du projet est les erreurs répétées identifiées par l'analyseur. Les mêmes exemples de code ne seront pas donnés dans l'article, mais des erreurs identiques sont présentes dans le code à des centaines d'endroits.
De plus, ces insertions n'ajoutent pas de qualité au code:
#line 1 "./asn1/acse/packet-acse-template.c"
Il y en a plus de 1000 dans l'ensemble du projet. Ces insertions rendent difficile pour l'analyseur de comparer les avertissements générés avec le fichier souhaité. Mais je suis sûr que les développeurs ordinaires n'apprécient pas le support 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 deux types de structures:
mate_cfg_gog et
mate_cfg_gop , elles sont très similaires, mais pas identiques. Très probablement, les fonctions ont été mélangées dans ce fragment de code, qui est lourd d'erreurs potentielles dans le programme lors de l'accès à la mémoire par pointeur.
Voici des extraits de structures de données confuses:
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 juste calculée de la variable
HDR_TCP.dest_port n'est pas
confirmée .
Erreurs logiques
Dans cette section, je donnerai quelques exemples d'erreurs dans les instructions conditionnelles, et toutes seront fondamentalement différentes les unes 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 . L'écriture d'expressions via l'opérateur AND signifie que lorsque la condition interne est atteinte, la valeur de la variable de
direction n'est pas exactement égale à l'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 morceau de code est que le résultat de la condition dépend 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'instruction conditionnelle 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 une vraie erreur, mais le code redondant rend difficile la lecture et la compréhension du programme, 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 que nous avons affaire à une vérification inutile, et éventuellement à une faute de frappe, et dans l'un des ifs, quelque chose d'autre devrait être vérifié.
Des actifs é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);
Vérifier le pointeur dans
g_assert à ce stade est superflu, car le pointeur est vérifié avant cela. Peut-être que dans cette fonction, il n'y avait que
g_assert auparavant , et ils ont oublié de le supprimer, mais peut-être qu'ici, vous devriez vérifier un champ de structure.
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);
Il n'est pas entièrement clair pourquoi une
assertion est présente dans la fonction, qui duplique la condition de la boucle. Le compteur de cycles dans le corps ne change pas.
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é plusieurs lignes plus tôt qu'il n'est vérifié s'il est égal à zéro ou non.
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 du traitement d'une exception dans le programme, ce tableau est d'abord effacé par la fonction
g_strfreev , puis l'une des lignes de ce tableau est utilisée dans le message d'erreur. Très probablement, ces lignes du code 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 ,
vous devez appeler la fonction
g_free à un moment donné. Dans le fragment de code présenté, cela n'est pas fait, et dans la boucle à chaque itération, un nouveau morceau de RAM est alloué. Plusieurs fuites de mémoire se produisent.
Quelques avertissements supplémentaires pour des extraits 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, il existe de nombreux autres endroits similaires dans le code où la mémoire n'est pas 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 hardiment la valeur du compteur de boucles, et le font plusieurs fois. Il est difficile de dire si c'est une erreur ou non, mais il y a environ 10 cycles 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 que la fonction prend est immédiatement effiloché par une autre valeur. C'est très suspect. De plus, le code contient plusieurs dizaines de tels emplacements, il est donc difficile de dire s'il s'agit d'une erreur ou non. J'ai rencontré un code similaire plus tôt dans un autre grand projet, là c'était le bon code, personne n'a 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 incorrecte de la fonction
headerData . Les fonctions ont une valeur par défaut différente pour le paramètre de
rôle . Cela peut ne pas conduire au comportement attendu par le 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.
Au contraire, 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 qu'il y a peu d'exemples d'erreurs dans l'examen, mais dans le rapport complet, les cas présentés sont répétés des dizaines et des centaines de fois. Les aperçus d'avertissement de PVS-Studio sont fournis à des fins de démonstration uniquement. C'est une contribution à la qualité des projets open source, mais les contrôles ponctuels sont le moyen le plus inefficace d'appliquer la 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.
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Svyatoslav Razmyslov.
Wireshark 3.x: analyse de code sous macOS et revue des erreurs