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.

diff --git a/mh.c b/mh.c
index 63e12d2fb22a34137184f0314567381f413520b8..db908a1bce426d48d43b9a2da4de8d36f9202e0a 100644 (file)
--- 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++)
   {