~kevin8t8/mutt: Fix imap postponed mailbox use-after-free error.

d9dd1d513b676a75f1cecd9c1afb90a8ad52adc4 — Kevin McCarthy 6 months ago 011cfc6

Fix imap postponed mailbox use-after-free error.

mutt_get_postponed() was calling mx_close_mailbox(), but not checking
the return value.  Because the postponed context is an actual
read-write, selected mailbox in a new connection, a failed close left
a dangling pointer in connection->idata->ctx.

imap_keepalive() traversed the connection list, finding the Postponed
connection still there, and passed the dangling pointer to
imap_check_mailbox.

Change an empty postponed mailbox to just call fastclose.  Change the
other closes to retry on a postive return code "reopen" event and then
finally just call fastclose.

Outside the index and postponed menu, Mutt's code only uses append or
readonly temporary contexts.  Those are guaranteed to call
mx_fastclose_mailbox() and return 0.

patch browse

2 files changed, 29 insertions(+), 4 deletions(-)

M mx.c
M postpone.c

M mx.c => mx.c +15 -1

@@ 829,7 829,21 @@ static int trash_append (CONTEXT *ctx)
   return 0;
 }
 
-/* save changes and close mailbox */
+/* save changes and close mailbox.
+ *
+ * returns 0 on success.
+ *        -1 on error
+ *        one of the check_mailbox enums if aborted for one of those reasons.
+ *
+ * Note: it's very important to ensure the mailbox is properly closed
+ *       before free'ing the context.  For selected mailboxes, IMAP
+ *       will cache the context inside connection->idata until
+ *       imap_close_mailbox() removes it.
+ *
+ *       Readonly, dontwrite, and append mailboxes are guaranteed to call
+ *       mx_fastclose_mailbox(), so for most of Mutt's code you won't see
+ *       return value checks for temporary contexts.
+ */
 int mx_close_mailbox (CONTEXT *ctx, int *index_hint)
 {
   int i, move_messages = 0, purge = 1, read_msgs = 0;

M postpone.c => postpone.c +14 -3

@@ 242,6 242,7 @@ int mutt_get_postponed (CONTEXT *ctx, HEADER *hdr, HEADER **cur, BUFFER *fcc)
   LIST *next;
   const char *p;
   int opt_delete;
+  int close_rc;
 
   if (!Postponed)
     return (-1);


@@ 256,7 257,7 @@ int mutt_get_postponed (CONTEXT *ctx, HEADER *hdr, HEADER **cur, BUFFER *fcc)
   if (! PostContext->msgcount)
   {
     PostCount = 0;
-    mx_close_mailbox (PostContext, NULL);
+    mx_fastclose_mailbox (PostContext);
     FREE (&PostContext);
     mutt_error _("No postponed messages.");
     return (-1);


@@ 269,7 270,13 @@ int mutt_get_postponed (CONTEXT *ctx, HEADER *hdr, HEADER **cur, BUFFER *fcc)
   }
   else if ((h = select_msg ()) == NULL)
   {
-    mx_close_mailbox (PostContext, NULL);
+    /* messages might have been marked for deletion.
+     * try once more on reopen before giving up. */
+    close_rc = mx_close_mailbox (PostContext, NULL);
+    if (close_rc > 0)
+      close_rc = mx_close_mailbox (PostContext, NULL);
+    if (close_rc != 0)
+      mx_fastclose_mailbox (PostContext);
     FREE (&PostContext);
     return (-1);
   }


@@ 291,7 298,11 @@ int mutt_get_postponed (CONTEXT *ctx, HEADER *hdr, HEADER **cur, BUFFER *fcc)
   /* avoid the "purge deleted messages" prompt */
   opt_delete = quadoption (OPT_DELETE);
   set_quadoption (OPT_DELETE, MUTT_YES);
-  mx_close_mailbox (PostContext, NULL);
+  close_rc = mx_close_mailbox (PostContext, NULL);
+  if (close_rc > 0)
+    close_rc = mx_close_mailbox (PostContext, NULL);
+  if (close_rc != 0)
+    mx_fastclose_mailbox (PostContext);
   set_quadoption (OPT_DELETE, opt_delete);
 
   FREE (&PostContext);