Re: read after free in notmuch new

[David Bremner]

David Bremner  writes:

> David Bremner  writes:
>
>> I haven't had a chance to really track this down, but it seems there is
>> a memory error in notmuch new (or a maybe false positive from valgrind).
>>
>> Attached is the log from running "make memory-test OPTIONS=--medium" on
>> current git master (0e037c34).
>>
>> It looks like we talloc the message_id string with the message object as
>> parent, but it somehow outlives the message object.
>
> Sorry, that had a few commits beyond master.
>
> master (08343d3d) gives essentially the same log.
>

This should be fixed as of commit 

 4e649d000b9d3764aea98cb0e1120947d7f76f3d
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: read after free in notmuch new

[David Bremner]

David Bremner  writes:

> Tomi Ollila  writes:
>
>> To me it looks like replacing g_hash_table_insert() with 
>> g_hash_table_replace() would do the trick.
>>
>> (or even g_hash_table_add()!)
>>
>> One has to read the documentation a bit (and compare the docstrings of
>> these 2 functions to guess the missing pieces) to get some understanding to
>> this...
>>
>
> Hi Tomi;
>
> Thanks for the suggestion. Unfortunately in my experiments it just
> shifts the invalid memory access to a different piece of memory. I think
> the problem is that a pointer to the previous copy of that key also
> leaked a reference via last_ref, so when we kill that via
> g_hash_table_replace it causes the same problem.
>

Thinking a bit more about it, at least in this case the pointer that was
just inserted remains valid after insertion, and can be
talloc_strdup'ed, i.e.

diff --git a/lib/database.cc b/lib/database.cc
index f0bfe566..eddb780c 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -652,7 +652,7 @@ parse_references (void *ctx,
ref = _parse_message_id (ctx, refs, );
 
if (ref && strcmp (ref, message_id)) {
-   g_hash_table_insert (hash, ref, NULL);
+   g_hash_table_add (hash, ref);
last_ref = ref;
}
 }
@@ -661,7 +661,7 @@ parse_references (void *ctx,
  * reference to the database.  We should avoid making a message
  * its own parent, thus the above check.
  */
-return last_ref;
+return talloc_strdup(ctx, last_ref);
 }
 
 notmuch_status_t

I'll let valgrind chew on that for a bit and see what it says.


___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: read after free in notmuch new

[David Bremner]

Tomi Ollila  writes:

> To me it looks like replacing g_hash_table_insert() with 
> g_hash_table_replace() would do the trick.
>
> (or even g_hash_table_add()!)
>
> One has to read the documentation a bit (and compare the docstrings of
> these 2 functions to guess the missing pieces) to get some understanding to
> this...
>

Hi Tomi;

Thanks for the suggestion. Unfortunately in my experiments it just
shifts the invalid memory access to a different piece of memory. I think
the problem is that a pointer to the previous copy of that key also
leaked a reference via last_ref, so when we kill that via
g_hash_table_replace it causes the same problem.

d
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: read after free in notmuch new

[Tomi Ollila]

On Tue, Feb 21 2017, David Bremner  wrote:

> David Bremner  writes:
>
>> David Bremner  writes:
>>
>>> I haven't had a chance to really track this down, but it seems there is
>>> a memory error in notmuch new (or a maybe false positive from valgrind).
>>>
>>> Attached is the log from running "make memory-test OPTIONS=--medium" on
>>> current git master (0e037c34).
>>>
>>> It looks like we talloc the message_id string with the message object as
>>> parent, but it somehow outlives the message object.
>>
>> Sorry, that had a few commits beyond master.
>>
>> master (08343d3d) gives essentially the same log.
>>
>
> The log says the relevent piece of memory was freed at line 655 of 
> database.cc, which
> is the g_hash_table_insert in the code 
>
>   ref = _parse_message_id (ctx, refs, );
>
>   if (ref && strcmp (ref, message_id)) {
>   g_hash_table_insert (hash, ref, NULL);
>   last_ref = ref;
>   }
>
>
> According to the docs for g_hash_table_insert
>
>If the key already exists in the GHashTable its current value is
>replaced with the new value. If you supplied a value_destroy_func
>when creating the GHashTable, the old value is freed using that
>function. If you supplied a key_destroy_func when creating the
>GHashTable, the passed key is freed using that function.
>
> Since we do pass a key_destroy_func, it seems we are being naughty by
> returning last_ref just below.

To me it looks like replacing g_hash_table_insert() with 
g_hash_table_replace() would do the trick.

(or even g_hash_table_add()!)

One has to read the documentation a bit (and compare the docstrings of
these 2 functions to guess the missing pieces) to get some understanding to
this...

Tomi

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: read after free in notmuch new

[David Bremner]

David Bremner  writes:

> David Bremner  writes:
>
>> I haven't had a chance to really track this down, but it seems there is
>> a memory error in notmuch new (or a maybe false positive from valgrind).
>>
>> Attached is the log from running "make memory-test OPTIONS=--medium" on
>> current git master (0e037c34).
>>
>> It looks like we talloc the message_id string with the message object as
>> parent, but it somehow outlives the message object.
>
> Sorry, that had a few commits beyond master.
>
> master (08343d3d) gives essentially the same log.
>

The log says the relevent piece of memory was freed at line 655 of database.cc, 
which
is the g_hash_table_insert in the code 

ref = _parse_message_id (ctx, refs, );

if (ref && strcmp (ref, message_id)) {
g_hash_table_insert (hash, ref, NULL);
last_ref = ref;
}


According to the docs for g_hash_table_insert

   If the key already exists in the GHashTable its current value is
   replaced with the new value. If you supplied a value_destroy_func
   when creating the GHashTable, the old value is freed using that
   function. If you supplied a key_destroy_func when creating the
   GHashTable, the passed key is freed using that function.

Since we do pass a key_destroy_func, it seems we are being naughty by
returning last_ref just below.

I'm not sure about the best solution; one option would be to drop the
key_destroy_func and manually talloc_free ref, something like

char *ref=NULL;

 while (*refs) {
if (ref) talloc_free (ref);
ref = _parse_message_id (ctx, refs, );

if (ref && strcmp (ref, message_id)) {
g_hash_table_insert (hash, ref, NULL);
last_ref = ref;
}
 }
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: read after free in notmuch new

[David Bremner]

David Bremner  writes:

> I haven't had a chance to really track this down, but it seems there is
> a memory error in notmuch new (or a maybe false positive from valgrind).
>
> Attached is the log from running "make memory-test OPTIONS=--medium" on
> current git master (0e037c34).
>
> It looks like we talloc the message_id string with the message object as
> parent, but it somehow outlives the message object.

Sorry, that had a few commits beyond master.

master (08343d3d) gives essentially the same log.



1.log
Description: Binary data
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch