[PATCH] format_part_json: part_content->data is not null terminated

[PATCH] format_part_json: part_content->data is not null terminated

[Gregor Hoffleit]

In format_part_json, part_content->data is not a null terminated string.
Instead, we have to use part_content->len.
---
 notmuch-show.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 1a1d601..4b755e9 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -309,10 +309,15 @@ format_part_json (GMimeObject *part, int *part_count)
     if (g_mime_content_type_is_type (content_type, "text", "*") &&
 	!g_mime_content_type_is_type (content_type, "text", "html"))
     {
+	char *content_data;
+
 	show_part_content (part, stream_memory);
 	part_content = g_mime_stream_mem_get_byte_array (GMIME_STREAM_MEM (stream_memory));
 
-	printf (", \"content\": %s", json_quote_str (ctx, (char *) part_content->data));
+	content_data = talloc_size (ctx, part_content->len+1);
+	memcpy (content_data, (char *)part_content->data, part_content->len+1);
+	content_data[part_content->len] = 0;
+	printf (", \"content\": %s", json_quote_str (ctx, content_data));
     }
 
     fputs ("}", stdout);
-- 
1.7.0

Re: [PATCH] format_part_json: part_content->data is not null terminated

[David Bremner]

On Thu, 04 Mar 2010 11:49:48 +0100, Gregor Hoffleit <gregor@hoffleit.de> wrote:
> In format_part_json, part_content->data is not a null terminated
> string.

I'd like to see this bug fixed, and the patch is pretty small, but...

> Instead, we have to use part_content->len.
> +	content_data = talloc_size (ctx, part_content->len+1);
> +	memcpy (content_data, (char *)part_content->data, part_content->len+1);

Can anyone explain why we copy (what seems to me to be) one extra byte
here?  In principle reading outside our allocated memory could cause
problems; at minimum it makes a false positive for a memory checker like
valgrind.

David

Re: [PATCH] format_part_json: part_content->data is not null terminated

[David Edmondson]

On Thu, 01 Apr 2010 08:40:37 -0300, David Bremner <david@tethera.net> wrote:
> On Thu, 04 Mar 2010 11:49:48 +0100, Gregor Hoffleit <gregor@hoffleit.de> wrote:
> > In format_part_json, part_content->data is not a null terminated
> > string.
> 
> I'd like to see this bug fixed,

+1.

> and the patch is pretty small, but...
> 
> > Instead, we have to use part_content->len.
> > +	content_data = talloc_size (ctx, part_content->len+1);
> > +	memcpy (content_data, (char *)part_content->data, part_content->len+1);
> 
> Can anyone explain why we copy (what seems to me to be) one extra byte
> here?  In principle reading outside our allocated memory could cause
> problems; at minimum it makes a false positive for a memory checker like
> valgrind.

Agreed. It looks as though this should copy only part_content->len bytes.

dme.
-- 
David Edmondson, http://dme.org

Re: [PATCH] format_part_json: part_content->data is not null terminated

[Michal Sojka]

On Thu, 04 Mar 2010, Gregor Hoffleit wrote:
> -	printf (", \"content\": %s", json_quote_str (ctx, (char *) part_content->data));
> +	content_data = talloc_size (ctx, part_content->len+1);
> +	memcpy (content_data, (char *)part_content->data, part_content->len+1);
> +	content_data[part_content->len] = 0;
> +	printf (", \"content\": %s", json_quote_str (ctx, content_data));

What about modifying json_quote_str() to accept additional parameter
len? If I have 10MB attachment to the email, this unnecessary copy is
quite expensive, isn't it?

--Michal

Re: [PATCH] format_part_json: part_content->data is not null terminated

[David Edmondson]

On Thu, 01 Apr 2010 14:05:06 +0200, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> On Thu, 04 Mar 2010, Gregor Hoffleit wrote:
> > -	printf (", \"content\": %s", json_quote_str (ctx, (char *) part_content->data));
> > +	content_data = talloc_size (ctx, part_content->len+1);
> > +	memcpy (content_data, (char *)part_content->data, part_content->len+1);
> > +	content_data[part_content->len] = 0;
> > +	printf (", \"content\": %s", json_quote_str (ctx, content_data));
> 
> What about modifying json_quote_str() to accept additional parameter
> len? If I have 10MB attachment to the email, this unnecessary copy is
> quite expensive, isn't it?

Agreed. How about this patch:
  http://github.com/dme/notmuch/commit/5f23ae341788d28e455e53488d184d8caaa618c5
?

dme.
-- 
David Edmondson, http://dme.org

Re: [PATCH] format_part_json: part_content->data is not null terminated

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 523 bytes --]

On Mon, 05 Apr 2010 10:36:38 +0100, David Edmondson <dme@dme.org> wrote:
> Agreed. How about this patch:
>   http://github.com/dme/notmuch/commit/5f23ae341788d28e455e53488d184d8caaa618c5
> ?

Thanks, David.

(And thanks also, to David Bremner for providing a version with
cleaned-up whitespace issues.)

This is pushed now.

-Carl

PS. David, (Edmonson), putting the actual patch into the email message
is preferred. That lets me reply for any review necessary, and also
apply the patch when reading email when not online.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] format_part_json: part_content->data is not null terminated

[Gregor Hoffleit]

Both of you Davids are indeed completely right. Even more since the
next command in the patch after memcpy zeroes that byte.

This is how it's meant to be:

+    content_data = talloc_size (ctx, part_content->len+1);
+    memcpy (content_data, (char *)part_content->data, part_content->len);
+    content_data[part_content->len] = 0;

Should I submit a fixed patch?

Mea culpa,
    Gregor


* David Edmondson <dme@dme.org> [Do Apr 01 13:50:54 +0200 2010]
> On Thu, 01 Apr 2010 08:40:37 -0300, David Bremner <david@tethera.net> wrote:
> > On Thu, 04 Mar 2010 11:49:48 +0100, Gregor Hoffleit <gregor@hoffleit.de> wrote:
> > > In format_part_json, part_content->data is not a null terminated
> > > string.
> > 
> > I'd like to see this bug fixed,
> 
> +1.
> 
> > and the patch is pretty small, but...
> > 
> > > Instead, we have to use part_content->len.
> > > +    content_data = talloc_size (ctx, part_content->len+1);
> > > +    memcpy (content_data, (char *)part_content->data, part_content->len+1);
> > 
> > Can anyone explain why we copy (what seems to me to be) one extra byte
> > here?  In principle reading outside our allocated memory could cause
> > problems; at minimum it makes a false positive for a memory checker like
> > valgrind.
> 
> Agreed. It looks as though this should copy only part_content->len bytes.
> 
> dme.

-- 
Gregor Hoffleit <gregor.hoffleit@mediasupervision.de>
Media Supervision Software Consulting GmbH
Georg-Friedrich-Haendel-Str. 13, 69214 Eppelheim/Heidelberg
Tel: +49 6221 705079-22  /  Fax: +49 6221 705079-80
Amtsgericht Mannheim, HRB 336821, Geschäftsführer Reinhard Kratzke
http://www.mediasupervision.de/