wiretap: fix memleaks with wtap_rec::opt_comment

The memory ownership of wtap_rec::opt_comment was not clear. Users of
wtap were leaking memory (editcap.c). wtap readers were not sure about
freeing old comments (erf) or simply ignored memleaks (pcapng).

To fix this, ensure opt_comment is owned by wtap_rec and free it with
wtap_rec_cleanup. The erf issue was already addressed since
cf_get_packet_comment properly duplicates wth.opt_comment memory.

- wtap file formats (readers):
  - Should allocate memory for new comments.
  - Should free a comment from an earlier read before writing a new one.
- Users of wth:
  - Can only assume that opt_comment remains valid until the next read.
  - Can assume that wtap_dump does not modify the comment.
  - For random access (wtap_seek_read): should call wtap_rec_cleanup
    to free the comment.

The test_tshark_z_expert_comment and test_text2pcap_sip_pcapng tests now
pass when built with ASAN.

This change was created by carefully looking at all users opt
"opt_comment" and cf_get_packet_comment. Thanks to Vasil Velichkov for
an initial patch which helped validating this version.

Bug: 7515
Change-Id: If3152d1391e7e0d9860f04f3bc2ec41a1f6cc54b
Reviewed-on: https://code.wireshark.org/review/31713
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot
Reviewed-by: Vasil Velichkov <vvvelichkov@gmail.com>
Reviewed-by: Anders Broman <a.broman58@gmail.com>

8 files changed, 22 insertions, 3 deletions

