During certain operations, when Mutt is prepared for the context to
change, IMAP_REOPEN_ALLOW is set. This allows headers to be expunged
and new emails to be processed.
In this state, it's easy to forget that simple IMAP command
processing, such as imap_cmd_step() can trigger nested functions that
open idata->hcache(), and then close it when finished.
Before, if the current function opens idata->hcache and forgets to
close/reopen it around a potential nested operation, a segfault or
incorrect behavior could happen because a nested function closed the
idata->hcache.
Create a new function to open the idata->hcache along with a counter
to track nested reopening. Remove no longer needed close/reopen calls
in imap_sync_mailbox() and read_headers_condstore_qresync_updates().
Ensure the hcache is closed and the counter is zeroed in
imap_close_mailbox().
---
imap/imap.c | 47 +++++++++------------------------------------
imap/imap_private.h | 4 +++-
imap/message.c | 8 +++-----
imap/util.c | 18 ++++++++++++++---
4 files changed, 30 insertions(+), 47 deletions(-)
diff --git a/imap/imap.c b/imap/imap.c
index 0778a5e6..730fa16e 100644
--- a/imap/imap.c
+++ b/imap/imap.c
@@ -263,7 +263,7 @@ void imap_expunge_mailbox(IMAP_DATA *idata)
short old_sort;
#ifdef USE_HCACHE
- idata->hcache = imap_hcache_open(idata, NULL);
+ imap_idata_hcache_open(idata);
#endif
old_sort = Sort;
@@ -323,7 +323,7 @@ void imap_expunge_mailbox(IMAP_DATA *idata)
}
#if USE_HCACHE
- imap_hcache_close(idata);
+ imap_idata_hcache_close(idata);
#endif
/* We may be called on to expunge at any time. We can't rely on the caller
@@ -1519,7 +1519,7 @@ int imap_sync_mailbox(CONTEXT *ctx, int expunge, int
*index_hint)
}
#if USE_HCACHE
- idata->hcache = imap_hcache_open(idata, NULL);
+ imap_idata_hcache_open(idata);
#endif
/* save messages with real (non-flag) changes */
@@ -1545,30 +1545,6 @@ int imap_sync_mailbox(CONTEXT *ctx, int expunge, int
*index_hint)
* This works better if we're expunging, of course. */
if (h->env->changed || h->attach_del)
{
- /* NOTE and TODO:
- *
- * The mx_open_mailbox() in append mode below merely hijacks an
existing
- * idata; it doesn't reset idata->ctx. imap_append_message() ends up
- * using (borrowing) the same idata we are using.
- *
- * Right after the APPEND operation finishes, the server can send an
- * EXISTS notifying of the new message. Then, while still inside
- * imap_append_message(), imap_cmd_step() -> imap_cmd_finish() will
- * call imap_read_headers() to download those (because the idata's
- * reopen_allow is set).
- *
- * The imap_read_headers() will open (and clobber) the idata->hcache we
- * just opened above, then close it.
- *
- * The easy and less dangerous fix done here (for a stable branch bug
- * fix) is to close and reopen the header cache around the operation.
- *
- * A better fix would be allowing idata->hcache reuse. When that is
- * done, the close/reopen in read_headers_condstore_qresync_updates()
- * can also be removed. */
-#if USE_HCACHE
- imap_hcache_close(idata);
-#endif
if (!ctx->quiet)
mutt_message(_("Saving changed messages... [%d/%d]"), n+1,
ctx->msgcount);
@@ -1579,15 +1555,12 @@ int imap_sync_mailbox(CONTEXT *ctx, int expunge, int
*index_hint)
else
_mutt_save_message(h, appendctx, 1, 0, 0);
h->env->changed = 0;
-#if USE_HCACHE
- idata->hcache = imap_hcache_open(idata, NULL);
-#endif
}
}
}
#if USE_HCACHE
- imap_hcache_close(idata);
+ imap_idata_hcache_close(idata);
#endif
/* presort here to avoid doing 10 resorts in imap_exec_msgset.
@@ -1766,6 +1739,9 @@ int imap_close_mailbox(CONTEXT *ctx)
}
mutt_bcache_close(&idata->bcache);
+ mutt_hcache_close(idata->hcache);
+ idata->hcache = NULL;
+ idata->hcache_open_count = 0;
}
/* free IMAP part of headers */
@@ -1874,17 +1850,12 @@ static int imap_save_to_header_cache(CONTEXT *ctx,
HEADER *h)
{
int rc = 0;
#ifdef USE_HCACHE
- int close_hc = 1;
IMAP_DATA *idata;
idata = (IMAP_DATA *)ctx->data;
- if (idata->hcache)
- close_hc = 0;
- else
- idata->hcache = imap_hcache_open(idata, NULL);
+ imap_idata_hcache_open(idata);
rc = imap_hcache_put(idata, h);
- if (close_hc)
- imap_hcache_close(idata);
+ imap_idata_hcache_close(idata);
#endif
return rc;
}
diff --git a/imap/imap_private.h b/imap/imap_private.h
index f4755823..f43acb62 100644
--- a/imap/imap_private.h
+++ b/imap/imap_private.h
@@ -234,6 +234,7 @@ typedef struct
LIST *flags;
#ifdef USE_HCACHE
header_cache_t *hcache;
+ unsigned int hcache_open_count;
#endif
} IMAP_DATA;
/* I wish that were called IMAP_CONTEXT :( */
@@ -299,7 +300,8 @@ int imap_commit_message(CONTEXT *ctx, MESSAGE *msg);
/* util.c */
#ifdef USE_HCACHE
header_cache_t *imap_hcache_open(IMAP_DATA *idata, const char *path);
-void imap_hcache_close(IMAP_DATA *idata);
+void imap_idata_hcache_open(IMAP_DATA *idata);
+void imap_idata_hcache_close(IMAP_DATA *idata);
HEADER *imap_hcache_get(IMAP_DATA *idata, unsigned int uid);
int imap_hcache_put(IMAP_DATA *idata, HEADER *h);
int imap_hcache_del(IMAP_DATA *idata, unsigned int uid);
diff --git a/imap/message.c b/imap/message.c
index d953a9a1..1924ab21 100644
--- a/imap/message.c
+++ b/imap/message.c
@@ -267,7 +267,7 @@ retry:
idata->newMailCount = 0;
#if USE_HCACHE
- idata->hcache = imap_hcache_open(idata, NULL);
+ imap_idata_hcache_open(idata);
if (idata->hcache && initial_download)
{
@@ -423,7 +423,7 @@ retry:
bail:
#if USE_HCACHE
- imap_hcache_close(idata);
+ imap_idata_hcache_close(idata);
FREE(&uid_seqset);
#endif /* USE_HCACHE */
@@ -730,14 +730,12 @@ static int
read_headers_condstore_qresync_updates(IMAP_DATA *idata,
if (idata->reopen & IMAP_EXPUNGE_PENDING)
{
short old_sort;
- imap_hcache_close(idata);
old_sort = Sort;
Sort = SORT_ORDER;
imap_expunge_mailbox(idata);
Sort = old_sort;
- idata->hcache = imap_hcache_open(idata, NULL);
idata->reopen &= ~IMAP_EXPUNGE_PENDING;
}
@@ -810,7 +808,7 @@ fail:
mutt_hcache_delete(idata->hcache, "/MODSEQ", imap_hcache_keylen);
imap_hcache_clear_uid_seqset(idata);
- imap_hcache_close(idata);
+ imap_idata_hcache_close(idata);
if (!ctx->quiet)
{
diff --git a/imap/util.c b/imap/util.c
index 29f88a1b..d3a7c388 100644
--- a/imap/util.c
+++ b/imap/util.c
@@ -200,13 +200,25 @@ cleanup:
return rv;
}
-void imap_hcache_close(IMAP_DATA *idata)
+void imap_idata_hcache_open(IMAP_DATA *idata)
+{
+ if (!idata->hcache_open_count)
+ idata->hcache = imap_hcache_open(idata, NULL);
+ if (idata->hcache)
+ idata->hcache_open_count++;
+}
+
+void imap_idata_hcache_close(IMAP_DATA *idata)
{
if (!idata->hcache)
return;
- mutt_hcache_close(idata->hcache);
- idata->hcache = NULL;
+ idata->hcache_open_count--;
+ if (!idata->hcache_open_count)
+ {
+ mutt_hcache_close(idata->hcache);
+ idata->hcache = NULL;
+ }
}
HEADER *imap_hcache_get(IMAP_DATA *idata, unsigned int uid)
--
2.55.0
