Wireshark 3.x: code analysis under macOS and errors review08.04.2019 09:47

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

© Habrahabr.ru