# Wireshark 3\.x: анализ кода под macOS и обзор ошибок

Wireshark Foundation выпустила финальную stable\-версию популярного сетевого анализатора трафика — Wireshark 3\.0\.0\. В новом релизе устранено несколько багов, реализована возможность анализа новых протоколов и заменен драйвер WinPcap на Npcap\. Здесь заканчивается цитирование анонса и начинается наша заметка о багах в проекте\. Перед релизом их было исправлено явно недостаточно\. Давайте насобираем исправлений, чтобы был повод делать новый релиз :\)\.

![0623_Wireshark_ru/image1.png](https://import.viva64.com/docx/blog/0623_Wireshark_ru/image1.png)

## Введение

[Wireshark](https://www.wireshark.org/) \- это достаточно известный инструмент для захвата и анализа сетевого трафика\. Программа работает с подавляющим большинством известных протоколов, имеет понятный и логичный графический интерфейс, мощнейшую систему фильтров\. Wireshark \- кроссплатформенный, работает в таких ОС, как: Windows, Linux, macOS, Solaris, FreeBSD, NetBSD и многих других\.

Для поиска ошибок использовался статический анализатор [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. Для анализа исходного кода необходимо предварительно скомпилировать проект в какой\-нибудь операционной системе\. Выбор был большой не только из\-за кроссплатформенности проекта, но и из\-за кроссплатформенности анализатора\. Для анализа проекта я выбрал macOS\. Также запуск анализатора возможен в Windows и Linux\.

Про качество кода хочется рассказать отдельно\. К сожалению, я не могу назвать его хорошим\. Это субъективная оценка, но поскольку мы регулярно [проверяем](https://pvs-studio.ru/ru/blog/inspections/) множество проектов, у меня есть с чем сравнивать\. В данном случае в глаза бросается большое количество предупреждений PVS\-Studio на небольшом объёме кода\. Суммарно на этот проект выдано более 3500 предупреждений всех уровней\. Это характерно для проектов, в которых вообще не используются инструменты статического анализа, даже бесплатные\. Другим фактором, указывающим на качество проекта, являются повторяющиеся ошибки, выявленные анализатором\. В статье однотипные примеры кода не будут приводиться, но некоторые одинаковые ошибки присутствуют в коде в сотне мест\.

Ещё качества коду не добавляют и такие вставки:

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

Их встречается более 1000 во всём проекте\. Такие вставки усложняют анализатору сопоставление выдаваемых предупреждений с нужным файлом\. Но я уверен, что и обычные разработчики не получают удовольствия от поддержки такого кода\.

## Опечатки

**Предупреждение 1**

[V641](https://pvs-studio.ru/ru/docs/warnings/v641/) The size of the allocated memory buffer is not a multiple of the element size\. mate\_setup\.c 100

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

Есть структуры двух типов: _mate\_cfg\_gog_ и _mate\_cfg\_gop_, они очень похожи, но не идентичны\. Скорее всего, в этом фрагменте кода функции перепутали, что чревато потенциальными ошибками в программе во время обращения к памяти по указателю\.

Ниже приведены фрагменты перепутанных структур данных:

```cpp
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;
```

**Предупреждение 2**

[V519](https://pvs-studio.ru/ru/docs/warnings/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

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

В последней строке безусловно перетирается только что вычисленное значение переменной _HDR\_TCP\.dest\_port_\.

## Логические ошибки

В этом разделе приведу несколько примеров ошибок в условных операторах, причём все они будут принципиально отличаться друг от друга\.

**Предупреждение 1**

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'direction \=\= 0' is always false\. packet\-adb\.c 291

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

Во внешнем условии переменная _direction_ сравнивается с константой _P2P\_DIR\_RECV_\. Запись выражений через оператор AND означает, что при достижении внутреннего условия значение переменной _direction_ точно не будет равно другой константе _P2P\_DIR\_SENT_\. 

**Предупреждение 2**

[V590](https://pvs-studio.ru/ru/docs/warnings/v590/) Consider inspecting the '\(type \=\= 0x1\) \|\| \(type \!\= 0x4\)' expression\. The expression is excessive or contains a misprint\. packet\-fcsb3\.c 686

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

Ошибка этого фрагмента кода заключается в том, что результат условия зависит только от одного выражения:

```cpp
(type != FC_SBCCS_IU_CMD_DATA)
```

**Предупреждение 3**

[V590](https://pvs-studio.ru/ru/docs/warnings/v590/) Consider inspecting this expression\. The expression is excessive or contains a misprint\. snort\-config\.c 40

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

Результат условного оператора будет зависеть только от этой части выражения _\(source\[offset\] \=\= ' '\)_\. Проверка _\(source\[offset\] \!\= '\\0'\)_ является избыточной и её можно смело удалить\. Это не является настоящей ошибкой, но избыточный код затрудняет чтение и понимание программы, поэтому лучше его упростить\.

**Предупреждение 4**

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'eras\_pos \!\= NULL' is always true\. reedsolomon\.c 659

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

Возможно, мы имеем дело с лишней проверкой, а возможно \- с опечаткой, и в одном из if\-ов должно проверяться совсем иное\.

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

**Предупреждение 1**

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'sub\_dissectors \!\= NULL' is always true\. capture\_dissectors\.c 129

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'i < count' is always true\. packet\-netflow\.c 10363

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

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

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

**Предупреждение 1**

[V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'si\-\>conv' pointer was utilized before it was verified against nullptr\. Check lines: 2135, 2144\. packet\-smb2\.c 2135

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v774/) The 'protos' pointer was used after the memory was released\. packet\-k12\.c 311

```cpp
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_, а потом в сообщении о ошибке используется одна из строк этого массива\. Скорее всего, эти строки в коде следует поменять местами:

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

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

[V773](https://pvs-studio.ru/ru/docs/warnings/v773/) The 'ptmpstr' pointer was assigned values twice without releasing the memory\. A memory leak is possible\. idl2wrs\.c 2436

```cpp
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_](https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup) необходимо в какой\-то момент вызывать функцию [_g\_free_](https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#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](https://pvs-studio.ru/ru/docs/warnings/v535/) The variable 'i' is being used for this loop and for the outer loop\. Check lines: 7716, 7798\. packet\-opa\-mad\.c 7798

```cpp
/* 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](https://pvs-studio.ru/ru/docs/warnings/v763/) Parameter 'item' is always rewritten in function body before being used\. packet\-cdma2k\.c 1324

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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 бита приведёт к неопределённому поведению согласно стандарту языка\. 

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

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

## Заключение

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

Вы можете получить и проанализировать полный отчет самостоятельно\. Для этого просто необходимо [скачать](https://pvs-studio.ru/ru/pvs-studio/download/) и запустить анализатор PVS\-Studio\.