read after free in notmuch new

[David Bremner]

David Bremner <david at tethera.net> writes:

> Tomi Ollila <tomi.ollila at iki.fi> 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, &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.