diff options
author | Peter Wu <peter@lekensteyn.nl> | 2018-09-28 13:03:22 +0200 |
---|---|---|
committer | Guy Harris <guy@alum.mit.edu> | 2018-09-30 16:30:04 +0000 |
commit | 9118d959a4f6231a7e928161620e579467b610a9 (patch) | |
tree | ce71052a6b7c031e15cef807963a9f3e941284dc | |
parent | 5a401ccad9889c8073b57bdc5a03970fcf5d3d1d (diff) |
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>
-rw-r--r-- | ui/qt/packet_list.cpp | 17 |
1 files changed, 13 insertions, 4 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; - - if (!cf_read_record(cap_file_, fdata)) + wtap_rec rec; /* Record metadata */ + Buffer buf; /* Record data */ + + 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; |