IMAP client seems to struggle with very large UIDs - handle UID's > 0x7FFFFFFF (take 2)

RESOLVED FIXED in Thunderbird 57.0

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: druggeri, Assigned: jorgk)

Tracking

Thunderbird 57.0

Thunderbird Tracking Flags

(thunderbird_esr5256+ fixed, thunderbird56 fixed, thunderbird57 fixed)

Details

(Whiteboard: TB 56 beta 3 => TB 52.4.0 ESR)

Attachments

(3 attachments, 1 obsolete attachment)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

An unknown condition occurred on the mailserver where the UID used after ID 9095 was suddenly 2273345706.


Actual results:

Ever since the mailserver decided to start incrementing UID from 2273345706, I am unable to receive new messages. I AM able to apparently operate normally otherwise (view/delete/move/copy messages in INBOX and other folders), but no new messages ever arrive.

This exceedingly large UID seems to have jammed up Thunderbird IMAP processing. I am wondering if the number is too large to fit into the memory space allocated for the UID?


Expected results:

Since I am able to read new messages in my provider's webmail client AND K-9 mail on Android, I expect Thunderbird should have been able to read the messages as well. Attached is a conversation log captured with wireshark. Notice that there is never an attempted fetch for 54 (UID 2273345706).

The conversation log is also available on pastebin: https://pastebin.com/3kZTAj4M
Another case for Gene to look at.
Flags: needinfo?(gds)
2273345706 = 0x878080AA so as a 32bit integer, that would be negative. However, for UIDs the nsMsgKey type seems to be used with |typedef unsigned long nsMsgKey;|.

However, we also have code that goes: |const nsMsgKey nsMsgKey_None = 0xffffffff;|, so no message is one with the highest number available.

I also spotted |uint32_t highestRecordedUID = GetServerStateParser().HighestRecordedUID();| so that seems to indicate that we used unsigned 32bit integers.

I can also see
* 54 FETCH (UID 2273345706 FLAGS (\Recent))
19 OK Fetch completed.
20 UID fetch 2147483647 (UID RFC822.SIZE FLAGS BODY.PEEK[HEADER.FIELDS (From To Cc Bcc Subject Date Message-ID Priority X-Priority References Newsgroups In-Reply-To Content-Type Reply-To)])
20 OK Fetch completed.
in the log.

Can you run an IMAP log for us: https://wiki.mozilla.org/MailNews:Logging#Environment_Variables_to_set

On Windows, for example set
set NSPR_LOG_MODULES=IMAP:5,timestamp
set NSPR_LOG_FILE=D:\Desktop\imap.log
before starting TB from the command line.
Thanks for the quick reply, Jorg!

Agreed, a 32-bit int would overflow, but 32-bit uint should not on my platform. See notes here for usable sizes for MSWindows:
https://msdn.microsoft.com/en-us/library/296az74e.aspx

As a side note, notice that both int and long as well as uint and ulong have the same allowable range. This makes sense since they are both represented by 32 bits on Windows. Presumably the "best" type to use on MSWindows would be "UINT64" (which I think is __uint64) according to this article:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384264(v=vs.85).aspx

... which is NOT backwards compatible with 32-bit versions of Windows.


I have gathered the log file as requested. Apologies for omitting it - I was unsure which modules to enable and am concerned about privacy. From my fairly uneducated view, nothing stands out to me to be an obvious problem in the log file.
Interesting reading in that log.

2273345706 is mentioned once:
2017-08-17 10:41:08.195000 UTC - 6360[19c91d10]: 19b93000:mail.primary.net:S-INBOX:CreateNewLineFromSocket: * 53 FETCH (UID 2273345706 FLAGS (\Seen))

And there are also:
2017-08-17 10:41:08.137000 UTC - 6360[19c91d10]: 19b93000:mail.primary.net:A:CreateNewLineFromSocket: * OK [UIDNEXT 2273345730] Predicted next UID

and
2017-08-17 10:41:08.196000 UTC - 6360[19c91d10]: 19b93000:mail.primary.net:S-INBOX:SendData: 8 UID fetch 2147483647,2147483647,2147483647 (UID RFC822.SIZE FLAGS BODY.PEEK[HEADER.FIELDS (From To Cc Bcc Subject Date Message-ID Priority X-Priority References Newsgroups In-Reply-To Content-Type Reply-To)])

Maybe also something Aceman could look at.
Flags: needinfo?(acelists)
The rfc just says UID is a 32-bit value. No mention of signed or unsigned so I would assume it implies unsigned.

The wireshark log shows where the 2273345706 message is detected which matches the UIDNEXT predicted value that seems correct.

