Die Wireshark Foundation hat die endgültige stabile Version des beliebten Netzwerkverkehrsanalysators Wireshark 3.0.0 veröffentlicht. Die neue Version behebt mehrere Fehler. Es ist jetzt möglich, die neuen Protokolle zu analysieren. Außerdem wird der Treiber auf Npcap WinPcap ersetzt. Hier endet das Zitieren der Ankündigung und unser Hinweis zu Fehlern im Projekt beginnt. Die Projektautoren haben definitiv nicht ihr Bestes gegeben, um Fehler vor der Veröffentlichung zu beheben.
Sammeln wir jetzt Hotfixes, um ein Motiv für eine neue Version zu geben :).
Einführung
Wireshark ist ein bekanntes Tool zur Erfassung und Analyse des Netzwerkverkehrs. Das Programm funktioniert mit den meisten bekannten Protokollen, verfügt über eine intuitive und logische grafische Oberfläche, ein leistungsstarkes Filtersystem. Wireshark ist plattformübergreifend und funktioniert unter folgenden Betriebssystemen wie Windows, Linux, MacOS, Solaris, FreeBSD, NetBSD und vielen anderen.
Für die Quellcode-Analyse haben wir den statischen Code-Analysator
PVS-Studio verwendet. Um den Quellcode zu analysieren, mussten wir zuerst das Projekt in einem Betriebssystem kompilieren. Die Auswahl war nicht nur aufgrund des plattformübergreifenden Charakters des Projekts, sondern auch aufgrund des Analysators groß. Ich habe macOS für die Analyse ausgewählt. Sie können den Analyzer auch unter Windows und Linux ausführen.
Ich möchte besonders auf die Codequalität aufmerksam machen. Leider kann ich keine großen Punkte geben. Es ist eine subjektive Einschätzung, aber da wir regelmäßig viele Projekte überprüfen, habe ich einen Bezugsrahmen. Was in diesem Fall auffällt, ist eine große Anzahl von PVS-Studio-Warnungen für eine kleine Menge Code. Insgesamt wurden für dieses Projekt mehr als 3500 Warnungen aller Ebenen ausgelöst. Dies ist typisch für Projekte, bei denen im Allgemeinen keine statischen Analysewerkzeuge verwendet werden, auch keine kostenlosen. Ein weiterer Faktor, der auf die Projektqualität hinweist, sind wiederholte Fehler, die vom Analysegerät erkannt werden. Ich werde keine Codebeispiele vom gleichen Typ zitieren, während einige ähnliche Fehler an Hunderten von Stellen auftreten.
Solche Einfügungen verbessern auch nicht die Codequalitä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, ausgegebene Warnungen mit den entsprechenden Dateien abzugleichen. Nun, ich denke, durchschnittliche Entwickler werden keinen Kick davon bekommen, solchen Code zu pflegen.
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 gleich. Höchstwahrscheinlich sind in diesem Code Fragmentfunktionen verwechselt, was mit potenziellen Fehlern im Programm behaftet ist, wenn über einen Zeiger auf den Speicher zugegriffen wird.
Hier sind die Fragmente gemischter 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 Wert (der gerade ausgewertet wurde) der Variablen
HDR_TCP.dest_port neu geschrieben.
Logische Fehler
In diesem Abschnitt werde ich einige Beispiele für Fehler in bedingten Operatoren anführen, die sich alle vollständig 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
. Gemäß den mit dem AND-Operator geschriebenen Ausdrücken unterscheidet sich der Wert der variablen
Richtung beim Erreichen der inneren Bedingung definitiv von einer anderen Konstanten
P2P_DIR_SENT .
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 Codefragments 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 des bedingten Operators hängt nur von diesem Teil des Ausdrucks ab
(Quelle [Offset] == '') . Die Prüfung
(Quelle [Offset]! = '\ 0') ist redundant und kann sicher entfernt werden. Es ist nicht der eigentliche Fehler, aber redundanter Code erschwert das Lesen und Verstehen des Codes des Programms. Daher ist es besser, ihn 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 einer redundanten Prüfung zu tun, wahrscheinlich mit einem Tippfehler, und eine andere Sache muss in einer der Bedingungen des
if- Blocks überprüft werden.
Seltsame Behauptungen
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);
Die Prüfung des
g_assert- Zeigers ist hier redundant, da der Zeiger bereits zuvor geprüft wurde. Vielleicht war nur
g_assert in dieser Funktion und ein Entwickler hat vergessen, sie zu entfernen, aber vielleicht hätte hier ein Strukturfeld überprüft werden sollen.
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
Assert , das die Bedingung aus der Schleife dupliziert, in der Funktion stattfindet. Der Schleifenzähler ändert sich im Körper 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 einige Zeilen vor seiner Prüfung auf null dereferenziert.
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. Bei der Behandlung eines Sonderfalls im Programm wird dieses Array zuerst von der Funktion
g_strfreev gelöscht , und dann wird eine Zeichenfolge dieses Arrays in der Fehlermeldung verwendet. Höchstwahrscheinlich sollten diese Zeilen 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 wir
irgendwann die Funktion
g_free aufrufen. Dies wird im angegebenen Codeausschnitt nicht ausgeführt, und bei jeder Iteration wird ein neuer Teil des Speichers in der Schleife zugewiesen. Hier kommen mehrere Speicherlecks.
Einige andere 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 im Code viele andere ähnliche Fälle, in denen 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 mutig den Wert des Schleifenzählers, sogar einige Male. Wir können nicht sicher sagen, ob es sich um einen Fehler handelt oder nicht, es gibt jedoch ungefähr 10 solcher Schleifen 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 von der Funktion verwendete Elementzeiger wird sofort um einen anderen Wert geändert. Es ist sehr verdächtig. Darüber hinaus enthält der Code mehrere Dutzend solcher Stellen, sodass es schwierig ist zu entscheiden, ob es sich um einen Fehler handelt oder nicht. Ich bin in einem anderen großen Projekt auf ähnlichen Code gestoßen, dieser Code war dort korrekt, niemand hat es einfach gewagt, die Benutzeroberfläche 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 die ungültige Überladung der Funktion
headerData festgestellt . Funktionen haben unterschiedliche Standardwerte für den Rollenparameter. Dies kann zu einem falschen Verhalten führen, das nicht von einem Programmierer erwartet wird.
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äß Sprachstandard.
Höchstwahrscheinlich sollte der richtige Code folgendermaßen aussehen:
if (bitshift == 64) available_bits = 0; else available_bits = G_GUINT64_CONSTANT(0xFFFFFFFFFFFFFFFF) >> bitshift;
Fazit
Es mag den Anschein haben, dass diese Überprüfung nur wenige Fehler aufweist, aber im vollständigen Bericht wiederholen sich die betrachteten Fälle Dutzende und Hunderte Male. Darüber hinaus sind PVS-Studio-Warnüberprüfungen demonstrativer Natur. Sie stellen einen Beitrag zur Qualität von Open Source-Projekten dar, einmalige Überprüfungen sind jedoch hinsichtlich der statischen Analysemethode am ineffizientesten.
Sie können den vollständigen Bericht selbst abrufen und analysieren. Dazu müssen Sie nur den PVS-Studio-Analysator
herunterladen und ausführen.