Qt: Fix PacketDialog secondary data sources crash after closing file

The first data source tvb associated with a packet is always freed,
along with its data, at the same time as the associated pinfo->pool
scoped data and the tree, if any, in epan_dissect_cleanup() or
epan_dissect_reset().

Not so for secondary data sources; while the vast majority of data
source tvbs are chained to the first data source and so also freed
at the same time, and almost all of the others are never freed and
just leak (sometimes because of an exception before being set as a
child), it's not uncommon to have a tvb whose real data is at file
scope, with the assumption that this will outlive any packet scope.
(Note that the real data is not copied, for speed and memory usage.)

This is, for example, how epan/reassemble.c works, with the data
freed when a file is closed (although it is not managed by wmem.)

When a PacketDialog persists after the capture file closes, this
assumption is falsified. As we do not have a perfect way to detect
the scope of the real data (we could introduce a function to check
if the free_cb is NULL, which would be suggestive but not absolute),
deep copy the data for secondary data sources when the parent
PacketDialog indicates that the capture file is about to close.
Also, avoid calling API functions thacker examine the real data.
Looking at the offsets is OK, as proto_find_field_from_offset() does.

While it could be possible to clone the data source tvbs located in
edt->pi.data_src, the pointers would need to be updated in each of
the field_infos in the edt's tree as well.

Currently no dissectors (or other code) attach data source tvbs where
the tvb itself, not just the real data, is freed with a file is closed.
If they did, that could still cause a crash.

Fix #14363

1 files changed, 32 insertions, 2 deletions

diff --git a/ui/qt/byte_view_tab.cpp b/ui/qt/byte_view_tab.cpp
index 8a266e7ce0..f4407ac051 100644
--- a/ui/qt/byte_view_tab.cpp
+++ b/ui/qt/byte_view_tab.cpp
@@ -97,8 +97,9 @@ void ByteViewTab::addTab(const char *name, tvbuff_t *tvb) {
     if (tvb) {
         int data_len = (int) tvb_captured_length(tvb);
         if (data_len > 0) {
-            // Note: this does not copy the data and will be invalidated when
-            // the tvb becomes invalid (e.g. when the current packet changes).
+            // Note: this does not copy the data and will be invalidated
+            // when the tvbuff's real data becomes invalid (which is not
+            // necessarily when the tvb itself becomes invalid.)
             data = QByteArray::fromRawData((const char *) tvb_get_ptr(tvb, 0, data_len), data_len);
         }
     }
@@ -109,6 +110,30 @@ void ByteViewTab::addTab(const char *name, tvbuff_t *tvb) {
 
     if (tvb)
     {
+        // There are some secondary data source tvbuffs whose datais not freed
+        // when the epan_dissect_t is freed, but at some other point expected
+        // to outlive the packet, generally when the capture file is closed.
+        // If this is a PacketDialog, it can break that assumption.
+        // To get around this, we deep copy their data when the file is closed.
+        //
+        // XXX: We could add a function to the tvbuff API and only do this if
+        // there is no free_cb (a free_cb implies the data is freed at the
+        // same time as the tvb, i.e. when leaving the packet.)
+        if (is_fixed_packet_ && count() > 0) {
+            connect(this, &ByteViewTab::detachData, byte_view_text, &ByteViewText::detachData);
+        }
+        // See above - this tvb is (expected to be) scoped to the packet, but
+        // the real data is not necessarily so. If this is a PacketDialog
+        // and such a secondary data source, then we MUST NOT use any tvb
+        // function that accesses the real data after the capture file closes.
+        // That includes via the ds_tvb item of a field_info in the tree.
+        // proto_find_field_from_offset() is OK. See #14363.
+        //
+        // XXX: It sounds appealing to clone the secondary data source tvbs
+        // and set them to be freed when the byte_view_text is freed, perhaps
+        // even doing so only when the capture file is closing. However, while
+        // relatively simple for the few number of secondary data sources, it
+        // would be a pain to change the pointers for every field_info.
         byte_view_text->setProperty(tvb_data_property, VariantPointer<tvbuff_t>::asQVariant(tvb));
 
         connect(mainApp, SIGNAL(zoomMonospaceFont(QFont)), byte_view_text, SLOT(setMonospaceFont(QFont)));
@@ -338,3 +363,8 @@ void ByteViewTab::setCaptureFile(capture_file *cf)
 
     cap_file_ = cf;
 }
+
+void ByteViewTab::captureFileClosing()
+{
+    emit detachData();
+}