Geekly Articles each Day
📜 ⬆️ ⬇️

Wireshark 3.x: code analysis for macOS and error review

Picture 1

Wireshark Foundation has released the final stable-version of the popular network traffic analyzer - Wireshark 3.0.0. The new release eliminated several bugs, implemented the ability to analyze new protocols and replaced the WinPcap driver with Npcap. Here ends the citation of the announcement and begins our note about the bugs in the project. Before the release of them was fixed clearly not enough. Let's collect fixes so that there is a reason to make a new release :).

Introduction


Wireshark is a well-known tool for capturing and analyzing network traffic. The program works with the vast majority of known protocols, has a clear and logical graphical interface, a powerful filter system. Wireshark - cross-platform, works in such OS as: Windows, Linux, macOS, Solaris, FreeBSD, NetBSD and many others.

For searching errors, the PVS-Studio static analyzer was used. To analyze the source code, you must first compile the project in any operating system. The choice was great not only because of the cross-platform project, but also because of the cross-platform analyzer. To analyze the project, I chose macOS. Also, the launch of the analyzer is possible in Windows and Linux.
')
About the quality of the code I want to tell separately. Unfortunately, I can not call it good. This is a subjective assessment, but since we regularly check many projects, I have something to compare with. In this case, a large number of PVS-Studio warnings catches the eye on a small amount of code. In total, more than 3,500 warnings of all levels have been issued for this project. This is typical for projects in which static analysis tools are not used at all, even free ones. Another factor indicating the quality of the project is repeated errors detected by the analyzer. In the article, the same type of code examples will not be given, but some identical errors are present in the code in a hundred places.

More quality code does not add such inserts:

/* Input file: packet-acse-template.c */ #line 1 "./asn1/acse/packet-acse-template.c"

There are more than 1000 of them in the whole project. Such inserts make it difficult for the analyzer to match the issued warnings with the desired file. But I am sure that ordinary developers do not enjoy the support of such code.

Typos


Warning 1

V641 is a multiple of the size of the element size. 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)); .... }

There are two types of structures: mate_cfg_gog and mate_cfg_gop , they are very similar, but not identical. Most likely, in this code snippet, the functions are confused, which is fraught with potential errors in the program during memory access by the pointer.

The following are fragments of confused data structures:

 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;

Warning 2

V519 The 'HDR_TCP.dest_port' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 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); .... }

The last line of course just overwrites the newly calculated value of the variable HDR_TCP.dest_port .

Logical errors


In this section I will give several examples of errors in conditional operators, and all of them will be fundamentally different from each other.

Warning 1

V547 Expression 'direction == 0' is always false. 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; // unreachable code } else { service_data->remote_id = arg0; } .... } .... }

In the external condition, the direction variable is compared with the constant P2P_DIR_RECV . Writing expressions through the AND operator means that when the internal condition is reached, the value of the direction variable will definitely not be equal to another constant P2P_DIR_SENT .

Warning 2

V590 Consider inspecting the '(type == 0x1) || (type! = 0x4) 'expression. The expression is misprint. packet-fcsb3.c 686

 static int dissect_fc_sbccs (....) { .... else if ((type == FC_SBCCS_IU_CMD_HDR) || (type != FC_SBCCS_IU_CMD_DATA)) { .... }

The error of this code snippet is that the result of the condition depends on only one expression:

 (type != FC_SBCCS_IU_CMD_DATA)

Warning 3

V590 Consider inspecting this expression. The expression is misprint. snort-config.c 40

 static char *skipWhiteSpace(char *source, int *accumulated_offset) { int offset = 0; /* Skip any leading whitespace */ while (source[offset] != '\0' && source[offset] == ' ') { offset++; } *accumulated_offset += offset; return source + offset; }

The result of the conditional operator will depend only on this part of the expression (source [offset] == ​​'') . The check (source [offset]! = '\ 0') is redundant and can be safely removed. This is not a real mistake, but redundant code makes it difficult to read and understand the program, so it’s best to simplify it.

Warning 4

V547 Expression 'eras_pos! = NULL' is always true. 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]); } } .... }

Perhaps we are dealing with an extra check, and possibly with a typo, and one of the if-s should be checked for something completely different.

Strange asserts


Warning 1

V547 Expression 'sub_dissectors! = NULL' is always true. 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); // <= .... }

Checking the pointer in g_assert in this place is superfluous, since the pointer is checked before this. Perhaps, in this function there used to be only g_assert , and it was forgotten to be deleted, but perhaps there should have been some field structure checked.

Warning 2

