The default bug view has changed. See this FAQ.

Unsolicited capabilities in tagged IMAP responses not correctly parsed, last token not recognized

VERIFIED FIXED in Thunderbird 3

Status

MailNews Core
Networking: IMAP
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: rsx11m, Assigned: rsx11m)

Tracking

({fixed-seamonkey2.0.1, regression})

Trunk
Thunderbird 3
fixed-seamonkey2.0.1, regression
Bug Flags:
blocking-thunderbird3 -

Thunderbird Tracking Flags

(thunderbird3.0 .1-fixed)

Details

(Whiteboard: [tb3ride-along][fixed RC1 build 3])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

8 years ago
One of my IMAP servers reports quota in the Folder Properties with Thunderbird 2.0.0.23 but not with 3.0pre builds. Comparing the IMAP logs between those two versions shows the following differences during authentication:

3.0pre:
> CreateNewLineFromSocket: * OK [CAPABILITY ... AUTH=GSSAPI AUTH=PLAIN AUTH=LOGIN] greeting
> SendData: 1 authenticate plain
> CreateNewLineFromSocket: +
> SendData: Logging suppressed for this command (authentication information)
> CreateNewLineFromSocket: 1 OK [CAPABILITY ... QUOTA] success

2.0.0.23:
> CreateNewLineFromSocket: * OK [CAPABILITY ... AUTH=GSSAPI AUTH=PLAIN AUTH=LOGIN] greeting
> SendData: 1 capability
> CreateNewLineFromSocket: * CAPABILITY ... QUOTA ... AUTH=GSSAPI AUTH=PLAIN AUTH=LOGIN
> CreateNewLineFromSocket: 1 OK CAPABILITY completed
> SendData: 2 authenticate plain
> CreateNewLineFromSocket: + 
> SendData: Logging suppressed for this command (authentication information)
> CreateNewLineFromSocket: 2 OK [CAPABILITY ... QUOTA] success

The server provides CAPABILITY information with the initial greeting (but no QUOTA at that time), and then again in a tagged response to the authentication (with QUOTA). Due to bug 401293 not present in 2.0.0.23, Thunderbird requests an explicit CAPABILITY with the QUOTA response. This looks like bug 470650 and should be fixed. However, note that the QUOTA token comes right before the ']' end token, which is a difference to attachment 354142 [details].

Some debugging in nsImapServerResponseParser::capability_data() showed that
the last token of the untagged greeting is correctly read (AUTH=LOGIN), and then the parsing stops. In contrast, the tagged response delivers a token QUOTA] and continues parsing the notification of a successful authentication. Since "QUOTA" != "QUOTA]" that capability is not recorded, thus no quota information is available in the Folder Properties. It probably works in 2.0.0.23 due to the additional solicited CAPABILITY with a response containing a clear QUOTA token.
(Assignee)

Comment 1

8 years ago
Created attachment 411045 [details] [diff] [review]
Mitigating patch (too ugly)

This is a very direct fix, extending the already long if/else-if chain by the variants with a trailing ']'. It also stops parsing if a ']' is present in the token. While this works and gives me the quota information again, there is probably a better way to parse the string correctly (ideally not passing the ']' and any trailing notifications down to capability_data in the first place, as done for the untagged responses already, I couldn't figure out though where this is done). Anyway, this may do as a safe short-term solution.
Attachment #411045 - Flags: superreview?(bienvenu)
Attachment #411045 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Summary: Unsolicited capabilities in tagged responses not correctly parsed, last token not recognized → Unsolicited capabilities in tagged IMAP responses not correctly parsed, last token not recognized

Comment 2

8 years ago
oh my, how about making a copy of the token and stripping off any trailing ']' and changing the comparisons to use the copy?
(Assignee)

Comment 3

8 years ago
Ok, that patch is ugly, but it was the quickest way to solve the issue. ;-)
I'll have a look into the NSPR string handling and see how to do it.
(Assignee)

Updated

8 years ago
Attachment #411045 - Attachment description: Mitigating patch → Mitigating patch (too ugly)
Attachment #411045 - Attachment is obsolete: true
Attachment #411045 - Flags: superreview?(bienvenu)
Attachment #411045 - Flags: review?(bugzilla)
(Assignee)

Comment 4

