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

[Kevin J. McCarthy]

On Mon, Aug 23, 2021 at 10:48:59AM +0200, Oswald Buddenhagen wrote:

On Sun, Aug 22, 2021 at 06:35:46AM -0700, Kevin J. McCarthy wrote:
I agree the server wasn't behaving nicely.  It simply omitted some 
of the headers.  (Probably they were deleted at some point between 
the "assembly" of the mailbox on its side and sending all the 
headers - the mailbox was very large and changing constantly I 
think.)  However the MSNs didn't repeat or exceed the initial max 
MSN reported, so I think it might be debatable if it was broken.


unless the problem was misdiagnosed due to mutt's tracking of 
unsolicited EXPUNGE and EXISTS responses being actually broken,


No, I remember going through the log.  It was during the initial connect 
header FETCH.  There were gaps in the result sequence numbers, e.g. 1, 
2, 3, 5, 6, 8, 9,  There weren't any EXPUNGE responses (which would 
have been illegal during the FETCH processing in any case.)


that server would be most definitely broken. there is absolutely no 
wiggle room here.


Okay, that's clear enough, and I believe you.  :-)

So, Pieter-Tjerk, perhaps I'm being too strict about the index 
assignment then.  Again, I don't want to make that change for stable, 
(and in any case I think we should be sorting by UID to prevent any 
other possible issues like this in the future).  But let me take a 
second look for master.


My concern is violating that general assumption that index is in the 
range of 0..msgcount-1.  The other mail backends make use of that, but 
that's independent of the IMAP code of course.


I looked at the Index Menu usage a while ago, and I think I came to the 
conclusion it might be safe, but there was some ambiguous usage in 
there.  I will take a second look at that.


--
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

[Oswald Buddenhagen]

On Sat, Aug 21, 2021 at 12:50:20PM -0700, Kevin J. McCarthy wrote:

On Sat, Aug 21, 2021 at 09:32:11PM +0200, Pieter-Tjerk de Boer via Mutt-dev 
wrote:

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?


This was observed in Trac ticket 3942: 




wow. mr. crispin would NOT have been amused ...

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

[Kevin J. McCarthy]

On Sun, Aug 22, 2021 at 12:55:49PM +0200, Pieter-Tjerk de Boer via Mutt-dev 
wrote:

On Sat, Aug 21, 2021 at 12:50:20PM -0700, Kevin J. McCarthy wrote:

This was observed in Trac ticket 3942: 



Ouch, that seems a rather severely broken IMAP server implementation then.
The assumption, made by mutt and discussed in that ticket, namely that the
MSNs are consecutive and can't exceed the message count, is in fact well
supported by the RFC: not just by the sentence I quoted but also by the
further examples given in that RFC.


I agree the server wasn't behaving nicely.  It simply omitted some of 
the headers.  (Probably they were deleted at some point between the 
"assembly" of the mailbox on its side and sending all the headers - the 
mailbox was very large and changing constantly I think.)  However the 
MSNs didn't repeat or exceed the initial max MSN reported, so I think it 
might be debatable if it was broken.



Well, as a user, I would expect 'unsorted' to match the order the messages
have on the IMAP server. This is practically useful, as it ensures that the
latest additions to the mailbox are all together at the end.

Could we solve this by applying my previous patch, and adding a check at
the end that the index values (derived from MSN as per my patch) are in
fact consecutive, and modifying them as needed?


I wouldn't be willing to do this for these stable branch fixes.  I'd 
have to think about it for master.  My inclination is to leave the index 
separate from MSN.  The mis-order is not an ordinary situation, so I 
don't think it's worth reintroducing the complication for this case.



On Sat, Aug 21, 2021 at 01:21:37PM -0700, Kevin J. McCarthy wrote:


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.


Nice!
For what it's worth, I tested it the same way as I tested my own earlier
patch (triggering the bug by temporarily making the IMAP server unreachable),
and it works okay in that case.


Thank you for testing the fixes.  I'm going to test and look at it some 
more the next few days, but will get a stable release out with these 
fixes this week.


--
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

[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