223942 - Can't download all messages from my IMAP account [Negative UID]

Status: RESOLVED DUPLICATE of bug 518918

(MailNews Core :: Networking: IMAP, defect)

Description

[Roman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007

If i have 100 Mails on my imap server [Mercur 4.2 www.atrium-software.com] and i
would like to download these all to my client, mozilla crashs. I talked to the
developers of the Mercur and they told me there is a problem by mozilla. he use
negative UID's. This looks it came from a unsignet integer who is overruned.

Reproducible: Always

Steps to Reproduce:
1.copy 100 Mails on an  Imap account on Mercure Mailserver v.4.2
2.try to view one of these mails with mozilla
3.Mozilla would maximal download 30 Messages. In no case it download all messages.

Actual Results:  
In some Cases Mozilla try to donload messages till it crashs, sometimes it runns
normaly. but not all messages are here.

Expected Results:  
Mey be it is only the Variable of the UID who have to be bigger, like long or double

Comment 1

[Henrik Gemal]

please attach a imap protocol log:
http://mozilla.org/quality/mailnews/mail-troubleshoot.html#imap

Comment 2

[David :Bienvenu]

besides a protocol log, a test account would be useful as well. In the past,
negative UID's have worked, I believe.

Comment 3

[Patrick]

[tracking and removed ALL CAPS from bug summary]

Comment 4

[Stephan Gropp]

I have the same problem with Windows/XP and Mozilla 1.5.
Error message is mentioning just an "invalid sequence in UID" right after I'm
starting the download of eMails. It even doesn't synchronize one message.

Comment 5

[Roman]

Attachment: Imap Log file

here is a logfile, the problem occure when i would like to receive mails form
the account info2sk8shop.ch and specialli if i would see the contents of
Erledigt_Roman

Thanky a lot

Comment 6

[Konstantin Buharov]

The same for Thunderbird 1.0/Windows, Thunderbird 1.0/Linux and Mozilla
1.7,1.8a3/Linux.

Server:
* OK MERCUR IMAP4-Server (v4.02.09 Mjk4MzUtNjI3Mi0xOTQ3MA==) for Windows NT
ready at Mon, 10 Jan 2005 16:23:04 +0300

Problem in UIDs higher than 0x7fffffff - somewhere in Mozilla code they were
treated as "signed int".

There are also possible duplicates for this bug
https://bugzilla.mozilla.org/show_bug.cgi?id=277905

Comment 7

[Lars Fredriksen]

Just ran into this with Thunderbird 2.0.0.6 on FredBSD with imap-uw-2006j_3,1. imap-uw does checks for negative UDS and you get a Bogus sequence UID error message from imapd.

If you look in mailnews/imap/src, there seems to be 2 issues:
1) there are several methods that use atoi() to convert a string UID to an int then assigns it to an unsigned. For most practical things, this is probably not going to matter much - but it should use stroul() instead.
2) there are several methods that use the AppendInt() method to convert an unsigned int 32 UID to a string.  In xpcom/string/public/nsTString.h we declare AppendInt() with an unsigned parameter to call the default AppendInt() method with the unsigned int parameter casted to an int. This will cause a UID higher then 0x7fffffff to end up in the string as a negative number which imap-uw do not like. Adding a AppendInt method in xpcom/string/src/nsStringObsolete.cpp  that accepts a unsigned int as a parameter, and uses %u instead of %d in the sprintf() statement seems to fix the problem (at least partially). 

After having done this, thunderbird will now get all the headers, but it will go into a loop once you try to click on a header to read the mail.  I am not sure yet what that is all about. 

I would suggest you increase the priority on this bug to get it fixed. This recommendation is based on the assumption that IMAP UIDs are defined in the protocol to be Unsigned. Prior versions of imap-uw treated UIDs as signed ints, and thus did not complain.....and thus thunderbird and imap-uw did things the same way ...

Comment 8

[Lars Fredriksen]

Found thunderbird to be looping because a couple of methods were using PRInt32 to hold the current UID. Fixed those and now things seems to work just fine. I have changes to 8 files in mailnews/imap/src: nsIMAPBodyShell.cpp, nsImapMailFolder.cpp nsImapProtocol.cpp, nsImapServerResponseParser.cpp nsImapService.cpp nsImapUndoTxn.cpp nsImapUrl.cpp and nsImapUtils.cpp. I also went back and explicitly called AppendUInt() instead of relying on overloading AppendInt().

Comment 9

[Andy Lyttle]

I've been discussing this issue with Mark Crispin, the developer of uw-imapd.  According to RFC 3501, it's definitely supposed to be an unsigned integer, but both Apple Mail and Horde IMP have the same bug.  I only figured out what was going on because Thunderbird actually reports an error, while both other clients just silently ignore the messages.

