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

2010-04-05 Thread 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

2010-04-05 Thread 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

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

2010-04-01 Thread 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

2010-04-01 Thread 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