Re: [PATCH 2/6] Fix imap_delete_mailbox() double free and pass by value.

[Kevin J. McCarthy]

On Wed, Sep 30, 2026 at 09:38:56AM +0200, Alejandro Colomar via Mutt-dev wrote:

imap_delete_mailbox() was passing an IMAP_MBOX parameter
by value.  Change this to pass by reference.

For this part:

        Reviewed-by: Alejandro Colomar <a...@kernel.org>

Thank you!

 Additionally,
imap_delete_mailbox() was free'ing mx.mbox in an error handler, which
the caller was also doing, leading to a possible double free.  So the
free inside the function was removed.

I think this part is wrong; see below.

---
 browser.c   | 2 +-
 imap/imap.c | 7 +++----
 imap/imap.h | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/browser.c b/browser.c
index 7f3498ce..77d64174 100644
--- a/browser.c
+++ b/browser.c
@@ -1234,7 +1234,7 @@ void _mutt_buffer_select_file(BUFFER *f, int flags, char 
***files, int *numfiles
                    mx.mbox);
           if (mutt_yesorno(msg, MUTT_NO) == MUTT_YES)
           {
-            if (!imap_delete_mailbox(Context, mx))
+            if (!imap_delete_mailbox(Context, &mx))
             {
               /* free the mailbox from the browser */
               FREE(&((state.entry)[nentry].display_name));

This FREE() call happens on success (imap_delete_mailbox(...) == 0).

The double free in question is actually a bit further down, at
https://gitlab.com/muttmua/mutt/-/blob/kevin%2Faccount-dynamic-fields/browser.c#L1260

diff --git a/imap/imap.c b/imap/imap.c
index cd3edcd9..a86a8379 100644
--- a/imap/imap.c
+++ b/imap/imap.c
@@ -153,17 +153,16 @@ int imap_rename_mailbox(IMAP_DATA *idata, IMAP_MBOX *mx, 
const char *newname)
   return rc;
 }

-int imap_delete_mailbox(CONTEXT *ctx, IMAP_MBOX mx)
+int imap_delete_mailbox(CONTEXT *ctx, IMAP_MBOX *mx)
 {
   char buf[LONG_STRING*2], mbox[LONG_STRING];
   IMAP_DATA *idata;

   if (!ctx || !ctx->data)
   {
-    if (!(idata = imap_conn_find(&mx.account,
+    if (!(idata = imap_conn_find(&mx->account,
                                  option(OPTIMAPPASSIVE) ? MUTT_IMAP_CONN_NONEW 
: 0)))
     {
-      FREE(&mx.mbox);
       return -1;

This FREE() call happens on error (it returns -1).

It seems to me that they are not duplicate.  Did I miss anything?  I've
only done a quick look, so it could be.

I appreciate you taking a look, even if it was just quick!  :-)

--
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA

signature.asc
Description: PGP signature