V547 Expression 'i <count' is always true. 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); // <= tmplt_p->fields_p[fields_type][i].type = type; tmplt_p->fields_p[fields_type][i].length = length; tmplt_p->fields_p[fields_type][i].pen = pen; tmplt_p->fields_p[fields_type][i].pen_str = pen_str; if (length != VARIABLE_LENGTH) {/ tmplt_p->length += length; } } .... } .... }

It is not entirely clear why the function contains assert , which duplicates the condition from the loop. The loop counter in the body does not change.

Pointer errors


Warning 1

V595 The 'si-> conv' pointer was used before it was verified against nullptr. Check lines: 2135, 2144. packet-smb2.c 2135

 static int dissect_smb2_fid(....) { .... g_hash_table_insert(si->conv->fids, sfi, sfi); // <= si->file = sfi; if (si->saved) { si->saved->file = sfi; si->saved->policy_hnd = policy_hnd; } if (si->conv) { // <= eo_file_info = (.... *)g_hash_table_lookup(si->conv->files,&policy_hnd); .... } .... }

The pointer si-> conv is dereferenced several lines earlier than it is checked whether it is equal to zero or not.

Warning 2

V774 The 'protos' pointer was used after the memory was released. 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 is an array of strings. During the processing of a special situation in the program, this array is first cleared by the g_strfreev function, and then one of the rows of this array is used in the error message. Most likely, these lines in the code should be swapped:

 *err = g_strdup_printf("Could not find dissector for: '%s'", protos[i]); g_strfreev(protos);

Memory leaks


V773 The 'ptmpstr' pointer was assigned values. A memory leak is 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); } .... }

After the g_strdup function, you need to call the g_free function at some point. In the presented code snippet this is not done, and in the loop at each iteration a new section of RAM is allocated. Multiple memory leaks occur.

A few more warnings for similar code snippets:


Unfortunately, there are many other similar places in the code where memory is not released.

miscellanea


Warning 1

V535 The variable 'i' is being used for the loop. Check lines: 7716, 7798. packet-opa-mad.c 7798

 /* Parse GetVFInfo MAD from the Performance Admin class. */ static gint parse_GetVFInfo(....) { .... for (i = 0; i < records; i++) { // <= line 7716 .... for (i = 0; i < PM_UTIL_BUCKETS; i++) { // <= line 7748 GetVFInfo_Util_Stats_Bucket_item = proto_tree_add_item(....); proto_item_set_text(....); local_offset += 4; } .... for (i = 0; i < PM_ERR_BUCKETS; i++) { // <= line 7798 GetVFInfo_Error_Stats_Bucket_item = proto_tree_add_item(....); proto_item_set_text(....); local_offset += 4; .... } .... } .... }

In a very long function, developers boldly change the value of the loop counter, and they do it several times. It is difficult to say whether this is a mistake or not, but there are about 10 similar cycles in the project.

Warning 2

Parameter 'item' is always rewritten in function body before being used. 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, ....); // <= subtree = proto_item_add_subtree(item, ett_cdma2k_subtree1); .... }

The item pointer that the function accepts is immediately overwritten by another value. This is very suspicious. Moreover, there are several dozens of such places in the code, so it’s hard to say if this is a mistake or not. I had previously encountered a similar code in another large project; there it was the correct code, just no one dared to change the interface of the function.

Warning 3

V762 It was possible a virtual function was overridden incorrectly. See the third argument of the function 'headerData' in the derived class 'PacketListModel' and the base class 'QAbstractItemModel'. packet_list_model.h 48

 QVariant QAbstractItemModel::headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const // <= class PacketListModel : public QAbstractItemModel { Q_OBJECT public: .... QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole | Qt::ToolTipRole) const; // <= .... };

The analyzer detected incorrect overloading of the headerData function. Functions have a different default value for the role parameter. This may not lead to the behavior that the programmer expected.

Warning 4

V610 Undefined behavior. Check the shift operator '>>'. The right operand ('bitshift' = [0..64]) is the operand. 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; .... }

Shifting the number to 64 bits will result in undefined behavior according to the language standard.

Rather, the correct code should be:

 if (bitshift == 64) available_bits = 0; else available_bits = G_GUINT64_CONSTANT(0xFFFFFFFFFFFFFFFF) >> bitshift;

Conclusion


It may seem that there are few examples of errors in the review, but in the full report the cases presented are repeated tens and hundreds of times. The PVS-Studio warning reviews are of a demo nature. This is a contribution to the quality of open source projects, but one-time checks are the most inefficient way to use the static analysis methodology.

You can get and analyze the full report yourself. For this, you just need to download and run the PVS-Studio analyzer.



If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Wireshark 3.x: code analysis under macOS and errors review

Source: https://habr.com/ru/post/447158/

More articles:


All Articles