If a spammer sends a message with a forged X-UID header, and you're using mbox format which uses an X-UID header to track UIDs, imapd will be tricked into thinking that using a UID above 2^31 is a good idea, and all subsequent messages will be numbered sequentially above that.  I'm working on configuring my MTA to strip out X-UID headers, but obviously there are other situations in which one might encounter UIDs above 2^31.

Comment 10

[David :Bienvenu]

this bug is fixed in Thunderbird, at least on the trunk, but also in 2.0.0.9, I believe, thx to Lars. So I believe this bug is a dup of the bug that was fixed.

Comment 11

[Andy Lyttle]

I'm no longer seeing the error message I was getting before:
"Bogus sequence in UID FETCH: Syntax error in sequence."

But messages with a UID above 2^31 still aren't showing up for me, in 2.0.0.9.  Any idea why?

Comment 12

[Lars Fredriksen]

Hi Guys,
I just compiled and installed 2.0.0.9, and the "UID FETCH:" error that I originally ran into seems to be fixed as David reported in #10 above. Unfortunately, there still seems to be something left that has not yet been changed to Unsigned. If I click on an e-mail to mark it junk, I get 2 popups saying: "The current command did not succeed. The mail server responded: Bogus sequence in UID STORE: Syntax error in sequence." I still have a debug version of Imapd running where I added some debug statements, and it is definitively seeing negative UIDs in the request. I do not see any X-UID: headers in the message source. Applying my old patches this problem seemd to go away when I fixed the AppendInt() to be AppendUInt() in nsImapUtils.cpp. I did so everywhere we tried to append a PRUint32. nsBuildImapMessageUIR(), AllocateImapUidString(). I also changed the snprintf() in AppendUid() to be a %lu instead of a %u. Lastly I changed curToken and saveStartToken in ParseUidString to be of PRUint32 type instead of int32.

If anyone is interested I can provide the diffs I have applied to 2.0.0.9. It will have to wait until after Thanksgiving though :-)

Thanks for looking into this issue!

Lars

Comment 13

[David :Bienvenu]

the  patch in bug 277905 was landed on the trunk and 2.0.0.x branch - I'm not sure how that patch needs to be extended for the mark as junk case, but I can have a quick look - it should be a very small change...

Comment 14

[David :Bienvenu]

I looked at the code on the trunk and on the 2.0.0.x branch - it looks to me like the code that stores the /JUNK keyword uses AllocateImapUidString, which should work fine. I'm pretty sure 2.0.0.9 should have those fixes, so I'm not sure what's going on. Is the 2.0.0.9 source that you see still use PRInt32 in AllocateImapUidString()?

Comment 15

[Lars Fredriksen]

David,
I'll send you my current diffs, which some are not required based on the
changes you have already made, but that way you can look at them if you want.
Faster than me trying to type out what I changed.  I don't see a way to attach
the diff files here. AllocateImapUidString() does use PRUint32, but it still
uses AppendInt, where it really should be using AppendUInt(), or AppendUID().

Comment 16

[David :Bienvenu]

I don't think we can change the string classes, unfortunately.

AllocateImapUidString doesn't use AppendInt - it uses AppendUID, at least in the code in CVS that I'm looking at...but that's just the trunk code. I see what happened - the AllocateImapUidString code moved between the trunk and the branch, and the branch version doesn't use AppendUID - I'll fix that, but I don't know when it will make its way into a release...

It would be helpful if you could apply the patch I'm about to generate to a clean 2.0.0.9 tree, and see if you still have any problems with negative uid's (i.e., remove your existing patch).

Comment 17

[David :Bienvenu]

Attachment: fix AllocateImapUIDString

Comment 18

[Lars Fredriksen]

Hi David,
I'll try to get to it over the weekend.

Lars

Comment 19

[David :Bienvenu]

Lars, thx. If it works for you, I'll request approval to check this in for the next 2.0.0.x release.

Or, if you can give me a test account on your server with the negative UID's, I can try it myself :-)

Comment 20

[Andy Lyttle]

So I'm a complete C++ n00b, but I've been blindly poking around, and I noticed fUidOfSingleMessageFetch is defined in /mailnews/imap/src/nsImapServerResponseParser.h as PRInt32 instead of PRUint32.  It appears this doesn't actually get used for anything, so it should be irrelevant, but it would be good to fix, yes?

I've found other things defined as signed integers that look like they probably should be unsigned, but so far I'm at a loss as to which variable is causing the problem here.  Definitely still not working as of Thunderbird 2.0.0.14 though.  I've set up a test account; e-mail me if you want the login info.

Comment 21

[David :Bienvenu]

Attachment: cleanup unused var - checked in.

Comment 22

[David :Bienvenu]

imap uids > 0x80000000 are working in 3.0, afaik. I do remember fixing a bug sometime during the 3.0 process, but I don't remember the number.