Improved deregistering fields.

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 <stig@bjorlykke.org>
Tested-by: Stig Bjørlykke <stig@bjorlykke.org>

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;
   }
 

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

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;
 	}
 

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;
   }
 

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"
 
 
 }

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);
 
 		/*

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 */

ui/gtk/main.c

@@ -69,6 +69,7 @@
 #include <epan/column.h>
 #include <epan/disabled_protos.h>
 #include <epan/epan.h>
+#include <epan/proto.h>
 #include <epan/epan_dissect.h>
 #include <epan/dfilter/dfilter.h>
 #include <epan/strutil.h>
@@ -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)
 {

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);
 

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 */

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()));

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);
 

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);

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 */

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;
     }

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);