Geekly articles weekly

Wireshark 3.x：macOS下的代码分析和错误检查

图片1

Wireshark Foundation发布了流行的网络流量分析器的最终稳定版本-Wireshark 3.0.0。 新版本修复了多个错误，除了替换了Npcap WinPcap上的驱动程序之外，现在还可以分析新协议。 这是声明引用结束的地方，我们有关项目中错误的注释开始了。 这些项目的作者肯定在发布之前未尽最大努力来修复错误。

现在让我们收集修补程序，以提供进行新发行版的动机:)。

引言


Wireshark是捕获和分析网络流量的著名工具。 该程序可与绝大多数已知协议一起使用，具有直观和逻辑的图形界面，以及功能强大的过滤器系统。 Wireshark是跨平台的，可在以下操作系统中运行：Windows，Linux，macOS，Solaris，FreeBSD，NetBSD等。

为了进行源代码分析，我们使用了PVS-Studio静态代码分析器。 为了分析源代码，首先我们需要在OS中编译项目。 选择的范围之广不仅是由于项目的跨平台性质，还因为分析仪的性质。 我选择了macOS进行分析。 您也可以在Windows和Linux下运行分析仪。

我想特别注意代码质量。 不幸的是，我不能对此大加赞赏。 这是一个主观评估，但是由于我们会定期检查大量项目，因此我有一个参考框架。 在这种情况下，最突出的是针对少量代码的大量PVS-Studio警告。 总共为该项目触发了3500多个级别的警告。 对于项目而言，这是典型的情况，这些项目通常不使用静态分析工具，甚至不使用免费工具。 指示项目质量的另一个因素是分析仪检测到的重复错误。 我不会引用相同类型的代码示例，而在数百个地方会发生一些类似的错误。

这样的插入也不会提高代码质量：

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

整个项目中有1000多个。 这样的插入会使分析仪更难将发出的警告与适当的文件进行匹配。 好吧，我认为一般开发人员都不会从维护此类代码中受益匪浅。

错别字


警告1

V641分配的内存缓冲区的大小不是元素大小的倍数。 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为“ HDR_TCP.dest_port”变量连续两次分配值。 也许这是一个错误。 检查行：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表达式“ direction == 0”始终为false。 包-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; } .... } .... }

在外部条件下，将方向变量与常数P2P_DIR_RECV比较。 根据用AND运算符编写的表达式，当到达内部条件时，可变方向的值肯定会与另一个常量P2P_DIR_SENT不同。

警告2

V590考虑检查'（type == 0x1）|| （type！= 0x4）'表达式。 表达式过多或打印错误。 包-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考虑检查此表达式。 表达式过多或打印错误。 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] ==''） 。 校验（源[offset]！='\ 0'）是多余的，可以安全地删除。 这不是实际的错误，但是冗余代码使代码阅读和理解程序更加困难，因此最好对其进行简化。

警告4

V547表达式“ eras_pos！= NULL”始终为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表达式“ sub_dissectors！= NULL”始终为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表达式'i <count'始终为true。 数据包网络流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在针对nullptr进行验证之前，已使用“ si-> conv”指针。 检查行：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释放内存后使用了“原型”指针。 数据包-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'ptmpstr'指针被分配了两次值，而没有释放内存。 可能发生内存泄漏。 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'ptmpstr'指针被分配了两次值，而没有释放内存。 可能发生内存泄漏。 idl2wrs.c 2447
  • V773'ptmpstr'指针被分配了两次值，而没有释放内存。 可能发生内存泄漏。 idl2wrs.c 2713
  • V773'ptmpstr'指针被分配了两次值，而没有释放内存。 可能发生内存泄漏。 idl2wrs.c 2728
  • V773'ptmpstr'指针被分配了两次值，而没有释放内存。 可能发生内存泄漏。 idl2wrs.c 2732
  • V773'ptmpstr'指针被分配了两次值，而没有释放内存。 可能发生内存泄漏。 idl2wrs.c 2745

不幸的是，在代码中还有许多其他类似的情况，其中内存被释放。

杂项


警告1

V535变量“ i”用于该循环和外部循环。 检查行：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参数“ item”在使用前总是在功能体中重写。 包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); .... }

该函数获取的项目指针将立即更改为另一个值。 非常可疑。 而且，代码中包含数十个此类位置，因此很难确定它是否是错误。 我在另一个大型项目中遇到过类似的代码，这种代码在这里是正确的，没有人敢于更改函数的接口。

警告3

V762虚函数可能被错误地覆盖。 请参见派生类“ PacketListModel”和基类“ QAbstractItemModel”中函数“ headerData”的第三个参数。 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未定义的行为。 检查移位运算符“ >>”。 右操作数（'bitshift'= [0..64]）大于或等于提升后的左操作数的位长度。 协议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分析仪。

Source: https://habr.com/ru/post/zh-CN447156/

More articles:


All Articles