8 years ago
Created attachment 411065 [details] [diff] [review]
Proposed fix (v2)

This should be better and creates a temporary copy of the token now. It also covers the "token]other" and "token ] other" cases. PL_strndup() takes care of proper termination of the string.
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #411065 - Flags: superreview?(bienvenu)
Attachment #411065 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #411065 - Attachment description: Porposed fix (v2) → Proposed fix (v2)

Updated

8 years ago
Flags: blocking-thunderbird3?

Comment 5

8 years ago
I wonder if we can just null terminate fNextToken, chopping off the ']', which would make the patch a ton smaller...I'll look at the code.

Comment 6

8 years ago
I don't think I'd block on this w/o knowing how common this form of server response is, though I'd take a ride-along.
(Assignee)

Comment 7

8 years ago
(In reply to comment #5)
> I wonder if we can just null terminate fNextToken, chopping off the ']'

It's defined as const char* in nsIMAPGenericParser.h and seems to point to a location within the line which is currently being parsed, thus I didn't dare to simply write into it.
(In reply to comment #6)
> I don't think I'd block on this w/o knowing how common this form of server
> response is, though I'd take a ride-along.

Agreed.
Flags: blocking-thunderbird3? → blocking-thunderbird3-
Whiteboard: [tb3ride-along]

Comment 9

8 years ago
Created attachment 413680 [details] [diff] [review]
use modern string api

Comment 10

8 years ago
Comment on attachment 413680 [details] [diff] [review]
use modern string api

Does this fix the issue for you? I'm thinking if we're going to change all this code, might as well use CStrings...
Attachment #413680 - Flags: superreview?(bugzilla)
Attachment #413680 - Flags: review?(rsx11m.pub)
(Assignee)

Comment 11

8 years ago
Created attachment 413685 [details] [diff] [review]
Patch to reset parser for ']'

I can do that, but have worked on a solution for comment #7 by now. After some analysis of what AdvanceToNextToken() is actually doing, the line to be parsed is a strdup()-ed version pointed to by fStartOfLineOfTokens; while parsing, fNextToken points to the begin of the token while NS_strtok() finds the next token. When a space is found, it is set to '\0' and fCurrentTokenPlaceHolder points to the next character for the next pass.

Thus, when a ']' character is found in fNextToken, this patch determined the number of characters to move back and resets fCurrentTokenPlaceHolder (which is ensured to point right after the ']' in the string). Then, the character just before that pointer can be set to '\0', resulting in the same situation as if the ']' had been a white-space character.

If you consider this safe, this would be the most compact solution.
Attachment #413685 - Flags: superreview?(bienvenu)
Attachment #413685 - Flags: review?(bugzilla)

Comment 12

8 years ago
Comment on attachment 413685 [details] [diff] [review]
Patch to reset parser for ']'

I find that a little scary, to be honest, and I wouldn't take it for 3.0
(Assignee)

Comment 13

8 years ago
Comment on attachment 413680 [details] [diff] [review]
use modern string api

While this works, it continues parsing beyond the ']' and wouldn't take care of the "token]other" case.

I'll have a modified version based on this patch in a minute with those issues resolved.
(Assignee)

Comment 14

8 years ago
Created attachment 413692 [details] [diff] [review]
Patch using nsCString, better ']' handling

Ok, that's hopefully it. This is using now token.Find() to look up the index
of the ']' character, if present, end stores it in endToken, thus used as an indicator to stop parsing after this token. Then, token is truncated as before
to the position just before the ']', if at least another character is present. Since Find() returns -1 if the string is not found, this is used as condition
to carry on condition with the loop.
Attachment #411065 - Attachment is obsolete: true
Attachment #413685 - Attachment is obsolete: true
Attachment #413692 - Flags: superreview?(bienvenu)
Attachment #413692 - Flags: review?(bugzilla)
Attachment #411065 - Flags: superreview?(bienvenu)
Attachment #411065 - Flags: review?(bugzilla)
Attachment #413685 - Flags: superreview?(bienvenu)
Attachment #413685 - Flags: review?(bugzilla)
(Assignee)

Comment 15

8 years ago
(In reply to comment #14)
> ... just before the ']', if at least another character is present.

Actually, I've just noticed that I changed (endToken > 0) to (endToken >= 0), thus a stand-alone "]" token would be reduced to an empty string. This should
be equivalent, as neither "" nor "]" are looked for in the if() statements.

Updated

8 years ago
Attachment #413692 - Flags: superreview?(bienvenu) → superreview+

Comment 16

8 years ago
Comment on attachment 413692 [details] [diff] [review]
Patch using nsCString, better ']' handling

looks good, thx.
(Assignee)

Updated

8 years ago
Attachment #413692 - Flags: approval-thunderbird3?
(Assignee)

Comment 17

8 years ago
Comment on attachment 413680 [details] [diff] [review]
use modern string api

David, I'm canceling the requests as you approved attachment 413692 [details] [diff] [review], to avoid confusion.
Attachment #413680 - Flags: superreview?(bugzilla)
Attachment #413680 - Flags: review?(rsx11m.pub)
Comment on attachment 413692 [details] [diff] [review]
Patch using nsCString, better ']' handling

>+      nsCString token(fNextToken);
>+      endToken = token.Find("]");

nit: should be FindChar(']')

r+a=Standard8. As this is going in on the relbranch, I'll deal with all the checkins in a few mins.
Attachment #413692 - Flags: review?(bugzilla)
Attachment #413692 - Flags: review+
Attachment #413692 - Flags: approval-thunderbird3?
Attachment #413692 - Flags: approval-thunderbird3+
Checked in:

comm-central: http://hg.mozilla.org/comm-central/rev/9024e8262993
comm-1.9.1: http://hg.mozilla.org/releases/comm-1.9.1/rev/93f09aa8e90e
comm-1.9.1 (default): http://hg.mozilla.org/releases/comm-1.9.1/rev/fa8b00bb03ea
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [tb3ride-along] → [tb3ride-along][fixed RC1 build 3]
Target Milestone: --- → Thunderbird 3
(Assignee)

Comment 20

8 years ago
(In reply to comment #18)
> nit: should be FindChar(']')

Works for me with that change, thanks for the quick review and check-in.

Comment 21

7 years ago
(In reply to comment #8)
> (In reply to comment #6)
> > I don't think I'd block on this w/o knowing how common this form of server
> > response is, though I'd take a ride-along.
> 
> Agreed.

Dovecot in Version >=1.2 does it. Timo Sirainen did write to another bug (which I cannot find) and stated that this is due to efficiency and RFC-compliant.
As more and more big installations are moving to dovecot this is really an issue.
When I find the old bug I'll post the bug-id.

Comment 22

7 years ago
(In reply to comment #21)
> When I find the old bug I'll post the bug-id.

Here is the comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=401293#c35
(Assignee)

Comment 23

7 years ago
The issue is hopefully fixed for that type of server as well, bug 401293 caused the regression fixed in bug 470650, which in turn was complemented here.
Whiteboard: [tb3ride-along][fixed RC1 build 3] → [tb3ride-along][fixed RC1 build 3][fixedtb301]
status-thunderbird3.0: --- → .1-fixed
Whiteboard: [tb3ride-along][fixed RC1 build 3][fixedtb301] → [tb3ride-along][fixed RC1 build 3]

Updated

7 years ago
Keywords: fixed-seamonkey2.0.1
(In reply to comment #23)
> The issue is hopefully fixed for that type of server as well, bug 401293 caused
> the regression fixed in bug 470650, which in turn was complemented here.

Could you update the status of the bug to Fixed ? and do your testing using 3.0.1pre so we could add the verified-tb3 keyword ?
(Assignee)

Comment 25

7 years ago
Err, the bug has already been set to FIXED during checkin, and works fine
for me in in the 3.0 release and current nightlies. Are you referring to
comment #21? In that case, only Christian can verify that it's fixed too.
Thanks rsx11m
Status: RESOLVED → VERIFIED
Keywords: verified-thunderbird3.0

Comment 27

7 years ago
(In reply to comment #25)
>Are you referring to comment #21? In that case, only Christian can verify that >it's fixed too.

I don't know about the insides of your CAPABILITY parser code or when it's accepting which capabilities. But now thunderbird-3 accepts the quota capability offered after login and correctly shows the quota. So concerning this bug -> verified-tb3
(Assignee)

Comment 28

7 years ago
Thanks for confirming.
You need to log in before you can comment on or make changes to this bug.