What seems strange to me is the UID fetch 2147483647 ...  Where is that UID coming from? The tb log shows it fetched 3 times:

8 UID fetch 2147483647,2147483647,2147483647 ...

Is this a logging error? 

Whatever this means, there appears to be no response to it other than OK in the wireshark and tb log.
Flags: needinfo?(gds)
Hi, Gene;
Interesting observation - I didn't even realize that! 2147483647 is highest value for a 32-bit signed int.


2017-08-17 10:41:08.195000 UTC - 6360[19c91d10]: 19b93000:mail.primary.net:S-INBOX:CreateNewLineFromSocket: * 53 FETCH (UID 2273345706 FLAGS (\Seen))
2017-08-17 10:41:08.195000 UTC - 6360[19c91d10]: 19b93000:mail.primary.net:S-INBOX:CreateNewLineFromSocket: * 54 FETCH (UID 2273345726 FLAGS (\Seen))
2017-08-17 10:41:08.195000 UTC - 6360[19c91d10]: 19b93000:mail.primary.net:S-INBOX:CreateNewLineFromSocket: * 55 FETCH (UID 2273345727 FLAGS (\Seen))

This above chunk shows three messages, all higher than 2147483647. My working theory is that when the array of characters is converted using language builtins like atoi, truncation is occurring and the resulting value is the largest potential integer value. Since I am on windows platform, I looked on MSDN and came across this documentation:
https://msdn.microsoft.com/en-us/library/yd5xkb5c.aspx#Return%20Value

Indeed, that would indicate that if a pointer to a character array containing a value larger than INT_MAX were passed to atoi that INT_MAX would be returned (see previous note about what those constants are on windows). It seems the better function for this while still being compliant with ANSI spec is strtoul (which, on windows at least, is the same size as unsigned int)... assuming the code's using atoi and I'm not just making things up :-)




Placed here for convenience is a pointer to the IMAP RFC defining UID as a 32-bit value:
https://tools.ietf.org/html/rfc3501#section-2.3.1.1
Daniel, I think you are right. That explains why there are 3 fetches of UID = MAX_LONG! I see several places in the tb imap code where uid is obtained from atoi(). However, I also found an old tb bug report indicating problems with uid > 0x7fffffff have been fixed: Bug 518918. But apparently not. I agree, it seems strtoul() should be used instead. I also found a post where Mark Crispin (imap inventor and imap rfc author) verifies that UIDs are "clearly" uint32_t (unsigned 32-bit ints): http://mailman13.u.washington.edu/pipermail/imap-protocol/2007-January/000355.html. He says it clearly states it but it never really does mention unsigned, afaict).
Posted patch 1391128-strtoul.patch (v1). (obsolete) — Splinter Review
I've checked all the uses of atoi() and changed them to strtoul() where the assignment was to an uint32_t. The remaining atoi() assign to int32_t.

Uncompiled and untested.

Reporter, are you willing to try a specially compiled Daily TB 57 to see whether that fixes the problem.
Assignee: nobody → jorgk
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(acelists)
Attachment #8898671 - Flags: review?(acelists)
Attachment #8898671 - Flags: feedback?(gds)
See Also: → 518918
Summary: IMAP client seems to struggle with very large UIDs → IMAP client seems to struggle with very large UIDs - handle UID's > 0x7FFFFFFF (take 2)
Damn, that didn't compile, takes three arguments.
Attachment #8898671 - Attachment is obsolete: true
Attachment #8898671 - Flags: review?(acelists)
Attachment #8898671 - Flags: feedback?(gds)
Attachment #8898679 - Flags: review?(acelists)
Attachment #8898679 - Flags: feedback?(gds)
If the bug here is that there were some places that limited the UID to MAX_INT (2^31-1) instead of MAX_UINT (2^32-1), than fixing that is reasonable. I think UIDs are 32bit long, unsigned, and it seems the linked articles confirm that.
I think Jcranmer knows the details of this.
I also remember there are more internal problems to overcome if we wanted support for 64bit UIDs that some servers start to supports. But it may also be I have now mistaken IMAP UIDs with News UIDs...
Flags: needinfo?(Pidgeot18)
When will you understand, that Joshua normally doesn't respond? We can't hold proceedings waiting for him. Converting to a 32 bit ulong with atoi() is just plain foolish and ought to be fixed pronto without further ado, and uplifted to ESR asap.

Fingers crossed the patch fixes the problem and the reporter can confirm that with the try build available here (when complete):
https://archive.mozilla.org/pub/thunderbird/try-builds/mozilla@jorgk.com-7449542d0797d6a07e75a37c7173e2872c71c689/
Flags: needinfo?(druggeri)
Green try.
Hi, Jorg and all;
   Happy to try the daily builds - especially when they are bundled as a zipfile allowing me to test without installing.

   I can confirm that this nightly build works as expected. I now see the several messages in Thunderbird the current stable build is unable to grab.
