Fix handling of invalid UAT items

If the UAT file failed a field check, then the user_data pointer may be
empty. As a result uat_save() triggers an invalid write.

(Discovered while working with a dfilter_macros file having duplicate
names for bug 10957, caught by ASAN.)

The second issue fixed in this patch is that the validity of an item is
only calculated when a new record is added. So even if the user edits
the UAT and makes the entry valid, it would not be saved. This is solved
by adding a new uat_update_record() function which got wires up into GTK
and Qt.

Some open-coded g_array_index and UAT[_USER]_INDEX_PTR are also
converted.

Even after this patch, Qt has some issues with UAT handling. In
particular, it saves new, but empty/invalid, items. It also it does not
check individual fields when saving all fields (unlike Gtk). This patch
focused on getting Gtk fixed first so ignores those existing issues.

Change-Id: Ia35cfe9d2b793c65144ae7e29a1ed706b6668d99
Reviewed-on: https://code.wireshark.org/review/7120
Petri-Dish: Michael Mann <mmann78@netscape.net>
Reviewed-by: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Michael Mann <mmann78@netscape.net>

debian/libwireshark0.symbols

@@ -1436,6 +1436,7 @@ libwireshark.so.0 libwireshark0 #MINVER#
  uat_remove_record_idx@Base 1.9.1
  uat_save@Base 1.9.1
  uat_swap@Base 1.9.1
+ uat_update_record@Base 1.99.3
  udp_port_to_display@Base 1.99.2
  union_of_tap_listener_flags@Base 1.9.1
  update_crc10_by_bytes_tvb@Base 1.99.0

epan/uat-int.h

@@ -84,6 +84,9 @@ void uat_reset(void);
 WS_DLL_PUBLIC
 void* uat_add_record(uat_t*, const void* orig_rec_ptr, gboolean valid_rec);
 
+WS_DLL_PUBLIC
+void uat_update_record(uat_t *uat, const void *data, gboolean valid_rec);
+
 WS_DLL_PUBLIC
 void uat_swap(uat_t*, guint idx_a, guint idx_b);
 

epan/uat.c

@@ -129,7 +129,7 @@ void* uat_add_record(uat_t* uat, const void* data, gboolean valid_rec) {
     /* Save a copy of the raw (possibly that may contain invalid field values) data */
     g_array_append_vals (uat->raw_data, data, 1);
 
-    rec = uat->raw_data->data + (uat->record_size * (uat->raw_data->len-1));
+    rec = UAT_INDEX_PTR(uat, uat->raw_data->len - 1);
 
     if (uat->copy_cb) {
         uat->copy_cb(rec, data, (unsigned int) uat->record_size);
@@ -139,7 +139,7 @@ void* uat_add_record(uat_t* uat, const void* data, gboolean valid_rec) {
         /* Add a "known good" record to the list to be used by the dissector */
         g_array_append_vals (uat->user_data, data, 1);
 
-        rec = uat->user_data->data + (uat->record_size * (uat->user_data->len-1));
+        rec = UAT_USER_INDEX_PTR(uat, uat->user_data->len - 1);
 
         if (uat->copy_cb) {
             uat->copy_cb(rec, data, (unsigned int) uat->record_size);
@@ -151,12 +151,32 @@ void* uat_add_record(uat_t* uat, const void* data, gboolean valid_rec) {
     }
 
     g_array_append_vals (uat->valid_data, &valid_rec, 1);
-    valid = (gboolean*)(uat->valid_data->data + (sizeof(gboolean) * (uat->valid_data->len-1)));
+    valid = &g_array_index(uat->valid_data, gboolean, uat->valid_data->len-1);
     *valid = valid_rec;
 
     return rec;
 }
 
+/* Updates the validity of a record. */
+void uat_update_record(uat_t *uat, const void *data, gboolean valid_rec) {
+    guint pos;
+    gboolean *valid;
+
+    /* Locate internal UAT data pointer. */
+    for (pos = 0; pos < uat->raw_data->len; pos++) {
+        if (UAT_INDEX_PTR(uat, pos) == data) {
+            break;
+        }
+    }
+    if (pos == uat->raw_data->len) {
+        /* Data is not within list?! */
+        g_assert_not_reached();
+    }
+
+    valid = &g_array_index(uat->valid_data, gboolean, pos);
+    *valid = valid_rec;
+}
+
 void uat_swap(uat_t* uat, guint a, guint b) {
     size_t s = uat->record_size;
     void* tmp;
@@ -316,12 +336,13 @@ gboolean uat_save(uat_t* uat, char** error) {
 
     /* Now copy "good" raw_data entries to user_data */
     for ( i = 0 ; i < uat->raw_data->len ; i++ ) {
-        void* rec = uat->raw_data->data + (uat->record_size * i);
+        void *rec = UAT_INDEX_PTR(uat, i);
         gboolean* valid = (gboolean*)(uat->valid_data->data + sizeof(gboolean)*i);
         if (*valid) {
             g_array_append_vals(uat->user_data, rec, 1);
             if (uat->copy_cb) {
-                uat->copy_cb(UAT_USER_INDEX_PTR(uat,i), rec, (unsigned int) uat->record_size);
+                uat->copy_cb(UAT_USER_INDEX_PTR(uat, uat->user_data->len - 1),
+                             rec, (unsigned int) uat->record_size);
             }
 
             UAT_UPDATE(uat);

ui/gtk/uat_gui.c

@@ -394,6 +394,8 @@ static gboolean uat_dlg_cb(GtkWidget *win _U_, gpointer user_data) {
 		}
 
 		g_free(rec_tmp);
+	} else {
+		uat_update_record(dd->uat, dd->rec, TRUE);
 	}
 
 	dd->uat->changed = TRUE;

ui/qt/uat_dialog.cpp

@@ -380,10 +380,12 @@ void UatDialog::enumPrefCurrentIndexChanged(int index)
     if (field->cb.chk && field->cb.chk(rec, enum_txt.constData(), (unsigned) enum_txt.size(), field->cbdata.chk, field->fld_data, &err)) {
         field->cb.set(rec, enum_txt.constData(), (unsigned) enum_txt.size(), field->cbdata.set, field->fld_data);
         ok_button_->setEnabled(true);
+        uat_update_record(uat_, rec, TRUE);
     } else {
         /* XXX - do something useful with the error message string */
         g_free(err);
         ok_button_->setEnabled(false);
+        uat_update_record(uat_, rec, FALSE);
     }
     uat_->changed = TRUE;
 }
@@ -408,11 +410,13 @@ void UatDialog::stringPrefTextChanged(const QString &text)
             field->cb.set(rec, txt.constData(), (unsigned) txt.size(), field->cbdata.set, field->fld_data);
             saved_string_pref_ = text;
             ss = SyntaxLineEdit::Valid;
+            uat_update_record(uat_, rec, TRUE);
         } else {
             /* XXX - do something useful with the error message string */
             g_free(err);
             enable_ok = false;
             ss = SyntaxLineEdit::Invalid;
+            uat_update_record(uat_, rec, FALSE);
         }
     }