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

[Carl Worth]

On Mon, 05 Apr 2010 10:36:38 +0100, David Edmondson  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.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: 

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

[David Edmondson]

On Thu, 01 Apr 2010 14:05:06 +0200, Michal Sojka  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: [notmuch] [PATCH] format_part_json: part_content-data is not null terminated

[David Edmondson]

On Thu, 01 Apr 2010 14:05:06 +0200, Michal Sojka sojk...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Carl Worth]

On Mon, 05 Apr 2010 10:36:38 +0100, David Edmondson d...@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.


pgplFFaVVIsvj.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[notmuch] [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  [Do Apr 01 13:50:54 +0200 2010]
> On Thu, 01 Apr 2010 08:40:37 -0300, David Bremner  
> wrote:
> > On Thu, 04 Mar 2010 11:49:48 +0100, Gregor Hoffleit  
> > 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 
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/

[notmuch] [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

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

[David Edmondson]

On Thu, 01 Apr 2010 08:40:37 -0300, David Bremner  wrote:
> On Thu, 04 Mar 2010 11:49:48 +0100, Gregor Hoffleit  
> 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

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

[David Bremner]

On Thu, 04 Mar 2010 11:49:48 +0100, Gregor Hoffleit  
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: [notmuch] [PATCH] format_part_json: part_content-data is not null terminated

[David Bremner]

On Thu, 04 Mar 2010 11:49:48 +0100, Gregor Hoffleit gre...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [notmuch] [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 d...@dme.org [Do Apr 01 13:50:54 +0200 2010]
 On Thu, 01 Apr 2010 08:40:37 -0300, David Bremner da...@tethera.net wrote:
  On Thu, 04 Mar 2010 11:49:48 +0100, Gregor Hoffleit gre...@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.hoffl...@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/
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[notmuch] [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