From 195361c8cd100005e6562f54ccab15cb98cb20fd Mon Sep 17 00:00:00 2001 From: Jeffrey Stedfast Date: Thu, 16 Mar 2017 16:53:47 +0000 Subject: [PATCH 1/5] fix memory leaks in notmuch-show.c:format_headers_sprinter() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Internet_address_list_to_string() and g_mime_message_get_date_as_string() return allocated string buffers and not const, so from what I can tell from taking a look at the sprinter-sexp.c’s sexp_string() function, the code leaks the recipients_string as well as the date string. --- notmuch-show.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/notmuch-show.c b/notmuch-show.c index 744b6272..2dbf8704 100644 --- a/notmuch-show.c +++ b/notmuch-show.c @@ -244,8 +244,9 @@ format_headers_sprinter (sprinter_t *sp, GMimeMessage *message, * reflected in the file devel/schemata. */ InternetAddressList *recipients; - const char *recipients_string; + char *recipients_string; const char *reply_to_string; + char *date_string; sp->begin_map (sp); @@ -260,6 +261,7 @@ format_headers_sprinter (sprinter_t *sp, GMimeMessage *message, if (recipients_string) { sp->map_key (sp, "To"); sp->string (sp, recipients_string); + g_free (recipients_string); } recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_CC); @@ -267,6 +269,7 @@ format_headers_sprinter (sprinter_t *sp, GMimeMessage *message, if (recipients_string) { sp->map_key (sp, "Cc"); sp->string (sp, recipients_string); + g_free (recipients_string); } recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_BCC); @@ -274,6 +277,7 @@ format_headers_sprinter (sprinter_t *sp, GMimeMessage *message, if (recipients_string) { sp->map_key (sp, "Bcc"); sp->string (sp, recipients_string); + g_free (recipients_string); } reply_to_string = g_mime_message_get_reply_to (message); @@ -290,7 +294,9 @@ format_headers_sprinter (sprinter_t *sp, GMimeMessage *message, sp->string (sp, g_mime_object_get_header (GMIME_OBJECT (message), "References")); } else { sp->map_key (sp, "Date"); - sp->string (sp, g_mime_message_get_date_as_string (message)); + date_string = g_mime_message_get_date_as_string (message); + sp->string (sp, date_string); + g_free (date_string); } sp->end (sp); From 2ae6b8cb68759667c3caf024cb434de59bcc0899 Mon Sep 17 00:00:00 2001 From: David Bremner Date: Sat, 18 Mar 2017 14:07:45 -0300 Subject: [PATCH 2/5] cli/show: fix some memory leaks in format_part_text Mimic Jeff Stedfast's changes to format_headers_sprinter, clean up use of internet_address_list_to_string and g_mime_message_get_date_as_string. --- notmuch-show.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/notmuch-show.c b/notmuch-show.c index 2dbf8704..615857fe 100644 --- a/notmuch-show.c +++ b/notmuch-show.c @@ -502,7 +502,8 @@ format_part_text (const void *ctx, sprinter_t *sp, mime_node_t *node, if (GMIME_IS_MESSAGE (node->part)) { GMimeMessage *message = GMIME_MESSAGE (node->part); InternetAddressList *recipients; - const char *recipients_string; + char *recipients_string; + char *date_string; printf ("\fheader{\n"); if (node->envelope_file) @@ -513,11 +514,15 @@ format_part_text (const void *ctx, sprinter_t *sp, mime_node_t *node, recipients_string = internet_address_list_to_string (recipients, 0); if (recipients_string) printf ("To: %s\n", recipients_string); + g_free (recipients_string); recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_CC); recipients_string = internet_address_list_to_string (recipients, 0); if (recipients_string) printf ("Cc: %s\n", recipients_string); - printf ("Date: %s\n", g_mime_message_get_date_as_string (message)); + g_free (recipients_string); + date_string = g_mime_message_get_date_as_string (message); + printf ("Date: %s\n", date_string); + g_free (date_string); printf ("\fheader}\n"); printf ("\fbody{\n"); From b4cedc782415062f63d29b8f2de89956f2b8803b Mon Sep 17 00:00:00 2001 From: David Bremner Date: Sat, 18 Mar 2017 14:33:50 -0300 Subject: [PATCH 3/5] cli/show: fix usage of g_mime_content_type_to_string It returns an "allocated string", which needs to be freed. --- notmuch-show.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/notmuch-show.c b/notmuch-show.c index 615857fe..c0ed9c87 100644 --- a/notmuch-show.c +++ b/notmuch-show.c @@ -480,6 +480,7 @@ format_part_text (const void *ctx, sprinter_t *sp, mime_node_t *node, notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED) ? 1 : 0, notmuch_message_get_filename (message)); } else { + char *content_string; const char *disposition = _get_disposition (meta); const char *cid = g_mime_object_get_content_id (meta); const char *filename = leaf ? @@ -496,7 +497,10 @@ format_part_text (const void *ctx, sprinter_t *sp, mime_node_t *node, printf (", Filename: %s", filename); if (cid) printf (", Content-id: %s", cid); - printf (", Content-type: %s\n", g_mime_content_type_to_string (content_type)); + + content_string = g_mime_content_type_to_string (content_type); + printf (", Content-type: %s\n", content_string); + g_free (content_string); } if (GMIME_IS_MESSAGE (node->part)) { @@ -537,8 +541,9 @@ format_part_text (const void *ctx, sprinter_t *sp, mime_node_t *node, show_text_part_content (node->part, stream_stdout, 0); g_object_unref(stream_stdout); } else { - printf ("Non-text part: %s\n", - g_mime_content_type_to_string (content_type)); + char *content_string = g_mime_content_type_to_string (content_type); + printf ("Non-text part: %s\n", content_string); + g_free (content_string); } } @@ -606,6 +611,7 @@ format_part_sprinter (const void *ctx, sprinter_t *sp, mime_node_t *node, GMimeObject *meta = node->envelope_part ? GMIME_OBJECT (node->envelope_part) : node->part; GMimeContentType *content_type = g_mime_object_get_content_type (meta); + char *content_string; const char *disposition = _get_disposition (meta); const char *cid = g_mime_object_get_content_id (meta); const char *filename = GMIME_IS_PART (node->part) ? @@ -634,7 +640,9 @@ format_part_sprinter (const void *ctx, sprinter_t *sp, mime_node_t *node, } sp->map_key (sp, "content-type"); - sp->string (sp, g_mime_content_type_to_string (content_type)); + content_string = g_mime_content_type_to_string (content_type); + sp->string (sp, content_string); + g_free (content_string); if (disposition) { sp->map_key (sp, "content-disposition"); From eafa8c62b33570cf5f239b08a216b03ea8bf8866 Mon Sep 17 00:00:00 2001 From: David Bremner Date: Sat, 18 Mar 2017 14:46:42 -0300 Subject: [PATCH 4/5] cli/show: unref crlf filter. Mimic the handling of the other filter g_objects. This cleans up a fair sized memory leak. --- notmuch-show.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/notmuch-show.c b/notmuch-show.c index c0ed9c87..1954096d 100644 --- a/notmuch-show.c +++ b/notmuch-show.c @@ -318,6 +318,7 @@ show_text_part_content (GMimeObject *part, GMimeStream *stream_out, { GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part)); GMimeStream *stream_filter = NULL; + GMimeFilter *crlf_filter = NULL; GMimeDataWrapper *wrapper; const char *charset; @@ -329,8 +330,10 @@ show_text_part_content (GMimeObject *part, GMimeStream *stream_out, return; stream_filter = g_mime_stream_filter_new (stream_out); + crlf_filter = g_mime_filter_crlf_new (FALSE, FALSE); g_mime_stream_filter_add(GMIME_STREAM_FILTER (stream_filter), - g_mime_filter_crlf_new (FALSE, FALSE)); + crlf_filter); + g_object_unref (crlf_filter); charset = g_mime_object_get_content_type_parameter (part, "charset"); if (charset) { From 06adc276682d1d5f73d78df2e898ad4191eb4499 Mon Sep 17 00:00:00 2001 From: Tomi Ollila Date: Sat, 18 Mar 2017 00:28:48 +0200 Subject: [PATCH 5/5] lib/message.cc: fix Coverity finding (use after free) The object where pointer to `data` was received was deleted before it was used in _notmuch_string_list_append(). Relevant Coverity messages follow: 3: extract Assigning: data = std::__cxx11::string(message->doc.()).c_str(), which extracts wrapped state from temporary of type std::__cxx11::string. 4: dtor_free The internal representation of temporary of type std::__cxx11::string is freed by its destructor. 5: use after free: Wrapper object use after free (WRAPPER_ESCAPE) Using internal representation of destroyed object local data. --- lib/message.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/message.cc b/lib/message.cc index 007f1171..36a07a88 100644 --- a/lib/message.cc +++ b/lib/message.cc @@ -870,9 +870,9 @@ _notmuch_message_ensure_filename_list (notmuch_message_t *message) * * It would be nice to do the upgrade of the document directly * here, but the database is likely open in read-only mode. */ - const char *data; - data = message->doc.get_data ().c_str (); + std::string datastr = message->doc.get_data (); + const char *data = datastr.c_str (); if (data == NULL) INTERNAL_ERROR ("message with no filename");