Introduction

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

Typos

extern mate_cfg_gog* new_gogcfg(mate_config* mc, gchar* name) { mate_cfg_gog* cfg = (mate_cfg_gog *)g_malloc(sizeof(mate_cfg_gop)); .... }

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;

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); .... }

Logical errors

#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; } .... } .... }

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

(type != FC_SBCCS_IU_CMD_DATA)

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; }

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]); } } .... }

Strange asserts

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 ...

", name); if (getenv("WIRESHARK_ABORT_ON_DISSECTOR_BUG") != NULL) abort(); return; } g_assert(sub_dissectors != NULL); // <= .... }

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; } } .... } .... }

Errors with pointers

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); .... } .... }

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; } } .... }

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

Memory leaks

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

"); FPRINTF(eth_code, "....", tmpstr); FPRINTF(eth_code, "{

"); FPRINTF(eth_code, " ....", ptmpstr, ti->str); FPRINTF(eth_code, " return offset;

"); FPRINTF(eth_code, "}

"); FPRINTF(eth_code, "

"); ptmpstr=g_strdup(tmpstr); } .... }

V773 The 'ptmpstr' pointer was assigned values twice without releasing the memory. A memory leak is possible. idl2wrs.c 2447

V773 The 'ptmpstr' pointer was assigned values twice without releasing the memory. A memory leak is possible. idl2wrs.c 2713

V773 The 'ptmpstr' pointer was assigned values twice without releasing the memory. A memory leak is possible. idl2wrs.c 2728

V773 The 'ptmpstr' pointer was assigned values twice without releasing the memory. A memory leak is possible. idl2wrs.c 2732

V773 The 'ptmpstr' pointer was assigned values twice without releasing the memory. A memory leak is possible. idl2wrs.c 2745

Miscellaneous

