From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by arlo.cworth.org (Postfix) with ESMTP id D924A6DE1F59 for ; Wed, 22 Feb 2017 03:25:58 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at cworth.org X-Spam-Flag: NO X-Spam-Score: 0.506 X-Spam-Level: X-Spam-Status: No, score=0.506 tagged_above=-999 required=5 tests=[AWL=-0.146, SPF_NEUTRAL=0.652] autolearn=disabled Received: from arlo.cworth.org ([127.0.0.1]) by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id kdPM1isFR19G for ; Wed, 22 Feb 2017 03:25:57 -0800 (PST) Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34]) by arlo.cworth.org (Postfix) with ESMTP id 443A26DE1A2E for ; Wed, 22 Feb 2017 03:25:56 -0800 (PST) Received: from guru.guru-group.fi (localhost [IPv6:::1]) by guru.guru-group.fi (Postfix) with ESMTP id 3242A1001A7; Wed, 22 Feb 2017 13:25:26 +0200 (EET) From: Tomi Ollila To: David Bremner , notmuch@notmuchmail.org Subject: Re: [PATCH] lib: fix g_hash_table related read-after-free bug In-Reply-To: <20170222103207.10000-1-david@tethera.net> References: <87lgsz3soy.fsf@tethera.net> <20170222103207.10000-1-david@tethera.net> User-Agent: Notmuch/0.23.3+85~g2b85e66 (https://notmuchmail.org) Emacs/24.5.1 (x86_64-unknown-linux-gnu) X-Face: HhBM'cA~ MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Feb 2017 11:25:58 -0000 On Wed, Feb 22 2017, David Bremner 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