Wireshark 3.x: code analysis under macOS and errors review

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

Introduction

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:

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

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.

Typos

Warning 1

V641 The size of the allocated memory buffer is not a multiple 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 structures of two types: mate_cfg_gog and mate_cfg_gop, they 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:

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

In the last line, the value (that has just been evaluated) of the variable HDR_TCP.dest_port is rewritten.

Logical errors

In this section, I'll cite several examples of errors in conditional operators, and all of them will be completely 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. According to the expressions written with the AND operator, when getting to the inner condition, the value of the variable direction will definitely be different from another constant P2P_DIR_SENT.

Warning 2

V590 Consider inspecting the '(type == 0x1) || (type != 0x4)' expression. The expression is excessive or contains a 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 fragment is that the result of the condition depends only on one expression:

(type != FC_SBCCS_IU_CMD_DATA)

Warning 3

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. snort-config.c 40

static char *skipWhiteSpace(char *source, int *accumulated_offset)
{
  int offset = 0;

  /* Skip any leading whitespace */
  while (source[offset] != '' && 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] != '') is 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.

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 a redundant check, probably with a typo, and another thing has to be checked in one of the conditions of the if block.

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

The check of the g_assert pointer is redundant here, as the pointer was already checked before that. Perhaps, only g_assert was in this function and a developer forgot to remove it, but maybe a structure field should have been checked here.

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's not quite clear why assert, which duplicates the condition from the loop, takes place in the function. The loop counter won't change in the body.

Errors with pointers

Warning 1

V595 The 'si->conv' pointer was utilized 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 gets dereferenced a few lines before its check for null.

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. When handling a special case in the program this array is first cleared by the g_strfreev function and then one string of this array is used in the error message. Most likely, these lines should be interchanged:

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

Memory leaks

V773 The 'ptmpstr' pointer was assigned values twice without releasing the memory. 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 intn");
    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 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:

• 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

Unfortunately, in the code there are many other similar cases, where memory gets released.

Miscellaneous

Warning 1

V535 The variable 'i' is being used for this loop and for the outer 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, 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.

Warning 2

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

Warning 3

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 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 has detected the invalid overloading of the headerData function. Functions have different default values of the role parameter. This can cause the wrong behavior, not the one expected by a programmer.

Warning 4

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

A 64-bit shift will result in undefined behavior according to language standard.

Most likely, the correct code should be like this:

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

Conclusion

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.

Автор: SvyatoslavMC

Источник