From 9118d959a4f6231a7e928161620e579467b610a9 Mon Sep 17 00:00:00 2001
From: Peter Wu <peter@lekensteyn.nl>
Date: Fri, 28 Sep 2018 13:03:22 +0200
Subject: [PATCH] Qt/PacketList: read packet record in private buffer

To prevent potential interference with other users of the capture file,
read data in a private buffer instead of reusing the one from capFile.

An accidental (?) change in commit v2.9.0rc0-2001-g123bcb0362 resulted
in "cf_read_record" reallocating the capture_file->buf buffer. That
issue combined with the current behavior would result in a crash when
ignoring a packet followed by two times opening a context menu:

    ==32187==ERROR: AddressSanitizer: heap-use-after-free on address 0x7fda91642800 at pc 0x55a98f3faaa7 bp 0x7fffa2807860 sp 0x7fffa2807858
    READ of size 1 at 0x7fda91642800 thread T0
        #0 0x55a98f3faaa6 in QByteArray::operator[](int) const /usr/include/qt/QtCore/qbytearray.h:476:47
        #1 0x55a9901006eb in ByteViewText::drawLine(QPainter*, int, int) ui/qt/widgets/byte_view_text.cpp:370:35
        #2 0x55a9900fd109 in ByteViewText::paintEvent(QPaintEvent*) ui/qt/widgets/byte_view_text.cpp:217:9
        ...
        #50 0x55a98e9fd32a in PacketList::contextMenuEvent(QContextMenuEvent*) ui/qt/packet_list.cpp:614:15
        ...

    0x7fda91642800 is located 0 bytes inside of 3038371-byte region [0x7fda91642800,0x7fda919284a3)
    freed by thread T0 here:
        #0 0x55a98e65fd99 in __interceptor_realloc (run/wireshark+0x1019d99)
        #1 0x7fdac6e1bb88 in g_realloc /build/src/glib/glib/gmem.c:164
        #2 0x7fdaac12c908 in wtap_read_packet_bytes wiretap/wtap.c:1368:2
        #3 0x7fdaabf01e5a in libpcap_read_packet wiretap/libpcap.c:789:7
        #4 0x7fdaabef887d in libpcap_seek_read wiretap/libpcap.c:690:7
        #5 0x7fdaac12d5f5 in wtap_seek_read wiretap/wtap.c:1431:7
        #6 0x55a98e6c8611 in cf_read_record_r file.c:1566:8
        #7 0x55a98e6c88c5 in cf_read_record file.c:1576:10
        #8 0x55a98ea0b725 in PacketList::getFilterFromRowAndColumn() ui/qt/packet_list.cpp:1041:14
        #9 0x55a98e94e4a1 in MainWindow::setMenusForSelectedPacket() ui/qt/main_window_slots.cpp:1175:39

    previously allocated by thread T0 here:
        #0 0x55a98e65fd99 in __interceptor_realloc (run/wireshark+0x1019d99)
        #1 0x7fdac6e1bb88 in g_realloc /build/src/glib/glib/gmem.c:164
        #2 0x7fdaac12c908 in wtap_read_packet_bytes wiretap/wtap.c:1368:2
        #3 0x7fdaabf01e5a in libpcap_read_packet wiretap/libpcap.c:789:7
        #4 0x7fdaabef887d in libpcap_seek_read wiretap/libpcap.c:690:7
        #5 0x7fdaac12d5f5 in wtap_seek_read wiretap/wtap.c:1431:7
        #6 0x55a98e6c8611 in cf_read_record_r file.c:1566:8
        #7 0x55a98e6c88c5 in cf_read_record file.c:1576:10
        #8 0x55a98e6e0bde in cf_select_packet file.c:3777:8
        #9 0x55a98e9ea2ff in PacketList::selectionChanged(QItemSelection const&, QItemSelection const&) ui/qt/packet_list.cpp:420:9

This should be fixed now by I4f1264a406a28c79491dcd77c552193bf3cdf62d,
but let's avoid the shared buffer. It's not exactly a hot code path
anyway.

Change-Id: I548d7293a822601f4eb882672477540f066a066b
Reviewed-on: https://code.wireshark.org/review/29921
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot
Reviewed-by: Guy Harris <guy@alum.mit.edu>
---
 ui/qt/packet_list.cpp | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/ui/qt/packet_list.cpp b/ui/qt/packet_list.cpp
index 6aa804d179..f1750e6baa 100644
--- a/ui/qt/packet_list.cpp
+++ b/ui/qt/packet_list.cpp
@@ -1037,15 +1037,22 @@ QString PacketList::getFilterFromRowAndColumn()
 
     if (fdata != NULL) {
         epan_dissect_t edt;
+        wtap_rec rec; /* Record metadata */
+        Buffer buf;   /* Record data */
 
-        if (!cf_read_record(cap_file_, fdata))
+        wtap_rec_init(&rec);
+        ws_buffer_init(&buf, 1500);
+        if (!cf_read_record_r(cap_file_, fdata, &rec, &buf)) {
+            wtap_rec_cleanup(&rec);
+            ws_buffer_free(&buf);
             return filter; /* error reading the record */
+        }
         /* proto tree, visible. We need a proto tree if there's custom columns */
         epan_dissect_init(&edt, cap_file_->epan, have_custom_cols(&cap_file_->cinfo), FALSE);
         col_custom_prime_edt(&edt, &cap_file_->cinfo);
 
-        epan_dissect_run(&edt, cap_file_->cd_t, &cap_file_->rec,
-                         frame_tvbuff_new_buffer(&cap_file_->provider, fdata, &cap_file_->buf),
+        epan_dissect_run(&edt, cap_file_->cd_t, &rec,
+                         frame_tvbuff_new_buffer(&cap_file_->provider, fdata, &buf),
                          fdata, &cap_file_->cinfo);
         epan_dissect_fill_in_columns(&edt, TRUE, TRUE);
 
@@ -1094,6 +1101,8 @@ QString PacketList::getFilterFromRowAndColumn()
         }
 
         epan_dissect_cleanup(&edt);
+        wtap_rec_cleanup(&rec);
+        ws_buffer_free(&buf);
     }
 
     return filter;