Thanks for confirming, and of course thanks to Gene for the excellent and quick analysis! We're lucky that this is all that is needed apparently.

Yes, the ZIP options is great when you don't want to install. However, how will you manage your messages in a version that can't access them? I'll fast-track this to TB 52.4 but that will be released in six weeks since we've just brought out TB 52.3.

As for the Daily version, it's actually quite stable and you might consider using it for real. The only drawback is that some internal interfaces have changed from Mozilla core 52 to 57, so some add-ons don't work in TB 57, like QuickText. Unless you're a heavy add-on user, I recommend TB 57 Daily, but I also suggest to turn off updates, since at times Dailies don't work at all and you want to avoid catching one like that. Or if you're more comfortable with a beta, I can get this into a TB 56 beta in the next few days. Personally I find it pretty embarrassing that in seasoned software like TB we still don't handle UID's correctly after decades. Are you the only person in the universe with this problem?
Flags: needinfo?(druggeri)
Thanks for testing, I will look at the patch soon.
From IRC:
17:41 jcranmer aceman: I don't know anything specific re: IMAP UIDs
Flags: needinfo?(Pidgeot18)
Comment on attachment 8898679 [details] [diff] [review]
1391128-strtoul.patch (v2)

Review of attachment 8898679 [details] [diff] [review]:
-----------------------------------------------------------------

OK, great if it works. But is it a problem that unsigned long may be 64 bits long on some platforms? Now the strtoul may try to overflow the key. I don't know if atoi() could do that if the input string was large enough. IT seems from the report here, that it limits the returned value to INT_MAX.
So is this a potential problem if we hit an UID larger than 2^32-1 ?
Attachment #8898679 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #19)
> So is this a potential problem if we hit an UID larger than 2^32-1 ?
No, because the IMAP spec says 32bit for the UID: https://tools.ietf.org/html/rfc3501#section-2.3.1.1
Ah yes, sorry :)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/74b47ccb34f6
Use strtoul() instead of atoi() when converting UID to uint32_t. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Reporter, would you like this in a special TB 56 beta version?
Target Milestone: --- → Thunderbird 57.0
Comment on attachment 8898679 [details] [diff] [review]
1391128-strtoul.patch (v2)

This quite obviously needs uplift with little risk since we're just reading a textual integer representation better.
Attachment #8898679 - Flags: feedback?(gds)
Attachment #8898679 - Flags: approval-comm-esr52+
Attachment #8898679 - Flags: approval-comm-beta+
I apologize for the delay in response. I have not decided yet if I will use beta or daily because I have other usable methods such as Webmail and mobile telephone to manage emails. From my perspective, the biggest pain is not having the message filters running since I tend to read most messages on mobile... and that's not a huge issue for me. Given the regular and predictable release cadence, when would a likely time be for this hitting the stable branch?

To Jorg, that is an interesting question, and I bet that I am not the only person "ever" to experience this... but who knows? I do suspect that because the condition is unlikely to occur (with most mail servers NOT choosing a seemingly random huge UID out of the blue to rebase on) and most home users not being familiar with bug analysis and reporting techniques, that it just hasn't come up here or been made clear to be the issue as we've determined. I am still following up with my email provider as to why the server started incrementing UID with such a wide gap from previous UIDs, but this kind of strange condition is unsurprising given other unusual issues I've had with this particular provider.
I do want to say... from my perspective as an Open Source developer on a few projects (some huge, some small) I am highly impressed by how the TB project handled this bug report - especially a project of this size and tenure. Spectacular work all around. As a user, big thanks for the responsiveness. As a dev, big thanks for the inspiration :-)
Beta (TB 56):
https://hg.mozilla.org/releases/comm-beta/rev/044fa4c195e66d495c946d3d97bc19b99a39e59c

Thanks for your kind words. We don't turn all bugs around this quickly, but some even faster. Key here was Gene's analysis, so thanks again to him, much appreciated.

Turns out that we're doing another beta, TB 56 beta 3 for some other reason, so this might be as well be included.

The next "stable" TB 52.4 is scheduled for the week after 20th Sept. 2017, so four weeks away unless something terrible happens in 52.3 and we need to do a 52.3.1. Details: https://wiki.mozilla.org/RapidRelease/Calendar
Whiteboard: TB 56 beta 3 => TB 52.4.0 ESR
You need to log in before you can comment on or make changes to this bug.