Qt: fix use-after-free on error while saving exported packets
authorPeter Wu <peter@lekensteyn.nl>
Mon, 21 May 2018 20:33:37 +0000 (22:33 +0200)
committerAnders Broman <a.broman58@gmail.com>
Fri, 25 May 2018 12:49:50 +0000 (12:49 +0000)
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 <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot
Reviewed-by: Stig Bjørlykke <stig@bjorlykke.org>
Reviewed-by: Anders Broman <a.broman58@gmail.com>
file.c
ui/packet_range.c
ui/packet_range.h
ui/qt/export_dissection_dialog.cpp
ui/qt/main_window.cpp
ui/qt/main_window_slots.cpp
ui/qt/packet_range_group_box.cpp
ui/qt/packet_range_group_box.h
ui/qt/print_dialog.cpp

diff --git a/file.c b/file.c
index 500a414a0bbc9b0a1581d62d47dab8d5a0b4a398..c9b46ab7f20fa016be0324860b402a843539fa11 100644 (file)
--- 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);
index 56b4b3470ac45481ca0841161bed459bdb452787..a0cd7e21661c960e4ec9cd8ca08ed7d1ed3df536 100644 (file)
@@ -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) {
index 0d824e8e087f164893dae3a1b8ca12b5bde0e0cf..1622648199e9a46cb2d7545eb92bf42d81db3d9f 100644 (file)
@@ -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);
 
index 9ab0af8edb458f1abf2dd852ae1c43cc83ef9d19..3d09b3878553ea749269ad8eb22e5ca67b4d5837 100644 (file)
@@ -137,6 +137,7 @@ ExportDissectionDialog::~ExportDissectionDialog()
 {
 #if !defined(Q_OS_WIN)
     g_free(print_args_.file);
+    packet_range_cleanup(&print_args_.range);
 #endif
 }
 
index f4a5e5b1987662dee48a68dd26370615f1fee18a..1d6a9f12efd73f3f873cfcd943733cb2a339cbf6 100644 (file)
@@ -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();
 }
 
index 703c75b540f13b4a0f04ab2ae97bac4c8593373e..5ffed4b0a52baad3b53aa64f81f5be8ba8c81083 100644 (file)
@@ -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();
 }
 
index a616e961cdcf548bd61ec0932ec7ef6ec23d6a6d..95588b26f17e7bbb9ae1594f1e72c6ed3acf12ad 100644 (file)
@@ -25,8 +25,6 @@ PacketRangeGroupBox::PacketRangeGroupBox(QWidget *parent) :
 
 PacketRangeGroupBox::~PacketRangeGroupBox()
 {
-    if (range_)
-        wmem_free(NULL, range_->user_range);
     delete pr_ui_;
 }
 
index d943c19594dcdef3cc455806634a10fcbf43732a..215f3ee1901378b2b5e18c71fd0dba4ba322708a 100644 (file)
@@ -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
index d08201f683705c485309cb7409e6dade87d857ee..53ba9c66416e4c3f634044503807868fb73503f4 100644 (file)
@@ -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_;
 }