From e449b560c02d363603224a7558758eb7da0c6a73 Mon Sep 17 00:00:00 2001 From: John Thacker Date: Wed, 2 Nov 2022 09:11:30 -0400 Subject: [PATCH] epan: Properly generate filter expressions for custom columns Properly generate filter expressions for custom columns by using proto_construct_match_selected_string on each value and then joining them together later instead of trying to split the column expression value. This ensures that escaping is done properly for display filter strings, that commas internal to field values are not confused with commas between occurrences, that for multifield columns we can distinguish which field each value matches, etc. It's not entirely clear whether AND or OR logic is appropriate for multiple occurrences; currently OR is used. Bump glib requirement to 2.54 for g_ptr_array_find_with_equal_func (this doesn't drop support for any major distribution that already meets our other library requirements, like Qt.) Fix #18001. --- CMakeLists.txt | 2 +- epan/column-info.h | 7 ++- epan/column-utils.c | 19 ++++++++ epan/proto.c | 102 ++++++++++++++++++++++++++++++++++++++++++ epan/proto.h | 12 ++++- ui/qt/packet_list.cpp | 51 +++++---------------- 6 files changed, 149 insertions(+), 44 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index aa348b1e4f..882b586959 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1142,7 +1142,7 @@ endmacro() # The minimum package list find_package(Git) reset_find_package(GLIB2 GLIB2_MAIN_INCLUDE_DIR GLIB2_INTERNAL_INCLUDE_DIR) -find_package(GLIB2 "2.50.0" REQUIRED) +find_package(GLIB2 "2.54.0" REQUIRED) include_directories(SYSTEM ${GLIB2_INCLUDE_DIRS}) reset_find_package(GMODULE2) find_package(GMODULE2) diff --git a/epan/column-info.h b/epan/column-info.h index 89fb024b22..85cccb16ae 100644 --- a/epan/column-info.h +++ b/epan/column-info.h @@ -75,7 +75,7 @@ extern void col_init(column_info *cinfo, const struct epan_session *epan); */ WS_DLL_PUBLIC void col_fill_in_frame_data(const frame_data *fd, column_info *cinfo, const gint col, gboolean const fill_col_exprs); -/** Fill in all columns of the given packet. +/** Fill in all (non-custom) columns of the given packet. */ WS_DLL_PUBLIC void col_fill_in(packet_info *pinfo, const gboolean fill_col_exprs, const gboolean fill_fd_colums); @@ -94,6 +94,11 @@ void col_custom_set_edt(struct epan_dissect *edt, column_info *cinfo); WS_DLL_PUBLIC void col_custom_prime_edt(struct epan_dissect *edt, column_info *cinfo); +/** Get a filter expression for a custom column. This string must be g_free'd. + */ +WS_DLL_PUBLIC +char* col_custom_get_filter(struct epan_dissect *edt, column_info *cinfo, const gint col); + WS_DLL_PUBLIC gboolean have_custom_cols(column_info *cinfo); diff --git a/epan/column-utils.c b/epan/column-utils.c index 62e1c423f0..a07e8cced3 100644 --- a/epan/column-utils.c +++ b/epan/column-utils.c @@ -383,6 +383,25 @@ col_custom_prime_edt(epan_dissect_t *edt, column_info *cinfo) } } +char* +col_custom_get_filter(epan_dissect_t *edt, column_info *cinfo, const gint col) +{ + col_item_t* col_item; + + ws_assert(cinfo); + ws_assert(col < cinfo->num_cols); + + col_item = &cinfo->columns[col]; + if (col_item->fmt_matx[COL_CUSTOM] && + col_item->col_custom_fields && + col_item->col_custom_fields_ids) { + + return proto_custom_get_filter(edt, col_item->col_custom_fields_ids, + col_item->col_custom_occurrence); + } + return NULL; +} + void col_append_lstr(column_info *cinfo, const gint el, const gchar *str1, ...) { diff --git a/epan/proto.c b/epan/proto.c index 3a751258e2..d04e63d6af 100644 --- a/epan/proto.c +++ b/epan/proto.c @@ -7006,6 +7006,108 @@ proto_custom_set(proto_tree* tree, GSList *field_ids, gint occurrence, return abbrev ? abbrev : ""; } +gchar * +proto_custom_get_filter(epan_dissect_t* edt, GSList *field_ids, gint occurrence) +{ + int len, prev_len, last, i; + GPtrArray *finfos; + field_info *finfo = NULL; + header_field_info* hfinfo; + + char *filter = NULL; + GPtrArray *filter_array; + + int field_id; + + ws_assert(field_ids != NULL); + filter_array = g_ptr_array_new_full(g_slist_length(field_ids), g_free); + for (GSList *iter = field_ids; iter; iter = iter->next) { + field_id = *(int *)iter->data; + PROTO_REGISTRAR_GET_NTH((guint)field_id, hfinfo); + + /* do we need to rewind ? */ + if (!hfinfo) + return NULL; + + if (occurrence < 0) { + /* Search other direction */ + while (hfinfo->same_name_prev_id != -1) { + PROTO_REGISTRAR_GET_NTH(hfinfo->same_name_prev_id, hfinfo); + } + } + + prev_len = 0; /* Reset handled occurrences */ + + while (hfinfo) { + finfos = proto_get_finfo_ptr_array(edt->tree, hfinfo->id); + + if (!finfos || !(len = g_ptr_array_len(finfos))) { + if (occurrence < 0) { + hfinfo = hfinfo->same_name_next; + } else { + hfinfo = hfinfo_same_name_get_prev(hfinfo); + } + continue; + } + + /* Are there enough occurrences of the field? */ + if (((occurrence - prev_len) > len) || ((occurrence + prev_len) < -len)) { + if (occurrence < 0) { + hfinfo = hfinfo->same_name_next; + } else { + hfinfo = hfinfo_same_name_get_prev(hfinfo); + } + prev_len += len; + continue; + } + + /* Calculate single index or set outer bounderies */ + if (occurrence < 0) { + i = occurrence + len + prev_len; + last = i; + } else if (occurrence > 0) { + i = occurrence - 1 - prev_len; + last = i; + } else { + i = 0; + last = len - 1; + } + + prev_len += len; /* Count handled occurrences */ + + while (i <= last) { + finfo = (field_info *)g_ptr_array_index(finfos, i); + + filter = proto_construct_match_selected_string(finfo, edt); + if (filter) { + /* Only add the same expression once (especially for FT_PROTOCOL). + * The ptr array doesn't have NULL entries so g_str_equal is fine. + */ + if (!g_ptr_array_find_with_equal_func(filter_array, filter, g_str_equal, NULL)) { + g_ptr_array_add(filter_array, filter); + } + } + i++; + } + + if (occurrence == 0) { + /* Fetch next hfinfo with same name (abbrev) */ + hfinfo = hfinfo_same_name_get_prev(hfinfo); + } else { + hfinfo = NULL; + } + } + } + + g_ptr_array_add(filter_array, NULL); + + /* XXX: Should this be || or && ? */ + gchar *output = g_strjoinv(" || ", (char **)filter_array->pdata); + + g_ptr_array_free(filter_array, TRUE); + + return output; +} /* Set text of proto_item after having already been created. */ void diff --git a/epan/proto.h b/epan/proto.h index c75bd1262c..4336ccc6cd 100644 --- a/epan/proto.h +++ b/epan/proto.h @@ -3349,9 +3349,9 @@ WS_DLL_PUBLIC guchar proto_check_field_name_lower(const gchar *field_name); -/** Check if given string is a valid field name +/** Set the column text for a custom column @param tree the tree to append this item to - @param field_id the field id used for custom column + @param field_id the field ids used for custom column @param occurrence the occurrence of the field used for custom column @param result the buffer to fill with the field string @param expr the filter expression @@ -3362,6 +3362,14 @@ proto_custom_set(proto_tree* tree, GSList *field_id, gchar *result, gchar *expr, const int size ); +/** Construct a display filter string for a custom column + @param edt epan dissecting + @param field_id the field ids used for custom column + @param occurrence the occurrence of the field used for custom column + @return allocated display filter string. Needs to be freed with g_free(...) */ +gchar * +proto_custom_get_filter(struct epan_dissect *edt, GSList *field_id, gint occurrence); + /** @} */ const char * diff --git a/ui/qt/packet_list.cpp b/ui/qt/packet_list.cpp index eb6602af5f..4deecfc9bc 100644 --- a/ui/qt/packet_list.cpp +++ b/ui/qt/packet_list.cpp @@ -1314,54 +1314,25 @@ QString PacketList::getFilterFromRowAndColumn(QModelIndex idx) epan_dissect_run(&edt, cap_file_->cd_t, &rec, frame_tvbuff_new_buffer(&cap_file_->provider, fdata, &buf), fdata, &cap_file_->cinfo); - epan_dissect_fill_in_columns(&edt, TRUE, TRUE); - if ((cap_file_->cinfo.columns[column].col_custom_fields_ids == NULL) || - (g_slist_length(cap_file_->cinfo.columns[column].col_custom_fields_ids) == 1)) - { - /* Don't construct a filter on multifield custom columns, because - * we don't have a good reference for which values were found by - * which field. Fixing that requires changing logic in several - * places in the code (perhaps making col_expr_t a linked list?) + if (cap_file_->cinfo.columns[column].col_fmt == COL_CUSTOM) { + filter.append(gchar_free_to_qstring(col_custom_get_filter(&edt, &cap_file_->cinfo, column))); + } else { + /* We don't need to fill in the custom columns, as we get their + * filters above. */ + col_fill_in(&edt.pi, TRUE, TRUE); if (strlen(cap_file_->cinfo.col_expr.col_expr[column]) != 0 && strlen(cap_file_->cinfo.col_expr.col_expr_val[column]) != 0) { gboolean is_string_value = FALSE; - gboolean is_multiple_values = (strchr (cap_file_->cinfo.col_expr.col_expr_val[column], ',') != NULL); - if (cap_file_->cinfo.columns[column].col_fmt == COL_CUSTOM) { - header_field_info *hfi = proto_registrar_get_byname(cap_file_->cinfo.columns[column].col_custom_fields); - if (hfi && hfi->parent == -1) { - /* Protocol only */ - filter.append(cap_file_->cinfo.col_expr.col_expr[column]); - } else if (hfi && hfi->type == FT_STRING) { - /* Custom string, add quotes */ - is_string_value = TRUE; - } - } else { - header_field_info *hfi = proto_registrar_get_byname(cap_file_->cinfo.col_expr.col_expr[column]); - if (hfi && hfi->type == FT_STRING) { - /* Could be an address type such as usb.src which must be quoted. */ - is_string_value = TRUE; - } + header_field_info *hfi = proto_registrar_get_byname(cap_file_->cinfo.col_expr.col_expr[column]); + if (hfi && hfi->type == FT_STRING) { + /* Could be an address type such as usb.src which must be quoted. */ + is_string_value = TRUE; } if (filter.isEmpty()) { - if (is_multiple_values) { - /* Use the membership operator and find packets that have - * at least one matching value. Not clear if this (which - * is equivalent to OR) makes more sense than AND matching - * logic, but it's easy to construct. - */ - if (is_string_value) { - filter.append(QString("%1 in {\"%2\"}") - .arg(cap_file_->cinfo.col_expr.col_expr[column]) - .arg(cap_file_->cinfo.col_expr.col_expr_val[column]).split(",").join("\",\"")); - } else { - filter.append(QString("%1 in {%2}") - .arg(cap_file_->cinfo.col_expr.col_expr[column]) - .arg(cap_file_->cinfo.col_expr.col_expr_val[column])); - } - } else if (is_string_value) { + if (is_string_value) { filter.append(QString("%1 == \"%2\"") .arg(cap_file_->cinfo.col_expr.col_expr[column]) .arg(cap_file_->cinfo.col_expr.col_expr_val[column]));