Re: [PATCH] bugfix for handling incomplete IMAP header cache

[Kevin J. McCarthy]

On Sat, Aug 21, 2021 at 07:15:12AM -0700, Kevin J. McCarthy wrote:

Thank you - good catch.  I'll apply this as a separate patch today.


I've pushed up two commits to branch kevin/stable-imap-header-cache-hole 
on 
gitlab. . 
The first is your msn_begin fix commit and the second is a currently 
untested commit changing the sort order before generating msgset 
strings.


I'm going to stare at it a while, and test it the rest of this weekend. 
If you have the time inclination please feel free to give it try too.


Thank you.

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


signature.asc
Description: PGP signature

Re: [PATCH] bugfix for handling incomplete IMAP header cache

[Pieter-Tjerk de Boer via Mutt-dev]

On Sat, Aug 21, 2021 at 07:15:12AM -0700, Kevin J. McCarthy wrote:
> On Sat, Aug 21, 2021 at 11:35:19AM +0200, Pieter-Tjerk de Boer via Mutt-dev wr

> > This bug isn't just cosmetic; it can lead to unintended deletion of
> > large amounts of mails.
>
> I'm very sorry to hear about the loss of email.  That's the most terrible
> bug there can be.  :-(

Thanks; indeed, I was not amused, and that's an understatement... :-(

> > Fortunately, fixing this is trivial; at two places in imap/message.c,
> > replace
> >ctx->hdrs[idx]->index = idx;
> > by
> >ctx->hdrs[idx]->index = h.data->msn-1;
>
> Unfortunately I don't think this is the correct fix.  There can in fact be
> holes in the MSN sequence.

I'm by no means an IMAP or mutt code expert (never touch mutt code, nor
studied the IMAP RFCs, until trying to fix this bug), but in RFC3501,
section 2.3.1.2, the MSN is described as "A relative position from 1 to
the number of messages in the mailbox". This implies there can be no
holes, or am I overlooking something?

> I think a better fix would be changing the sort order to use UID instead
> when creating the msgset,

Indeed, that would be a more robust solution to the message delete problem,
as it then no longer relies on MSN and UID being monotonically related.

But I think it wouldn't solve the problem that the unsuccesfully-deleted
message jumps to a different position in the unsorted mailbox view.

Regards,
  Pieter-Tjerk

[PATCH] bugfix for handling incomplete IMAP header cache

[Pieter-Tjerk de Boer via Mutt-dev]

The below patch fixes mutt's IMAP message number handling in case it is
started with a header cache in which a mail somewhere "in the middle"
is missing (i.e., the cache does contain a mail later than the one that
is missing).
This bug isn't just cosmetic; it can lead to unintended deletion of
large amounts of mails.

Let me give a step-by-step example of what went wrong and how that
could lead to losing mail:

0. Use mutt with IMAP, header caching enabled, but not CONDSTORE
   nor QRESYNC.
1. Open an IMAP mailbox with e.g. 5 mails in it, with e.g. UIDs 11...15.
2. Mark the 3rd mail, UID=13, for deletion.
3. Give the sync-mailbox command; mutt immediately deletes the mail from
   the header cache and tells the IMAP server to also delete it.
4. Assume that the server does not actually do the synchronization;
   this could e.g. happen due to a network problem, or the server could
   simply say "NO" (as the server I'm using sometimes does).
   Now the mail with UID=13 is missing from the header cache, but it is
   still on the server.
5. Exit and restart mutt.
6. Mutt will first populate its view of the mailbox from its header cache,
   then request headers for the missing UID=13 mail, and _append_ the 
   UID=13 mail after the other mails.
   So in mutt's view the mailbox contains UIDs in the order 11,12,14,15,13.
   Note that this order violates the IMAP specification (and assumption
   in the code) that UIDs are monotonically increasing.
7. The user sees that mail 13 has jumped to the end of the mailbox, if
   not otherwise sorted; this may be confusing or surprising, but by
   itself it's not a serious problem.
8. Assume a new mail arrives, with UID 16.
   In mutt's view, the mailbox now contains UIDs 11,12,14,15,13,16.
9. Mark mails 13 and 16 for deletion.
10. Give the sync-mailbox command. In mutt's view, the two mails to be
   deleted are consecutive in mailbox order, and therefore it will tell
   the server to delete the entire UID range 13:16 .
11. Sadly, this inadvertently also deletes mails 14 and 15 !

This is not just theory. I've several times lost hundreds of mails due
to (presumably) this bug (though of course I wasn't yet aware of exactly
what was happening).

Fortunately, fixing this is trivial; at two places in imap/message.c,
replace
ctx->hdrs[idx]->index = idx;
by
ctx->hdrs[idx]->index = h.data->msn-1;
I.e., instead of writing the array index into the index field, write the
correct message number into it; this field is defined as "the absolute
(unsorted) message number" in mutt.h.

I also checked what happens if CONDSTORE and QRESYNC are enabled.
In that case, verify_qresync() notices that the quick resync didn't work
correctly, but the subsequent attempt at a normal resync fails in a
different way: it doesn't fetch the headers for the missing mail.
To the user, it then looks as if the mail is no longer in the mailbox,
even though the server still has it.
The below patch fixes this too.

Regards,
  Pieter-Tjerk


---
 imap/message.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/imap/message.c b/imap/message.c
index 103e48e5..42a9414d 100644
--- a/imap/message.c
+++ b/imap/message.c
@@ -240,6 +240,7 @@ int imap_read_headers (IMAP_DATA* idata, unsigned int 
msn_begin, unsigned int ms
   void *pmodseq = NULL;
   unsigned long long hc_modseq = 0;
   char *uid_seqset = NULL;
+  unsigned int msn_begin_original = msn_begin;
 #endif /* USE_HCACHE */
 
   ctx = idata->ctx;
@@ -357,6 +358,7 @@ retry:
   FREE (_seqset);
   uid_validity = 0;
   uidnext = 0;
+  msn_begin = msn_begin_original;
 
   goto retry;
 }
@@ -509,7 +511,7 @@ static int read_headers_normal_eval_cache (IMAP_DATA *idata,
 idata->msn_index[h.data->msn - 1] = ctx->hdrs[idx];
 int_hash_insert (idata->uid_hash, h.data->uid, ctx->hdrs[idx]);
 
-ctx->hdrs[idx]->index = idx;
+ctx->hdrs[idx]->index = h.data->msn - 1;
 /* messages which have not been expunged are ACTIVE (borrowed from mh
  * folders) */
 ctx->hdrs[idx]->active = 1;
@@ -943,7 +945,7 @@ static int read_headers_fetch_new (IMAP_DATA *idata, 
unsigned int msn_begin,
 idata->msn_index[h.data->msn - 1] = ctx->hdrs[idx];
 int_hash_insert (idata->uid_hash, h.data->uid, ctx->hdrs[idx]);
 
-ctx->hdrs[idx]->index = idx;
+ctx->hdrs[idx]->index = h.data->msn - 1;
 /* messages which have not been expunged are ACTIVE (borrowed from mh
  * folders) */
 ctx->hdrs[idx]->active = 1;
-- 
2.20.1