From ea167053ffc553b3a5f4ce6cbe0b78ecc8cd0dbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stig=20Bj=C3=B8rlykke?= Date: Thu, 6 Nov 2014 11:19:25 +0100 Subject: [PATCH] Improved deregistering fields. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This improvement avoids use of deallocated memory (crash) if using a deregistered field in display filter, color filter, custom column and other cases when the field is used as "interesting field". This functionality is currently used in http, imf and ldap preferences. Also removed unused proto_registrar_n() as this does not work correctly after deregistering fields. Change-Id: I043e3bf7a98bd773c9801e712a012d1eab8a7f94 Reviewed-on: https://code.wireshark.org/review/5161 Reviewed-by: Stig Bjørlykke Tested-by: Stig Bjørlykke --- asn1/ldap/packet-ldap-template.c | 6 +-- debian/libwireshark0.symbols | 3 +- epan/dissectors/packet-http.c | 6 +-- epan/dissectors/packet-imf.c | 6 +-- epan/dissectors/packet-ldap.c | 14 +++---- epan/proto.c | 70 +++++++++++++++++++++++++++++--- epan/proto.h | 15 ++++--- ui/gtk/main.c | 33 +++++++++++++++ ui/gtk/main.h | 3 ++ ui/gtk/uat_gui.c | 2 +- ui/qt/main_window.cpp | 2 + ui/qt/main_window.h | 1 + ui/qt/main_window_slots.cpp | 30 ++++++++++++++ ui/qt/uat_dialog.cpp | 2 +- ui/qt/wireshark_application.cpp | 3 ++ ui/qt/wireshark_application.h | 4 +- 16 files changed, 161 insertions(+), 39 deletions(-) diff --git a/asn1/ldap/packet-ldap-template.c b/asn1/ldap/packet-ldap-template.c index d15715be78..9c587c5381 100644 --- a/asn1/ldap/packet-ldap-template.c +++ b/asn1/ldap/packet-ldap-template.c @@ -496,14 +496,10 @@ attribute_types_initialize_cb(void) /* Unregister all fields */ for (i = 0; i < hf_size; i++) { proto_unregister_field (proto_ldap, *(hf[i].p_id)); - g_free (hf[i].p_id); - g_free ((char *) hf[i].hfinfo.name); - g_free ((char *) hf[i].hfinfo.abbrev); - g_free ((char *) hf[i].hfinfo.blurb); } g_hash_table_destroy (attribute_types_hash); - g_free (hf); + proto_add_deregistered_data (hf); attribute_types_hash = NULL; } diff --git a/debian/libwireshark0.symbols b/debian/libwireshark0.symbols index 3c329a7308..78bd66fc85 100644 --- a/debian/libwireshark0.symbols +++ b/debian/libwireshark0.symbols @@ -867,6 +867,7 @@ libwireshark.so.0 libwireshark0 #MINVER# process_reassembled_data@Base 1.9.1 process_stat_cmd_arg@Base 1.9.1 proto_all_finfos@Base 1.9.1 + proto_add_deregistered_data@Base 1.12.0~rc1 proto_can_match_selected@Base 1.9.1 proto_can_toggle_protocol@Base 1.9.1 proto_check_field_name@Base 1.9.1 @@ -878,6 +879,7 @@ libwireshark.so.0 libwireshark0 #MINVER# proto_find_field_from_offset@Base 1.9.1 proto_find_finfo@Base 1.9.1 proto_frame@Base 1.9.1 + proto_free_deregistered_fields@Base 1.12.0~rc1 proto_get_data_protocol@Base 1.9.1 proto_get_finfo_ptr_array@Base 1.9.1 proto_get_first_protocol@Base 1.9.1 @@ -924,7 +926,6 @@ libwireshark.so.0 libwireshark0 #MINVER# proto_registrar_get_nth@Base 1.9.1 proto_registrar_get_parent@Base 1.9.1 proto_registrar_is_protocol@Base 1.9.1 - proto_registrar_n@Base 1.9.1 proto_report_dissector_bug@Base 1.12.0~rc1 proto_set_cant_toggle@Base 1.9.1 proto_set_decoding@Base 1.9.1 diff --git a/epan/dissectors/packet-http.c b/epan/dissectors/packet-http.c index 2d6ade2e42..6addc14986 100644 --- a/epan/dissectors/packet-http.c +++ b/epan/dissectors/packet-http.c @@ -2315,14 +2315,10 @@ header_fields_initialize_cb(void) /* Unregister all fields */ for (i = 0; i < hf_size; i++) { proto_unregister_field (proto_http, *(hf[i].p_id)); - g_free (hf[i].p_id); - g_free ((char *) hf[i].hfinfo.name); - g_free ((char *) hf[i].hfinfo.abbrev); - g_free ((char *) hf[i].hfinfo.blurb); } g_hash_table_destroy (header_fields_hash); - g_free (hf); + proto_add_deregistered_data (hf); header_fields_hash = NULL; } diff --git a/epan/dissectors/packet-imf.c b/epan/dissectors/packet-imf.c index 3cdf42cf19..2747c086c7 100644 --- a/epan/dissectors/packet-imf.c +++ b/epan/dissectors/packet-imf.c @@ -868,14 +868,10 @@ header_fields_initialize_cb (void) /* Unregister all fields */ for (i = 0; i < hf_size; i++) { proto_unregister_field (proto_imf, *(hf[i].p_id)); - g_free (hf[i].p_id); - g_free ((char *) hf[i].hfinfo.name); - g_free ((char *) hf[i].hfinfo.abbrev); - g_free ((char *) hf[i].hfinfo.blurb); } g_hash_table_destroy (custom_field_table); - g_free (hf); + proto_add_deregistered_data (hf); custom_field_table = NULL; } diff --git a/epan/dissectors/packet-ldap.c b/epan/dissectors/packet-ldap.c index 6927b1fc92..fb9f821907 100644 --- a/epan/dissectors/packet-ldap.c +++ b/epan/dissectors/packet-ldap.c @@ -710,14 +710,10 @@ attribute_types_initialize_cb(void) /* Unregister all fields */ for (i = 0; i < hf_size; i++) { proto_unregister_field (proto_ldap, *(hf[i].p_id)); - g_free (hf[i].p_id); - g_free ((char *) hf[i].hfinfo.name); - g_free ((char *) hf[i].hfinfo.abbrev); - g_free ((char *) hf[i].hfinfo.blurb); } g_hash_table_destroy (attribute_types_hash); - g_free (hf); + proto_add_deregistered_data (hf); attribute_types_hash = NULL; } @@ -3824,7 +3820,7 @@ static int dissect_PasswordPolicyResponseValue_PDU(tvbuff_t *tvb _U_, packet_inf /*--- End of included file: packet-ldap-fn.c ---*/ -#line 890 "../../asn1/ldap/packet-ldap-template.c" +#line 886 "../../asn1/ldap/packet-ldap-template.c" static int dissect_LDAPMessage_PDU(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, ldap_conv_info_t *ldap_info) { int offset = 0; @@ -5740,7 +5736,7 @@ void proto_register_ldap(void) { NULL, HFILL }}, /*--- End of included file: packet-ldap-hfarr.c ---*/ -#line 2237 "../../asn1/ldap/packet-ldap-template.c" +#line 2233 "../../asn1/ldap/packet-ldap-template.c" }; /* List of subtrees */ @@ -5814,7 +5810,7 @@ void proto_register_ldap(void) { &ett_ldap_T_warning, /*--- End of included file: packet-ldap-ettarr.c ---*/ -#line 2251 "../../asn1/ldap/packet-ldap-template.c" +#line 2247 "../../asn1/ldap/packet-ldap-template.c" }; /* UAT for header fields */ static uat_field_t custom_attribute_types_uat_fields[] = { @@ -5980,7 +5976,7 @@ proto_reg_handoff_ldap(void) /*--- End of included file: packet-ldap-dis-tab.c ---*/ -#line 2400 "../../asn1/ldap/packet-ldap-template.c" +#line 2396 "../../asn1/ldap/packet-ldap-template.c" } diff --git a/epan/proto.c b/epan/proto.c index be4286ec79..9c42036245 100644 --- a/epan/proto.c +++ b/epan/proto.c @@ -288,6 +288,10 @@ struct _protocol { /* List of all protocols */ static GList *protocols = NULL; +/* Deregistered fields */ +static GPtrArray *deregistered_fields = NULL; +static GPtrArray *deregistered_data = NULL; + /* Contains information about a field when a dissector calls * proto_tree_add_item. */ #define FIELD_INFO_NEW(pool, fi) fi = wmem_new(pool, field_info) @@ -453,6 +457,8 @@ proto_init(void (register_all_protocols_func)(register_cb cb, gpointer client_da gpa_hfinfo.allocated_len = 0; gpa_hfinfo.hfi = NULL; gpa_name_map = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, save_same_name_hfinfo); + deregistered_fields = g_ptr_array_new(); + deregistered_data = g_ptr_array_new(); /* Initialize the ftype subsystem */ ftypes_initialize(); @@ -544,6 +550,17 @@ proto_cleanup(void) g_free(gpa_hfinfo.hfi); gpa_hfinfo.hfi = NULL; } + + if (deregistered_fields) { + g_ptr_array_free(deregistered_fields, FALSE); + deregistered_fields = NULL; + } + + if (deregistered_data) { + g_ptr_array_free(deregistered_data, FALSE); + deregistered_data = NULL; + } + g_free(tree_is_expanded); tree_is_expanded = NULL; } @@ -5293,11 +5310,52 @@ proto_unregister_field (const int parent, gint hf_id) /* Found the hf_id in this protocol */ g_hash_table_steal(gpa_name_map, hfi->abbrev); g_ptr_array_remove_index_fast(proto->fields, i); + g_ptr_array_add(deregistered_fields, gpa_hfinfo.hfi[hf_id]); return; } } } +void +proto_add_deregistered_data (void *data) +{ + g_ptr_array_add(deregistered_data, data); +} + +static void +free_deregistered_field (gpointer data, gpointer user_data _U_) +{ + header_field_info *hfi = (header_field_info *) data; + gint hf_id = hfi->id; + + g_free((char *)hfi->name); + g_free((char *)hfi->abbrev); + g_free((char *)hfi->blurb); + if (hfi->parent == -1) + g_slice_free(header_field_info, hfi); + + gpa_hfinfo.hfi[hf_id] = NULL; /* Invalidate this hf_id / proto_id */ +} + +static void +free_deregistered_data (gpointer data, gpointer user_data _U_) +{ + g_free (data); +} + +/* free deregistered fields and data */ +void +proto_free_deregistered_fields (void) +{ + g_ptr_array_foreach(deregistered_fields, free_deregistered_field, NULL); + g_ptr_array_free(deregistered_fields, TRUE); + deregistered_fields = g_ptr_array_new(); + + g_ptr_array_foreach(deregistered_data, free_deregistered_data, NULL); + g_ptr_array_free(deregistered_data, TRUE); + deregistered_data = g_ptr_array_new(); +} + /* chars allowed in field abbrev */ static const guint8 fld_abbrev_chars[256] = { @@ -6480,12 +6538,6 @@ hfinfo_int64_format(const header_field_info *hfinfo) return format; } -int -proto_registrar_n(void) -{ - return gpa_hfinfo.len; -} - const char * proto_registrar_get_name(const int n) { @@ -6787,6 +6839,9 @@ proto_registrar_dump_values(void) len = gpa_hfinfo.len; for (i = 0; i < len ; i++) { + if (gpa_hfinfo.hfi[i] == NULL) + continue; /* This is a deregistered protocol or field */ + PROTO_REGISTRAR_GET_NTH(i, hfinfo); if (hfinfo->id == hf_text_only) { @@ -6959,6 +7014,9 @@ proto_registrar_dump_fields(void) len = gpa_hfinfo.len; for (i = 0; i < len ; i++) { + if (gpa_hfinfo.hfi[i] == NULL) + continue; /* This is a deregistered protocol or header field */ + PROTO_REGISTRAR_GET_NTH(i, hfinfo); /* diff --git a/epan/proto.h b/epan/proto.h index 01d53506ab..70ffa5607d 100644 --- a/epan/proto.h +++ b/epan/proto.h @@ -1919,20 +1919,25 @@ proto_register_field_array(const int parent, hf_register_info *hf, const int num /** Unregister an already registered field. @param parent the protocol handle from proto_register_protocol() - @param hf_id the field to unregister */ + @param hf_id the field to deregister */ WS_DLL_PUBLIC void proto_unregister_field (const int parent, gint hf_id); +/** Add data to be freed when deregistered fields are freed. + @param data a pointer to data to free */ +WS_DLL_PUBLIC void +proto_add_deregistered_data (void *data); + +/** Free fields deregistered in proto_unregister_field(). */ +WS_DLL_PUBLIC void +proto_free_deregistered_fields (void); + /** Register a protocol subtree (ett) array. @param indices array of ett indices @param num_indices the number of records in indices */ WS_DLL_PUBLIC void proto_register_subtree_array(gint *const *indices, const int num_indices); -/** Returns number of items (protocols or header fields) registered. - @return the number of items */ -WS_DLL_PUBLIC int proto_registrar_n(void); - /** Get name of registered header_field number n. @param n item # n (0-indexed) @return the name of this registered item */ diff --git a/ui/gtk/main.c b/ui/gtk/main.c index 187f954c51..058418d1fc 100644 --- a/ui/gtk/main.c +++ b/ui/gtk/main.c @@ -69,6 +69,7 @@ #include #include #include +#include #include #include #include @@ -3823,6 +3824,38 @@ void change_configuration_profile (const gchar *profile_name) main_pane_load_window_geometry(); } +void +main_fields_changed (void) +{ + /* Reload color filters */ + color_filters_reload(); + + /* Syntax check filter */ + filter_te_syntax_check_cb(main_display_filter_widget, NULL); + if (cfile.dfilter) { + /* Check if filter is still valid */ + dfilter_t *dfp = NULL; + if (!dfilter_compile(cfile.dfilter, &dfp)) { + /* Not valid. Enable 'Apply' button and remove dfilter. */ + g_signal_emit_by_name(G_OBJECT(main_display_filter_widget), "changed"); + g_free(cfile.dfilter); + cfile.dfilter = NULL; + } + dfilter_free(dfp); + } + + if (have_custom_cols(&cfile.cinfo)) { + /* Recreate packet list according to new/changed/deleted fields */ + packet_list_recreate(); + } else if (cfile.state != FILE_CLOSED) { + /* Redissect packets if we have any */ + redissect_packets(); + } + destroy_packet_wins(); /* TODO: close windows until we can recreate */ + + proto_free_deregistered_fields(); +} + /** redissect packets and update UI */ void redissect_packets(void) { diff --git a/ui/gtk/main.h b/ui/gtk/main.h index 51730620dd..472b596be9 100644 --- a/ui/gtk/main.h +++ b/ui/gtk/main.h @@ -360,6 +360,9 @@ extern void create_console(void); /** Change configuration profile */ extern void change_configuration_profile(const gchar *profile_name); +/** Update GUI for changes in fields */ +extern void main_fields_changed (void); + /** redissect packets and update UI */ extern void redissect_packets(void); diff --git a/ui/gtk/uat_gui.c b/ui/gtk/uat_gui.c index 02c6d37447..9dd57caf17 100644 --- a/ui/gtk/uat_gui.c +++ b/ui/gtk/uat_gui.c @@ -777,7 +777,7 @@ static void uat_down_cb(GtkButton *button _U_, gpointer u) { static void uat_apply_changes(uat_t *uat) { if (uat->flags & UAT_AFFECTS_FIELDS) { /* Recreate list with new fields and redissect packets */ - packet_list_recreate (); + main_fields_changed (); } else { if (uat->flags & UAT_AFFECTS_DISSECTION) { /* Just redissect packets if we have any */ diff --git a/ui/qt/main_window.cpp b/ui/qt/main_window.cpp index 886b8395a4..2359ab75c0 100644 --- a/ui/qt/main_window.cpp +++ b/ui/qt/main_window.cpp @@ -308,6 +308,8 @@ MainWindow::MainWindow(QWidget *parent) : this, SLOT(filterExpressionsChanged())); connect(wsApp, SIGNAL(filterExpressionsChanged()), this, SLOT(filterExpressionsChanged())); + connect(wsApp, SIGNAL(fieldsChanged()), + this, SLOT(fieldsChanged())); connect(main_welcome_, SIGNAL(startCapture()), this, SLOT(startCapture())); diff --git a/ui/qt/main_window.h b/ui/qt/main_window.h index cef21d8ecb..2815347f83 100644 --- a/ui/qt/main_window.h +++ b/ui/qt/main_window.h @@ -224,6 +224,7 @@ private slots: void captureFilterSyntaxChanged(bool valid); void redissectPackets(); void recreatePacketList(); + void fieldsChanged(); void startInterfaceCapture(bool valid); diff --git a/ui/qt/main_window_slots.cpp b/ui/qt/main_window_slots.cpp index 211b89d4cc..caf635c3aa 100644 --- a/ui/qt/main_window_slots.cpp +++ b/ui/qt/main_window_slots.cpp @@ -1228,6 +1228,36 @@ void MainWindow::recreatePacketList() cfile.columns_changed = FALSE; /* Reset value */ } +void MainWindow::fieldsChanged() +{ + // Reload color filters + color_filters_reload(); + + // Syntax check filter + // TODO: Check if syntax filter is still valid after fields have changed + // and update background color. + if (cfile.dfilter) { + // Check if filter is still valid + dfilter_t *dfp = NULL; + if (!dfilter_compile(cfile.dfilter, &dfp)) { + // TODO: Not valid, enable "Apply" button. + g_free(cfile.dfilter); + cfile.dfilter = NULL; + } + dfilter_free(dfp); + } + + if (have_custom_cols(&cfile.cinfo)) { + /* Recreate packet list according to new/changed/deleted fields */ + recreatePacketList(); + } else if (cfile.state != FILE_CLOSED) { + /* Redissect packets if we have any */ + redissectPackets(); + } + + proto_free_deregistered_fields(); +} + void MainWindow::setFeaturesEnabled(bool enabled) { main_ui_->menuBar->setEnabled(enabled); diff --git a/ui/qt/uat_dialog.cpp b/ui/qt/uat_dialog.cpp index 918f8bd6c7..df1a364f74 100644 --- a/ui/qt/uat_dialog.cpp +++ b/ui/qt/uat_dialog.cpp @@ -471,7 +471,7 @@ void UatDialog::applyChanges() if (uat_->flags & UAT_AFFECTS_FIELDS) { /* Recreate list with new fields and redissect packets */ - wsApp->emitAppSignal(WiresharkApplication::ColumnsChanged); + wsApp->emitAppSignal(WiresharkApplication::FieldsChanged); } if (uat_->flags & UAT_AFFECTS_DISSECTION) { /* Just redissect packets if we have any */ diff --git a/ui/qt/wireshark_application.cpp b/ui/qt/wireshark_application.cpp index 9f04a6ef47..83b8a8de4f 100644 --- a/ui/qt/wireshark_application.cpp +++ b/ui/qt/wireshark_application.cpp @@ -635,6 +635,9 @@ void WiresharkApplication::emitAppSignal(AppSignal signal) case StaticRecentFilesRead: emit recentFilesRead(); break; + case FieldsChanged: + emit fieldsChanged(); + break; default: break; } diff --git a/ui/qt/wireshark_application.h b/ui/qt/wireshark_application.h index a6764cf137..981dc8e74c 100644 --- a/ui/qt/wireshark_application.h +++ b/ui/qt/wireshark_application.h @@ -70,7 +70,8 @@ public: FilterExpressionsChanged, PacketDissectionChanged, PreferencesChanged, - StaticRecentFilesRead + StaticRecentFilesRead, + FieldsChanged }; void registerUpdate(register_action_e action, const char *message); @@ -127,6 +128,7 @@ signals: void packetDissectionChanged(); void preferencesChanged(); void addressResolutionChanged(); + void fieldsChanged(); // XXX It might make more sense to move these to main.cpp or main_window.cpp or their own class. void captureCapturePrepared(capture_session *cap_session);