/* 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; .... } .... } .... }

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); .... }

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; // <= .... };

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; .... }

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

Conclusion

Wireshark Foundation released the final stable-version of the popular network traffic analyzer — Wireshark 3.0.0. The new release fixes several bugs, it is now possible to analyze the new protocols, apart from that the driver on Npcap WinPcap is replaced. Here is where quoting of the announcement ends and our note about bugs in the project starts off. The projects authors definitely haven't done their best in fixing bugs before the release.Let's collect hotfixes right now to give a motive in doing a new release :). Wireshark is a well-known tool to capture and analyze network traffic. The program works with the vast majority of known protocols, has intuitive and logical graphical interface, an all-powerful system of filters. Wireshark is cross-platform, works in such OSs, as: Windows, Linux, macOS, Solaris, FreeBSD, NetBSD and many others.To do the source code analysis, we used PVS-Studio static code analyzer. To analyze the source code, first we needed to compile the project in an OS. The choice was wide not only due to the cross platform nature of the project, but also because of that of the analyzer. I chose macOS for the analysis. You can also run the analyzer under Windows and Linux.I'd like to draw special attention to the code quality. Unfortunately, I can't give big points to it. It is a subjective assessment, but since we regularly check plenty of projects, I have a frame of reference. What stands out in this case is a great number of PVS-Studio warnings for a small amount of code. In total, more than 3500 warnings of all levels triggered for this project. It is typical for the projects, which generally don't use static analysis tools, even free ones. Another factor pointing at the project quality is repeated errors detected by the analyzer. I won't cite same-type code examples, whereas some similar errors take place in hundreds of places.Such inserts also don't give a boost to code quality:There are more than 1000 of them in the entire project. Such inserts make it more difficult for the analyzer to match issued warnings with the appropriate files. Well, I think average developers won't get a kick out of maintaining such code.V641 The size of the allocated memory buffer is not a multiple of the element size. mate_setup.c 100There are structures of two types:andthey are very similar, but not equal. Most likely, in this code fragment functions are mixed up, which is fraught with potential errors in the program when accessing memory by a pointer.Here are the fragments of mixed-up data structures:V519 The 'HDR_TCP.dest_port' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 495, 496. text_import.c 496In the last line, the value (that has just been evaluated) of the variableis rewritten.In this section, I'll cite several examples of errors in conditional operators, and all of them will be completely different from each other.V547 Expression 'direction == 0' is always false. packet-adb.c 291In the external condition, thevariable is compared with the constantAccording to the expressions written with the AND operator, when getting to the inner condition, the value of the variablewill definitely be different from another constantV590 Consider inspecting the '(type == 0x1) || (type != 0x4)' expression. The expression is excessive or contains a misprint. packet-fcsb3.c 686The error of this code fragment is that the result of the condition depends only on one expression:V590 Consider inspecting this expression. The expression is excessive or contains a misprint. snort-config.c 40The result of the conditional operator will depend only on this part of the expression. The checkis redundant and can be safely removed. It is not the actual error, but redundant code makes code reading and understanding the program more difficult, so it's better to simplify it.V547 Expression 'eras_pos != NULL' is always true. reedsolomon.c 659Perhaps, we are dealing with a redundant check, probably with a typo, and another thing has to be checked in one of the conditions of theblock.V547 Expression 'sub_dissectors != NULL' is always true. capture_dissectors.c 129The check of thepointer is redundant here, as the pointer was already checked before that. Perhaps, onlywas in this function and a developer forgot to remove it, but maybe a structure field should have been checked here.V547 Expression 'i < count' is always true. packet-netflow.c 10363It's not quite clear why, which duplicates the condition from the loop, takes place in the function. The loop counter won't change in the body.V595 The 'si->conv' pointer was utilized before it was verified against nullptr. Check lines: 2135, 2144. packet-smb2.c 2135The pointergets dereferenced a few lines before its check for null.V774 The 'protos' pointer was used after the memory was released. packet-k12.c 311is an array of strings. When handling a special case in the program this array is first cleared by thefunction and then one string of this array is used in the error message. Most likely, these lines should be interchanged:V773 The 'ptmpstr' pointer was assigned values twice without releasing the memory. A memory leak is possible. idl2wrs.c 2436After the g_strdup function we need to call the g_free function at some point. It is not done in the given code snippet and a new part of memory is allocated in the loop on each iteration. Here come multiple memory leaks.Some other warnings for similar code fragments:Unfortunately, in the code there are many other similar cases, where memory gets released.V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 7716, 7798. packet-opa-mad.c 7798In a very long function developers boldly change the value of the loop counter, even doing it a few times. We cannot say for sure if it's an error or not, however, there are about 10 such loops in the project.V763 Parameter 'item' is always rewritten in function body before being used. packet-cdma2k.c 1324Thepointer, taken by the function, is immediately changed with another value. It is very suspicious. Moreover, the code contains several dozen of such places, so it's hard to decide whether it's an error or not. I came across similar code in another large project, such code was correct there, no one simply dared to change the function's interface.V762 It is possible a virtual function was overridden incorrectly. See third argument of function 'headerData' in derived class 'PacketListModel' and base class 'QAbstractItemModel'. packet_list_model.h 48The analyzer has detected the invalid overloading of thefunction. Functions have different default values of theparameter. This can cause the wrong behavior, not the one expected by a programmer.V610 Undefined behavior. Check the shift operator '>>'. The right operand ('bitshift' = [0..64]) is greater than or equal to the length in bits of the promoted left operand. proto.c 10941A 64-bit shift will result in undefined behavior according to language standard.Most likely, the correct code should be like this:It might seem that this review shows few errors, but in the full report the considered cases repeat dozens and hundreds of times. In addition, PVS-Studio warnings reviews are of demonstrative nature. They represent contribution to the quality of open source projects, but one-time checks are the most inefficient in terms of static analysis methodology.You can get and analyze the full report yourself. To do this, you just need to download and run the PVS-Studio analyzer.