From 9d5ab211635e201692cc250732be50a234e03d1c Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Thu, 24 Jan 2019 00:15:08 +0100 Subject: [PATCH] wiretap: fix leak of options_buf and document memory handling Manually checked all callers of wtap_seek_read to ensure that wtap_rec_cleanup is called. Added missing wtap_rec_cleanup to: - Completion of sequential read: wtap_sequential_close - Callers of wtap_seek_read: - users of cf_read_record_r: - PacketListRecord::dissect This fixes one of the two ASAN memleak reports while running test_tshark_z_expert_comment and test_text2pcap_sip_pcapng (the other is about opt_comment which is still unfixed). Vasil Velichkov also found this issue and came up with a similar fix. Change-Id: I54a6aa70bfdb42a816d03ad4861d0ad821d0ef88 Reviewed-on: https://code.wireshark.org/review/31709 Petri-Dish: Peter Wu Tested-by: Petri Dish Buildbot Reviewed-by: Anders Broman --- ui/qt/models/packet_list_record.cpp | 4 +++- wiretap/file_access.c | 4 +++- wiretap/pcapng.c | 6 +++++- wiretap/wtap.c | 2 ++ 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/ui/qt/models/packet_list_record.cpp b/ui/qt/models/packet_list_record.cpp index 617af34aa2..ce90b77fb6 100644 --- a/ui/qt/models/packet_list_record.cpp +++ b/ui/qt/models/packet_list_record.cpp @@ -110,12 +110,12 @@ void PacketListRecord::dissect(capture_file *cap_file, bool dissect_color) return; } - memset(&rec, 0, sizeof rec); if (dissect_columns) { cinfo = &cap_file->cinfo; } + wtap_rec_init(&rec); ws_buffer_init(&buf, 1500); if (!cf_read_record_r(cap_file, fdata_, &rec, &buf)) { /* @@ -138,6 +138,7 @@ void PacketListRecord::dissect(capture_file *cap_file, bool dissect_color) colorized_ = true; } ws_buffer_free(&buf); + wtap_rec_cleanup(&rec); return; /* error reading the record */ } @@ -193,6 +194,7 @@ void PacketListRecord::dissect(capture_file *cap_file, bool dissect_color) epan_dissect_cleanup(&edt); ws_buffer_free(&buf); + wtap_rec_cleanup(&rec); } // This assumes only one packet list. We might want to move this to diff --git a/wiretap/file_access.c b/wiretap/file_access.c index 894da7b085..803dac8d0b 100644 --- a/wiretap/file_access.c +++ b/wiretap/file_access.c @@ -1128,7 +1128,9 @@ fail: return NULL; success: - wth->rec_data = (struct Buffer *)g_malloc(sizeof(struct Buffer)); + /* wth->rec_data is used for the sequential read and will be freed by + * wtap_sequential_close (which is also called by wtap_close). */ + wth->rec_data = g_new(struct Buffer, 1); ws_buffer_init(wth->rec_data, 1500); if ((wth->file_type_subtype == WTAP_FILE_TYPE_SUBTYPE_PCAP) || diff --git a/wiretap/pcapng.c b/wiretap/pcapng.c index 55fd663360..23e554830f 100644 --- a/wiretap/pcapng.c +++ b/wiretap/pcapng.c @@ -1341,7 +1341,11 @@ pcapng_read_packet_block(FILE_T fh, pcapng_block_header_t *bh, pcapng_t *pn, wta block_read - /* fixed and variable part, including padding */ (int)sizeof(bh->block_total_length); - /* Allocate enough memory to hold all options */ + /* Ensure sufficient temporary memory to hold all options. It is not freed + * on return to avoid frequent reallocations. When called for sequential + * read (wtap_read), "wblock->rec == &wth->rec" (options_buf will be freed + * by wtap_sequential_close). For random access, memory is managed by the + * caller of wtap_seek_read. */ opt_cont_buf_len = to_read; ws_buffer_assure_space(&wblock->rec->options_buf, opt_cont_buf_len); opt_ptr = ws_buffer_start_ptr(&wblock->rec->options_buf); diff --git a/wiretap/wtap.c b/wiretap/wtap.c index fe43e3b457..693999bbce 100644 --- a/wiretap/wtap.c +++ b/wiretap/wtap.c @@ -1195,6 +1195,8 @@ wtap_sequential_close(wtap *wth) wth->fh = NULL; } + wtap_rec_cleanup(&wth->rec); + if (wth->rec_data) { ws_buffer_free(wth->rec_data); g_free(wth->rec_data);