Die Wireshark Foundation hat die endgültige stabile Version des beliebten Netzwerkverkehrsanalysators Wireshark 3.0.0 veröffentlicht. Die neue Version behebt mehrere Fehler, implementiert die Möglichkeit, neue Protokolle zu analysieren, und ersetzt den WinPcap-Treiber durch Npcap. Hier endet das Zitieren der Ankündigung und unser Artikel über Fehler im Projekt beginnt. Vor der Veröffentlichung wurden sie eindeutig unzureichend korrigiert. Lassen Sie uns einige Korrekturen vornehmen, damit es einen Grund gibt, eine neue Version zu erstellen :).
Einführung
Wireshark ist ein bekanntes Tool zur Erfassung und Analyse des Netzwerkverkehrs. Das Programm arbeitet mit den meisten bekannten Protokollen, verfügt über eine klare und logische grafische Oberfläche und ein leistungsstarkes Filtersystem. Wireshark - plattformübergreifend, funktioniert unter Betriebssystemen wie Windows, Linux, MacOS, Solaris, FreeBSD, NetBSD und vielen anderen.
Um nach Fehlern zu suchen, haben wir den statischen Analysator
PVS-Studio verwendet. Um den Quellcode zu analysieren, müssen Sie zuerst das Projekt in einem Betriebssystem kompilieren. Die Auswahl war nicht nur aufgrund des plattformübergreifenden Charakters des Projekts groß, sondern auch aufgrund des plattformübergreifenden Charakters des Analysators. Für die Analyse des Projekts habe ich macOS gewählt. Der Analysator kann auch unter Windows und Linux gestartet werden.
Über die Qualität des Codes möchte ich separat berichten. Leider kann ich es nicht gut nennen. Dies ist eine subjektive Einschätzung, aber da wir regelmäßig viele Projekte
überprüfen , habe ich etwas zu vergleichen. In diesem Fall fällt eine große Anzahl von PVS-Studio-Warnungen auf eine kleine Menge Code auf. Insgesamt wurden für dieses Projekt über 3.500 Warnungen aller Ebenen ausgegeben. Dies ist typisch für Projekte, die überhaupt keine statischen Analysewerkzeuge verwenden, auch keine kostenlosen. Ein weiterer Faktor, der die Qualität des Projekts anzeigt, sind die vom Analysator identifizierten wiederholten Fehler. Dieselben Codebeispiele werden im Artikel nicht angegeben, aber einige identische Fehler sind im Code an Hunderten von Stellen vorhanden.
Solche Einfügungen verleihen dem Code auch keine Qualität:
#line 1 "./asn1/acse/packet-acse-template.c"
Es gibt mehr als 1000 von ihnen im gesamten Projekt. Solche Einfügungen erschweren es dem Analysator, die erzeugten Warnungen mit der gewünschten Datei zu vergleichen. Aber ich bin sicher, dass gewöhnliche Entwickler die Unterstützung eines solchen Codes nicht genießen.
Tippfehler
Warnung 1V641 Die Größe des zugewiesenen Speicherpuffers ist kein Vielfaches der Elementgröße. 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)); .... }
Es gibt zwei Arten von Strukturen:
mate_cfg_gog und
mate_cfg_gop . Sie sind sehr ähnlich, aber nicht identisch. Höchstwahrscheinlich wurden die Funktionen in diesem Codefragment verwechselt, das während des Speicherzugriffs per Zeiger mit potenziellen Fehlern im Programm behaftet ist.
Das Folgende sind Ausschnitte aus verwirrten Datenstrukturen:
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;
Warnung 2V519 Der Variablen 'HDR_TCP.dest_port' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 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); .... }
In der letzten Zeile wird der gerade berechnete Wert der Variablen
HDR_TCP.dest_port nicht bestätigt .
Logische Fehler
In diesem Abschnitt werde ich einige Beispiele für Fehler in bedingten Anweisungen geben, die sich alle grundlegend voneinander unterscheiden.
Warnung 1V547 Der Ausdruck 'direction == 0' ist immer falsch. 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;
Im externen Zustand wird die
Richtungsvariable mit der Konstanten
P2P_DIR_RECV verglichen. Das Schreiben von Ausdrücken über den AND-Operator bedeutet, dass der Wert der
Richtungsvariablen bei Erreichen der internen Bedingung nicht genau der anderen Konstante
P2P_DIR_SENT entspricht .
Warnung 2V590 Überprüfen Sie die '(Typ == 0x1) || (Typ! = 0x4) 'Ausdruck. Der Ausdruck ist übertrieben oder enthält einen Druckfehler. packet-fcsb3.c 686
static int dissect_fc_sbccs (....) { .... else if ((type == FC_SBCCS_IU_CMD_HDR) || (type != FC_SBCCS_IU_CMD_DATA)) { .... }
Der Fehler dieses Codeteils besteht darin, dass das Ergebnis der Bedingung nur von einem Ausdruck abhängt:
(type != FC_SBCCS_IU_CMD_DATA)
Warnung 3V590 Überprüfen Sie diesen Ausdruck. Der Ausdruck ist übertrieben oder enthält einen Druckfehler. 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; }
Das Ergebnis der bedingten Anweisung hängt nur von diesem Teil des Ausdrucks ab
(Quelle [Offset] == '') . Die Prüfung
(Quelle [Offset]! = '\ 0') ist redundant und kann sicher entfernt werden. Dies ist kein wirklicher Fehler, aber redundanter Code erschwert das Lesen und Verstehen des Programms. Daher ist es besser, es zu vereinfachen.
Warnung 4V547 Der Ausdruck 'eras_pos! = NULL' ist immer wahr. 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]); } } .... }
Vielleicht haben wir es mit unnötiger Überprüfung und möglicherweise mit einem Tippfehler zu tun, und in einem der Wenns sollte etwas anderes überprüft werden.
Seltsame Vermögenswerte
Warnung 1V547 Ausdruck 'sub_dissectors! = NULL' ist immer wahr. 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);
Das Überprüfen des Zeigers in
g_assert an dieser Stelle ist überflüssig, da der Zeiger zuvor überprüft wird. Vielleicht gab es in dieser Funktion
vorher nur
g_assert und sie haben vergessen, es zu löschen, aber vielleicht sollten Sie hier ein Strukturfeld überprüfen.
Warnung 2V547 Der Ausdruck 'i <count' ist immer wahr. 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);
Es ist nicht ganz klar, warum in der Funktion eine
Zusicherung vorhanden ist, die die Bedingung aus der Schleife dupliziert. Der Zykluszähler im Körper ändert sich nicht.
Fehler mit Zeigern
Warnung 1V595 Der Zeiger 'si-> conv' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen: 2135, 2144. packet-smb2.c 2135
static int dissect_smb2_fid(....) { .... g_hash_table_insert(si->conv->fids, sfi, sfi);
Der Zeiger
si-> conv wird mehrere Zeilen früher dereferenziert, als geprüft wird, ob er gleich Null ist oder nicht.
Warnung 2V774 Der 'Protos'-Zeiger wurde verwendet, nachdem der Speicher freigegeben wurde. Paket-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 ist eine Reihe von Zeichenfolgen. Während der Verarbeitung einer Ausnahme im Programm wird dieses Array zuerst von der Funktion
g_strfreev gelöscht, und dann wird eine der Zeilen dieses Arrays in der Fehlermeldung verwendet. Höchstwahrscheinlich sollten diese Zeilen im Code vertauscht werden:
*err = g_strdup_printf("Could not find dissector for: '%s'", protos[i]); g_strfreev(protos);
Speicherlecks
V773 Dem Zeiger 'ptmpstr' wurden zweimal Werte zugewiesen, ohne den Speicher freizugeben. Ein Speicherverlust ist möglich. 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); } .... }
Nach der Funktion
g_strdup müssen
Sie irgendwann die Funktion
g_free aufrufen. In dem dargestellten Codefragment wird dies nicht durchgeführt, und in der Schleife wird bei jeder Iteration ein neues Stück RAM zugewiesen. Es treten mehrere Speicherlecks auf.
Noch ein paar Warnungen für ähnliche Codefragmente:
- V773 Dem Zeiger 'ptmpstr' wurden zweimal Werte zugewiesen, ohne den Speicher freizugeben. Ein Speicherverlust ist möglich. idl2wrs.c 2447
- V773 Dem Zeiger 'ptmpstr' wurden zweimal Werte zugewiesen, ohne den Speicher freizugeben. Ein Speicherverlust ist möglich. idl2wrs.c 2713
- V773 Dem Zeiger 'ptmpstr' wurden zweimal Werte zugewiesen, ohne den Speicher freizugeben. Ein Speicherverlust ist möglich. idl2wrs.c 2728
- V773 Dem Zeiger 'ptmpstr' wurden zweimal Werte zugewiesen, ohne den Speicher freizugeben. Ein Speicherverlust ist möglich. idl2wrs.c 2732
- V773 Dem Zeiger 'ptmpstr' wurden zweimal Werte zugewiesen, ohne den Speicher freizugeben. Ein Speicherverlust ist möglich. idl2wrs.c 2745
Leider gibt es viele andere ähnliche Stellen im Code, an denen kein Speicher freigegeben wird.
Verschiedenes
Warnung 1V535 Die Variable 'i' wird für diese Schleife und für die äußere Schleife verwendet. Überprüfen Sie die Zeilen: 7716, 7798. packet-opa-mad.c 7798
static gint parse_GetVFInfo(....) { .... for (i = 0; i < records; i++) {
In einer sehr langen Funktion ändern Entwickler den Wert des Schleifenzählers mutig und tun dies mehrmals. Es ist schwer zu sagen, ob dies ein Fehler ist oder nicht, aber es gibt ungefähr 10 solcher Zyklen im Projekt.
Warnung 2V763 Der Parameter 'item' wird vor seiner Verwendung immer im Funktionskörper neu geschrieben. 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, ....);
Der Elementzeiger, den die Funktion verwendet, wird sofort durch einen anderen Wert ausgefranst. Das ist sehr verdächtig. Darüber hinaus enthält der Code mehrere Dutzend solcher Stellen, sodass es schwierig ist zu sagen, ob dies ein Fehler ist oder nicht. Ich habe einen ähnlichen Code früher in einem anderen großen Projekt getroffen, dort war es der richtige Code, nur niemand wagte es, die Schnittstelle der Funktion zu ändern.
Warnung 3V762 Möglicherweise wurde eine virtuelle Funktion falsch überschrieben. Siehe drittes Argument der Funktion 'headerData' in der abgeleiteten Klasse 'PacketListModel' und der Basisklasse 'QAbstractItemModel'. packet_list_model.h 48
QVariant QAbstractItemModel::headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const
Der Analysator hat eine falsche Überlastung der Funktion
headerData festgestellt . Funktionen haben einen anderen Standardwert für den Rollenparameter. Dies führt möglicherweise nicht zu dem vom Programmierer erwarteten Verhalten.
Warnung 4V610 Undefiniertes Verhalten. Überprüfen Sie den Schaltoperator '>>'. Der rechte Operand ('bitshift' = [0..64]) ist größer oder gleich der Länge des heraufgestuften linken Operanden in Bit. 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; .... }
Eine 64-Bit-Verschiebung führt zu einem undefinierten Verhalten gemäß dem Sprachstandard.
Der richtige Code sollte vielmehr so lauten:
if (bitshift == 64) available_bits = 0; else available_bits = G_GUINT64_CONSTANT(0xFFFFFFFFFFFFFFFF) >> bitshift;
Fazit
Es mag den Anschein haben, dass es nur wenige Beispiele für Fehler in der Überprüfung gibt, aber im vollständigen Bericht werden die vorgestellten Fälle Dutzende und Hunderte Male wiederholt. PVS-Studio-Warnübersichten dienen nur zu Demonstrationszwecken. Dies ist ein Beitrag zur Qualität von Open Source-Projekten, aber einmalige Überprüfungen sind die ineffizienteste Methode zur Anwendung der statischen Analysemethode.
Sie können den vollständigen Bericht selbst abrufen und analysieren. Dazu müssen Sie nur den PVS-Studio-Analysator
herunterladen und ausführen.
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Link zur Übersetzung: Svyatoslav Razmyslov.
Wireshark 3.x: Code-Analyse unter macOS und Fehlerüberprüfung