to the top
close form
Для получения триального ключа
заполните форму ниже
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Wireshark 3.x: анализ кода под macOS и …

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

08 Апр 2019

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

0623_Wireshark_ru/image1.png

Введение

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

Для поиска ошибок использовался статический анализатор PVS-Studio. Для анализа исходного кода необходимо предварительно скомпилировать проект в какой-нибудь операционной системе. Выбор был большой не только из-за кроссплатформенности проекта, но и из-за кроссплатформенности анализатора. Для анализа проекта я выбрал macOS. Также запуск анализатора возможен в Windows и Linux.

Про качество кода хочется рассказать отдельно. К сожалению, я не могу назвать его хорошим. Это субъективная оценка, но поскольку мы регулярно проверяем множество проектов, у меня есть с чем сравнивать. В данном случае в глаза бросается большое количество предупреждений PVS-Studio на небольшом объёме кода. Суммарно на этот проект выдано более 3500 предупреждений всех уровней. Это характерно для проектов, в которых вообще не используются инструменты статического анализа, даже бесплатные. Другим фактором, указывающим на качество проекта, являются повторяющиеся ошибки, выявленные анализатором. В статье однотипные примеры кода не будут приводиться, но некоторые одинаковые ошибки присутствуют в коде в сотне мест.

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

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

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

Опечатки

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

Есть структуры двух типов: mate_cfg_gog и 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;

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

В последней строке безусловно перетирается только что вычисленное значение переменной HDR_TCP.dest_port.

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

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

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

Во внешнем условии переменная direction сравнивается с константой P2P_DIR_RECV. Запись выражений через оператор AND означает, что при достижении внутреннего условия значение переменной direction точно не будет равно другой константе P2P_DIR_SENT.

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

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

(type != FC_SBCCS_IU_CMD_DATA)

Предупреждение 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] != '\0' && source[offset] == ' ') {
    offset++;
  }

  *accumulated_offset += offset;
  return source + offset;
}

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

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

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

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

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

Популярные статьи по теме
Под капотом SAST: как инструменты анализа кода ищут дефекты безопасности

Дата: 26 Янв 2023

Автор: Сергей Васильев

Сегодня речь о том, как SAST-решения ищут дефекты безопасности. Расскажу, как разные подходы к поиску потенциальных уязвимостей дополняют друг друга, зачем нужен каждый из них и как теория ложится на…
Ложные представления программистов о неопределённом поведении

Дата: 17 Янв 2023

Автор: Гость

Неопределённое поведение (UB) – непростая концепция в языках программирования и компиляторах. Я слышал много заблуждений в том, что гарантирует компилятор при наличии UB. Это печально, но неудивитель…
Топ-10 ошибок в C++ проектах за 2022 год

Дата: 29 Дек 2022

Автор: Владислав Столяров

Дело идёт к Новому году, а значит, самое время традиционно вспомнить десять самых интересных срабатываний, которые нашёл PVS-Studio в 2022 году.
PVS-Studio и RPCS3: лучшие предупреждения в один клик

Дата: 12 Дек 2022

Автор: Александр Куренев

Best Warnings — режим анализатора, оставляющий в окне вывода 10 лучших предупреждений. Мы предлагаем вам ознакомиться с обновлённым режимом Best Warnings на примере проверки проекта RPCS3.
Holy C++

Дата: 23 Ноя 2022

Автор: Гость

В этой статье постараюсь затронуть все вещи, которые можно без зазрения совести выкинуть из С++, не потеряв ничего (кроме боли), уменьшить стандарт, нагрузку на создателей компиляторов, студентов, из…

Комментарии (0)

Следующие комментарии next comments
close comment form
Unicorn with delicious cookie
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо