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 D8A376DE1E48 for ; Wed, 22 Feb 2017 02:32:13 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at cworth.org X-Spam-Flag: NO X-Spam-Score: -0.005 X-Spam-Level: X-Spam-Status: No, score=-0.005 tagged_above=-999 required=5 tests=[AWL=0.006, SPF_PASS=-0.001, T_RP_MATCHES_RCVD=-0.01] 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 ETsHN2z68LnX for ; Wed, 22 Feb 2017 02:32:13 -0800 (PST) Received: from fethera.tethera.net (fethera.tethera.net [198.245.60.197]) by arlo.cworth.org (Postfix) with ESMTPS id 4825C6DE1E3F for ; Wed, 22 Feb 2017 02:32:12 -0800 (PST) Received: from remotemail by fethera.tethera.net with local (Exim 4.84_2) (envelope-from ) id 1cgUCr-0002LN-Po; Wed, 22 Feb 2017 05:31:33 -0500 Received: (nullmailer pid 10048 invoked by uid 1000); Wed, 22 Feb 2017 10:32:09 -0000 From: David Bremner To: David Bremner , Tomi Ollila , notmuch@notmuchmail.org Subject: [PATCH] lib: fix g_hash_table related read-after-free bug Date: Wed, 22 Feb 2017 06:32:07 -0400 Message-Id: <20170222103207.10000-1-david@tethera.net> X-Mailer: git-send-email 2.11.0 In-Reply-To: <87lgsz3soy.fsf@tethera.net> References: <87lgsz3soy.fsf@tethera.net> 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 10:32:14 -0000 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. --- 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