From 9600523147e11e54a94771e48310bf0e5e77bfeb Mon Sep 17 00:00:00 2001 From: Kevin McCarthy Date: Tue, 8 Dec 2015 09:11:30 -0800 Subject: [PATCH] Fix hash table key "use after free" in mh_check_mailbox(). (closes #3797) The fnames hash uses the maildir->header->path as the key. As matches are found, the headers are freed. This inadvertantly also freed the key to the hashtable entry; the next hash_find() going to the same bucket might end up comparing keys with a freed string. This patch stores the path in the struct maildir canon_fname field (just as maildir_check_mailbox() does) and uses that as the hash key instead. This field isn't used outside of maildir_check_mailbox(), and is automatically freed for us in the maildir_move_to_context() call at the bottom of both functions. Note there are other ways to fix this problem: - Add a new mode to the hash table, causing it to strdup the keys and free them itself. - Delete the entries in the fnames hash, rather leaving them there. The first seems the cleanest, but would end up touching much more code. The second is also clean, but might have a negative performance impact. Additionally, peeking back in history to changeset 1d45a50b6f9b, it looks like the canon_fname used to be used by mh too, so perhaps removing the strdup may have been a mistake during refactoring at some point. --- mh.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mh.c b/mh.c index 63e12d2fb..db908a1bc 100644 --- a/mh.c +++ b/mh.c @@ -2106,7 +2106,11 @@ int mh_check_mailbox (CONTEXT * ctx, int *index_hint) fnames = hash_create (1031, 0); for (p = md; p; p = p->next) - hash_insert (fnames, p->h->path, p, 0); + { + /* the hash key must survive past the header, which is freed below. */ + p->canon_fname = safe_strdup (p->h->path); + hash_insert (fnames, p->canon_fname, p, 0); + } for (i = 0; i < ctx->msgcount; i++) { -- 2.40.0