Wireshark 3.x: анализ кода под macOS и обзор ошибок08.04.2019 10:02

Странные ассерты

Предупреждение 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); // <=
  ....
}

Проверка указателя в g_assert в этом месте является лишней, так как указатель проверяется перед этим. Возможно, в этой функции раньше был только g_assert, и его забыли удалить, но, возможно, тут следовало проверить какое-нибудь поле структуры.

Предупреждение 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;
        }
    }
    ....
  }
  ....
}

Не совсем понятно, зачем в функции присутствует assert, который дублирует условие из цикла. Счётчик цикла в теле не изменяется.

Ошибки с указателями

Предупреждение 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);
    ....
  }
  ....
}

Указатель si→conv разыменовывается на несколько строк ранее, чем проверяется, равен он нулю или нет.

Предупреждение 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 — это массив строк. Во время обработки особой ситуации в программе, этот массив сначала очищается функцией g_strfreev, а потом в сообщении о ошибке используется одна из строк этого массива. Скорее всего, эти строки в коде следует поменять местами:

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

Утечки памяти

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

После функции g_strdup необходимо в какой-то момент вызывать функцию g_free. В представленном фрагменте кода это не делается, и в цикле на каждой итерации выделяется новый участок оперативной памяти. Возникают множественные утечки памяти.

Ещё несколько предупреждений на похожие фрагменты кода:

• 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

К сожалению, в коде ещё много других подобных мест, где не освобождается память.

Разное

Предупреждение 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;
      ....
    }
    ....
  }
  ....
}

В очень длинной функции разработчики смело изменяют значение счётчика цикла, причём делают это несколько раз. Сложно сказать, является это ошибкой или нет, но подобных циклов в проекте около 10.

Предупреждение 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);
  ....
}

Указатель item, который принимает функция, сразу перетирается другим значением. Это очень подозрительно. Причём в коде присутствует несколько десятков таких мест, поэтому сложно сказать, ошибка это или нет. Похожий код я ранее встречал в другом большом проекте, там это был верный код, просто никто не решался изменить интерфейс функции.

Предупреждение 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; // <=
  ....
};

Анализатор обнаружил некорректную перегрузку функции headerData. У функций отличается дефолтное значение параметра role. Это может приводить не к тому поведению, которое ожидал программист.

Предупреждение 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;
  ....
}

Сдвиг числа на 64 бита приведёт к неопределённому поведению согласно стандарту языка.

Скорее, правильный код должен быть таким:

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

Заключение

Может показаться, что в обзоре приведено мало примеров ошибок, но в полном отчёте представленные случаи повторяются десятки и сотни раз. Обзоры предупреждений PVS-Studio носят демонстрационный характер. Это вклад в качество проектов с открытым исходным кодом, но разовые проверки — самый неэффективный способ применения методологии статического анализа.

Вы можете получить и проанализировать полный отчет самостоятельно. Для этого просто необходимо скачать и запустить анализатор PVS-Studio.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Wireshark 3.x: code analysis under macOS and errors review

© Habrahabr.ru