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

    Picture 1

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

    Введение


    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.



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Wireshark 3.x: code analysis under macOS and errors review
    • +35
    • 5,7k
    • 9
    PVS-Studio
    205,00
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Комментарии 9

      +1
      Когда уже будет анализ кода IntelliJ IDEA Community? А если я пропустил, то ткните в ссылку пожалуйста.
        +2
        Вот статья: PVS-Studio для Java.
          0
          Ага, спасибо
            0
            Я почему-то понял это
            Когда уже будет анализ кода IntelliJ IDEA Community?
            как — когда вы проанализируете community версию IntelliJ IDEA?
            Вроде бы она open source.
              +1
              Так по ссылке проанализировано же.
                0
                «Извините, был напуган.»
          –3
          Отправляете ли вы багрепорты владельцам проектов?
          –1

          А вот с циклами это за гранию добры и злы. Цопипаства…

          Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

          Самое читаемое