diff --git a/editcap.c b/editcap.c
index 495ab6d1d9..c1616db61e 100644
--- a/editcap.c
+++ b/editcap.c
@@ -1945,7 +1945,8 @@ main(int argc, char *argv[])
                 if (comment != NULL) {
                     /* Copy and change rather than modify returned rec */
                     temp_rec = *rec;
-                    temp_rec.opt_comment = g_strdup(comment);
+                    /* The comment is not modified by dumper, cast away. */
+                    temp_rec.opt_comment = (char *)comment;
                     temp_rec.has_comment_changed = TRUE;
                     rec = &temp_rec;
                 } else {
diff --git a/epan/wslua/wslua_dumper.c b/epan/wslua/wslua_dumper.c
index 64e4a6affd..1d9fa36ba5 100644
--- a/epan/wslua/wslua_dumper.c
+++ b/epan/wslua/wslua_dumper.c
@@ -440,6 +440,11 @@ WSLUA_METHOD Dumper_dump_current(lua_State* L) {
     rec.rec_header.packet_header.pkt_encap = lua_pinfo->rec->rec_header.packet_header.pkt_encap;
     rec.rec_header.packet_header.pseudo_header = *lua_pinfo->pseudo_header;
 
+    /*
+     * wtap_dump does not modify rec.opt_comment, so it should be possible to
+     * pass epan_get_user_comment() or lua_pinfo->rec->opt_comment directly.
+     * Temporarily duplicating the memory should not hurt though.
+     */
     if (lua_pinfo->fd->has_user_comment) {
         rec.opt_comment = wmem_strdup(wmem_packet_scope(), epan_get_user_comment(lua_pinfo->epan, lua_pinfo->fd));
         rec.has_comment_changed = TRUE;
diff --git a/epan/wslua/wslua_frame_info.c b/epan/wslua/wslua_frame_info.c
index e283bd4459..fa6359c6cf 100644
--- a/epan/wslua/wslua_frame_info.c
+++ b/epan/wslua/wslua_frame_info.c
@@ -207,6 +207,7 @@ WSLUA_ATTRIBUTE_NAMED_NUMBER_SETTER(FrameInfo,original_length,rec->rec_header.pa
 WSLUA_ATTRIBUTE_NAMED_NUMBER_GETTER(FrameInfo,encap,rec->rec_header.packet_header.pkt_encap);
 WSLUA_ATTRIBUTE_NAMED_NUMBER_SETTER(FrameInfo,encap,rec->rec_header.packet_header.pkt_encap,int);
 
+// XXX rec->opt_comment is glib-allocated memory, rec is wtap_rec. What frees it?
 /* WSLUA_ATTRIBUTE FrameInfo_comment RW A string comment for the packet, if the
     `wtap_presence_flags.COMMENTS` was set in the presence flags; nil if there is no comment. */
 WSLUA_ATTRIBUTE_NAMED_STRING_GETTER(FrameInfo,comment,rec->opt_comment);
diff --git a/ui/tap_export_pdu.c b/ui/tap_export_pdu.c
index 5d5a11986f..b2a299b74a 100644
--- a/ui/tap_export_pdu.c
+++ b/ui/tap_export_pdu.c
@@ -54,6 +54,9 @@ export_pdu_packet(void *tapdata, packet_info *pinfo, epan_dissect_t *edt, const
 
     rec.rec_header.packet_header.pkt_encap = exp_pdu_tap_data->pkt_encap;
 
+    /* rec.opt_comment is not modified by wtap_dump, but if for some reason the
+     * epan_get_user_comment() or pinfo->rec->opt_comment are invalidated,
+     * copying it here does not hurt. (Can invalidation really happen?) */
     if (pinfo->fd->has_user_comment) {
         rec.opt_comment = g_strdup(epan_get_user_comment(edt->session, pinfo->fd));
         rec.has_comment_changed = TRUE;
diff --git a/wiretap/erf.c b/wiretap/erf.c
index 45884da253..03a0419a6f 100644
--- a/wiretap/erf.c
+++ b/wiretap/erf.c
@@ -2257,6 +2257,9 @@ static int erf_update_anchors_from_header(erf_t *erf_priv, wtap_rec *rec, union
   }
 
   if (comment) {
+    /* Will be freed by either wtap_sequential_close (for rec = &wth->rec) or by
+     * the caller of wtap_seek_read. See wtap_rec_cleanup. */
+    g_free(rec->opt_comment);
     rec->opt_comment = g_strdup(comment);
     rec->presence_flags |= WTAP_HAS_COMMENTS;
   } else {
@@ -2264,8 +2267,7 @@ static int erf_update_anchors_from_header(erf_t *erf_priv, wtap_rec *rec, union
      * Need to set opt_comment to NULL to prevent other packets
      * from displaying the same comment
      */
-    /* XXX: We cannot free the old comment because it can be for a different
-     * frame and still in use, wiretap should be handling this better! */
+    g_free(rec->opt_comment);
     rec->opt_comment = NULL;
   }
 
diff --git a/wiretap/nettrace_3gpp_32_423.c b/wiretap/nettrace_3gpp_32_423.c
index eb41e2ba8b..1bc2c15490 100644
--- a/wiretap/nettrace_3gpp_32_423.c
+++ b/wiretap/nettrace_3gpp_32_423.c
@@ -171,7 +171,9 @@ nettrace_read(wtap *wth, int *err, gchar **err_info, gint64 *data_offset)
 	wth->rec.rec_header.packet_header.pkt_encap = file_info->wth_tmp_file->rec.rec_header.packet_header.pkt_encap;
 	wth->rec.tsprec = file_info->wth_tmp_file->rec.tsprec;
 	wth->rec.rec_header.packet_header.interface_id = file_info->wth_tmp_file->rec.rec_header.packet_header.interface_id;
+	/* Steal memory from the pcapng wth. */
 	wth->rec.opt_comment = file_info->wth_tmp_file->rec.opt_comment;
+	file_info->wth_tmp_file->rec.opt_comment = NULL;
 	wth->rec.rec_header.packet_header.drop_count = file_info->wth_tmp_file->rec.rec_header.packet_header.drop_count;
 	wth->rec.rec_header.packet_header.pack_flags = file_info->wth_tmp_file->rec.rec_header.packet_header.pack_flags;
 
diff --git a/wiretap/pcapng.c b/wiretap/pcapng.c
index 23e554830f..bd101b4722 100644
--- a/wiretap/pcapng.c
+++ b/wiretap/pcapng.c
@@ -1323,6 +1323,7 @@ pcapng_read_packet_block(FILE_T fh, pcapng_block_header_t *bh, pcapng_t *pn, wta
     }
 
     /* Option defaults */
+    g_free(wblock->rec->opt_comment);   /* Free memory from an earlier read. */
     wblock->rec->opt_comment = NULL;
     wblock->rec->rec_header.packet_header.drop_count  = -1;
     wblock->rec->rec_header.packet_header.pack_flags  = 0;
@@ -1374,6 +1375,7 @@ pcapng_read_packet_block(FILE_T fh, pcapng_block_header_t *bh, pcapng_t *pn, wta
             case(OPT_COMMENT):
                 if (oh->option_length > 0 && oh->option_length < opt_cont_buf_len) {
                     wblock->rec->presence_flags |= WTAP_HAS_COMMENTS;
+                    g_free(wblock->rec->opt_comment);
                     wblock->rec->opt_comment = g_strndup((char *)option_content, oh->option_length);
                     pcapng_debug("pcapng_read_packet_block: length %u opt_comment '%s'", oh->option_length, wblock->rec->opt_comment);
                 } else {
@@ -1571,6 +1573,7 @@ pcapng_read_simple_packet_block(FILE_T fh, pcapng_block_header_t *bh, pcapng_t *
     wblock->rec->ts.secs = 0;
     wblock->rec->ts.nsecs = 0;
     wblock->rec->rec_header.packet_header.interface_id = 0;
+    g_free(wblock->rec->opt_comment);   /* Free memory from an earlier read. */
     wblock->rec->opt_comment = NULL;
     wblock->rec->rec_header.packet_header.drop_count = 0;
     wblock->rec->rec_header.packet_header.pack_flags = 0;
diff --git a/wiretap/wtap.c b/wiretap/wtap.c
index 693999bbce..fb77a4b23e 100644
--- a/wiretap/wtap.c
+++ b/wiretap/wtap.c
@@ -1460,6 +1460,8 @@ wtap_rec_init(wtap_rec *rec)
 void
 wtap_rec_cleanup(wtap_rec *rec)
 {
+	g_free(rec->opt_comment);
+	rec->opt_comment = NULL;
 	ws_buffer_free(&rec->options_buf);
 }