From b078310bd0daf7cf8ba6df765cba9fcd98713dd8 Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Mon, 21 May 2018 22:33:37 +0200 Subject: [PATCH] Qt: fix use-after-free on error while saving exported packets MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit When an error occurs while saving packets using the Export Specified Packets dialog (e.g. try to overwrite the opened capture file), the dialog is displayed again. As PacketRangeGroupBox freed the packet selection range, a crash (use-after-free) occurs. Removes some unnecessary code in MainWindow::exportDissections as well. Change-Id: I63898427eff7e71799d89c8a22246db8f93a9ff6 Fixes: v2.5.0rc0-968-g38b40acb2d ("Qt: fix a memory leak when exporting packets") Reviewed-on: https://code.wireshark.org/review/27695 Petri-Dish: Peter Wu Tested-by: Petri Dish Buildbot Reviewed-by: Stig Bjørlykke Reviewed-by: Anders Broman --- file.c | 1 + ui/packet_range.c | 4 ++++ ui/packet_range.h | 3 +++ ui/qt/export_dissection_dialog.cpp | 1 + ui/qt/main_window.cpp | 23 +++++++++-------------- ui/qt/main_window_slots.cpp | 4 +++- ui/qt/packet_range_group_box.cpp | 2 -- ui/qt/packet_range_group_box.h | 4 ++++ ui/qt/print_dialog.cpp | 3 ++- 9 files changed, 27 insertions(+), 18 deletions(-) diff --git a/file.c b/file.c index 500a414a0b..c9b46ab7f2 100644 --- a/file.c +++ b/file.c @@ -2114,6 +2114,7 @@ cf_retap_packets(capture_file *cf) "all packets", TRUE, retap_packet, &callback_args, TRUE); + packet_range_cleanup(&range); epan_dissect_cleanup(&callback_args.edt); cf_callback_invoke(cf_cb_file_retap_finished, cf); diff --git a/ui/packet_range.c b/ui/packet_range.c index 56b4b3470a..a0cd7e2166 100644 --- a/ui/packet_range.c +++ b/ui/packet_range.c @@ -225,6 +225,10 @@ void packet_range_init(packet_range_t *range, capture_file *cf) { packet_range_calc_user(range); } +void packet_range_cleanup(packet_range_t *range) { + wmem_free(NULL, range->user_range); +} + /* check whether the packet range is OK */ convert_ret_t packet_range_check(packet_range_t *range) { if (range->process == range_process_user_range && range->user_range == NULL) { diff --git a/ui/packet_range.h b/ui/packet_range.h index 0d824e8e08..1622648199 100644 --- a/ui/packet_range.h +++ b/ui/packet_range.h @@ -84,6 +84,9 @@ typedef enum { /* init the range structure */ extern void packet_range_init(packet_range_t *range, capture_file *cf); +/* Cleanup the range structure before the caller frees "range". */ +extern void packet_range_cleanup(packet_range_t *range); + /* check whether the packet range is OK */ extern convert_ret_t packet_range_check(packet_range_t *range); diff --git a/ui/qt/export_dissection_dialog.cpp b/ui/qt/export_dissection_dialog.cpp index 9ab0af8edb..3d09b38785 100644 --- a/ui/qt/export_dissection_dialog.cpp +++ b/ui/qt/export_dissection_dialog.cpp @@ -137,6 +137,7 @@ ExportDissectionDialog::~ExportDissectionDialog() { #if !defined(Q_OS_WIN) g_free(print_args_.file); + packet_range_cleanup(&print_args_.range); #endif } diff --git a/ui/qt/main_window.cpp b/ui/qt/main_window.cpp index f4a5e5b198..1d6a9f12ef 100644 --- a/ui/qt/main_window.cpp +++ b/ui/qt/main_window.cpp @@ -1575,7 +1575,7 @@ void MainWindow::exportSelectedPackets() { case CANCELLED: /* The user said "forget it". Just get rid of the dialog box and return. */ - return; + goto cleanup; } /* @@ -1632,7 +1632,7 @@ void MainWindow::exportSelectedPackets() { packet_list_queue_draw(); /* Add this filename to the list of recent files in the "Recent Files" submenu */ add_menu_recent_capture_file(qUtf8Printable(file_name)); - return; + goto cleanup; case CF_WRITE_ERROR: /* The save failed; let the user try again. */ @@ -1640,24 +1640,19 @@ void MainWindow::exportSelectedPackets() { case CF_WRITE_ABORTED: /* The user aborted the save; just return. */ - return; + goto cleanup; } } - return; + +cleanup: + packet_range_cleanup(&range); } void MainWindow::exportDissections(export_type_e export_type) { - ExportDissectionDialog ed_dlg(this, capture_file_.capFile(), export_type); - packet_range_t range; - - if (!capture_file_.capFile()) - return; - - /* Init the packet range */ - packet_range_init(&range, capture_file_.capFile()); - range.process_filtered = TRUE; - range.include_dependents = TRUE; + capture_file *cf = capture_file_.capFile(); + g_return_if_fail(cf); + ExportDissectionDialog ed_dlg(this, cf, export_type); ed_dlg.exec(); } diff --git a/ui/qt/main_window_slots.cpp b/ui/qt/main_window_slots.cpp index 703c75b540..5ffed4b0a5 100644 --- a/ui/qt/main_window_slots.cpp +++ b/ui/qt/main_window_slots.cpp @@ -2109,8 +2109,10 @@ void MainWindow::on_actionStatisticsHpfeeds_triggered() void MainWindow::on_actionFilePrint_triggered() { - PrintDialog pdlg(this, capture_file_.capFile()); + capture_file *cf = capture_file_.capFile(); + g_return_if_fail(cf); + PrintDialog pdlg(this, cf); pdlg.exec(); } diff --git a/ui/qt/packet_range_group_box.cpp b/ui/qt/packet_range_group_box.cpp index a616e961cd..95588b26f1 100644 --- a/ui/qt/packet_range_group_box.cpp +++ b/ui/qt/packet_range_group_box.cpp @@ -25,8 +25,6 @@ PacketRangeGroupBox::PacketRangeGroupBox(QWidget *parent) : PacketRangeGroupBox::~PacketRangeGroupBox() { - if (range_) - wmem_free(NULL, range_->user_range); delete pr_ui_; } diff --git a/ui/qt/packet_range_group_box.h b/ui/qt/packet_range_group_box.h index d943c19594..215f3ee190 100644 --- a/ui/qt/packet_range_group_box.h +++ b/ui/qt/packet_range_group_box.h @@ -23,6 +23,10 @@ namespace Ui { class PacketRangeGroupBox; } +/** + * UI element for controlling a range selection. The range provided in + * "initRange" is not owned by this class but will be modified. + */ class PacketRangeGroupBox : public QGroupBox { Q_OBJECT diff --git a/ui/qt/print_dialog.cpp b/ui/qt/print_dialog.cpp index d08201f683..53ba9c6641 100644 --- a/ui/qt/print_dialog.cpp +++ b/ui/qt/print_dialog.cpp @@ -68,7 +68,7 @@ PrintDialog::PrintDialog(QWidget *parent, capture_file *cf) : page_pos_(0), in_preview_(FALSE) { - if (!cf) done(QDialog::Rejected); // ...or assert? + Q_ASSERT(cf); pd_ui_->setupUi(this); setWindowTitle(wsApp->windowTitleString(tr("Print"))); @@ -123,6 +123,7 @@ PrintDialog::PrintDialog(QWidget *parent, capture_file *cf) : PrintDialog::~PrintDialog() { + packet_range_cleanup(&print_args_.range); delete pd_ui_; } -- 2.34.1