Re: [PATCH] lib: fix g_hash_table related read-after-free bug

[Tomi Ollila <tomi.ollila@iki.fi>]

On Wed, Feb 22 2017, David Bremner <david@tethera.net> wrote:

> The two g_hash_table functions (insert, add) have different behaviour
> with respect to existing keys. g_hash_table_insert frees the new key,
> while g_hash_table_add (which is really g_hash_table_replace in
> disguise) frees the existing key. With this change 'ref' is live until
> the end of the function (assuming single-threaded access to
> 'hash'). We can't guarantee it will continue to be live in the
> future (i.e. there may be a future key duplication) so we copy it with
> the allocation context passed to parse_references (in practice this is
> the notmuch_message_t object whose parents we are finding).
>
> Thanks to Tomi for the simpler approach to the problem based on
> reading the fine glib manual.
> ---

Great work! LGTM.

Tomi

PS: tried to look whethet there were talloc_ref() (the glib way)...
there is "refcounting" but a bit different (unsuitable) way...

>
> this at least passes the --medium memory test. I'll run the full one
> but it probably needs a day or so to complete.
>
>  lib/database.cc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> 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
> -- 
> 2.11.0