From 4f3646fe62244f7aba3f309c509c3414f1028650 Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Sat, 14 Feb 2015 15:36:09 +0100 Subject: [PATCH] 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 Reviewed-by: Peter Wu Tested-by: Petri Dish Buildbot Reviewed-by: Michael Mann --- debian/libwireshark0.symbols | 1 + epan/uat-int.h | 3 +++ epan/uat.c | 31 ++++++++++++++++++++++++++----- ui/gtk/uat_gui.c | 2 ++ ui/qt/uat_dialog.cpp | 4 ++++ 5 files changed, 36 insertions(+), 5 deletions(-) diff --git a/debian/libwireshark0.symbols b/debian/libwireshark0.symbols index cdba1f3626..baad88053b 100644 --- a/debian/libwireshark0.symbols +++ b/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 diff --git a/epan/uat-int.h b/epan/uat-int.h index 88c52f4b33..c1b488ba99 100644 --- a/epan/uat-int.h +++ b/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); diff --git a/epan/uat.c b/epan/uat.c index 0b4099a6c7..c285aacae5 100644 --- a/epan/uat.c +++ 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); diff --git a/ui/gtk/uat_gui.c b/ui/gtk/uat_gui.c index 422e25dfa7..1b30680b76 100644 --- a/ui/gtk/uat_gui.c +++ b/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; diff --git a/ui/qt/uat_dialog.cpp b/ui/qt/uat_dialog.cpp index 7473f0e227..a2aa8d481b 100644 --- a/ui/qt/uat_dialog.cpp +++ b/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); } }