Do not request IMAP capability command if server announce it in greetings

VERIFIED FIXED in mozilla1.9.1b2

Status

MailNews Core
Networking: IMAP
--
enhancement
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Nikolay Shopik, Assigned: Dale Wiggins)

Tracking

1.8 Branch
mozilla1.9.1b2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8
Build Identifier: version 2.0.0.6 (20070728)

Many IMAP servers can announce their capability in greetings. TB must not request capability command if server already announce it in greetings. At least there should be option force it on or off.

Reproducible: Always

Steps to Reproduce:
1. Start your favorite sniffer to cap packets
2. Run TB
3. Analyze packets in sniffer
4. Look for capability request even if server show already capability before.
(Reporter)

Updated

10 years ago
Version: unspecified → 2.0
Assignee: nobody → bienvenu
Component: General → Networking: IMAP
Product: Thunderbird → Core
QA Contact: general → networking.imap
Version: 2.0 → 1.8 Branch

Comment 1

10 years ago
does the imap rfc really say servers can return the capabilities in the greeting? That's news to me.
(Reporter)

Comment 2

10 years ago
Yes its part of RFC3501 (7.1.) which clearly says:
"Status responses MAY include an OPTIONAL "response code".  A response code consists of data inside square brackets in the form of an atom, possibly followed by a space and arguments."

Comment 3

10 years ago
Though not the same bug, this is closely related to one I've just filed
https://bugzilla.mozilla.org/show_bug.cgi?id=404323

Where Thunderbird isn't asking for changed capabilities after not getting
these responses in the login OK result.

Cheers,
     Duncan
(Reporter)

Comment 4

10 years ago
still reproducible on trunk 3.0a1pre (2008040204)
Windows XP sp2
(Reporter)

Comment 5

10 years ago
Making new since its really obvious 
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

10 years ago
is bug 404323 really related to the extent that anything done here in this bug might impact 404323 ?
(Reporter)

Comment 7

10 years ago
bug 404323 violates RFC2060 clause 6.1.1.

Comment 8

10 years ago
My understanding from section 7.1 is even CAPABILITY is appeared in the initial OK or PREAUTH response, there is no harm to ask for it explicitly later, although it is unnecessary. 

Nikolay thoughts?
(Reporter)

Comment 9

10 years ago
That's why I marked this as RFE and NOT a bug. This is usually save few packets roundtrip, still important when you are on slow connection or/and high latency.

Comment 10

10 years ago
Gotcha, makes sense. 

Comment 11

10 years ago
(In reply to comment #6)
> is bug 404323 really related to the extent that anything done here in this bug
> might impact 404323 ?
> 

Not at all, I thought at the time that they'd probably be easily addressed by
the same person at the same time.  Sorry.

Comment 12

10 years ago
(In reply to comment #7)
> bug 404323 violates RFC2060 clause 6.1.1.
> 

RFC2060 is obsoleted by 3501 which doesn't have this restriction.

Comment 13

10 years ago
(In reply to comment #11)
> (In reply to comment #6)
> > is bug 404323 really related to the extent that anything done here in this bug
> > might impact 404323 ?
> > 
> 
> Not at all, I thought at the time that they'd probably be easily addressed by
> the same person at the same time.  Sorry.
> 

Actually, on second thoughts, they're both related to whether or not youhould/shouldn't re request CAPABILITIES from a server and when, so
could be closely related (in the source) but I haven't looked at the code
and don't really know.

Comment 14

10 years ago
This bug contradicts bug 404323.
(Reporter)

Comment 15

10 years ago
Definitely its contradicts and nobody notice this :) I see here only one solution create new boolean value to control behavior of TB. Like mail.imap.check_capability_in_greeting and make it false by default, so this means by default it will act like described in bug 404323 because its more important. And if you are sure you don't have different CAPABILITIES you can turn this is by setting TRUE.

Comment 16

10 years ago
(In reply to comment #14)
> This bug contradicts bug 404323.
> 

I don't think it does, If the server does advertise capabilities in a response (login or greetings) then the client shouldn't rerequest
capabilities (401293). In 404323 I'm saying if the server doesn't advertise capabilities in the response, then the client should rerequest capabilities.
(Reporter)

Updated

10 years ago
OS: Windows XP → All
Hardware: PC → All
Summary: TB must not request IMAP capability command if server announce it in greetings → Do not request IMAP capability command if server announce it in greetings

Comment 17

10 years ago
Dale, this is an IMAP bug that we'd like to fix...
(Assignee)

Comment 18

10 years ago
Sure, I'll have a look.
Assignee: bienvenu → dwiggins
(Assignee)

Comment 19

9 years ago
Created attachment 332735 [details] [diff] [review]
Proposed fix for CAPABILITY issue

Successfully parses CAPABILITY in greeting, doesn't ask for CAPABILITY later in this case.
Attachment #332735 - Flags: superreview?(bienvenu)
Attachment #332735 - Flags: review?(mnyromyr)
(Assignee)

Comment 20

9 years ago
Created attachment 335917 [details] [diff] [review]
Unbitrotted version of my last patch

No substantive changes.
Attachment #332735 - Attachment is obsolete: true
Attachment #335917 - Flags: superreview?(bienvenu)
Attachment #335917 - Flags: review?(mnyromyr)
Attachment #332735 - Flags: superreview?(bienvenu)
Attachment #332735 - Flags: review?(mnyromyr)
(Assignee)

Comment 21

9 years ago
Created attachment 336852 [details] [diff] [review]
New version of my last patch done with Mercurial

No substantive changes.
Attachment #335917 - Attachment is obsolete: true
Attachment #336852 - Flags: superreview?(bienvenu)
Attachment #336852 - Flags: review?(mnyromyr)
Attachment #335917 - Flags: superreview?(bienvenu)
Attachment #335917 - Flags: review?(mnyromyr)

Comment 22

9 years ago
Comment on attachment 336852 [details] [diff] [review]
New version of my last patch done with Mercurial

Sorry for the delay.

>+++ b/mailnews/imap/src/nsImapProtocol.cpp	Wed Sep 03 12:48:35 2008 -0400
>@@ -1321,6 +1321,25 @@ void nsImapProtocol::EstablishServerConn
>   if (!PL_strncasecmp(serverResponse, "* OK", 4))
>   {
>     SetConnectionStatus(0);

Please use -U8 for 8 lines of context next time. :)

>+    nsCAutoString tmpstr(serverResponse);
>+    PRInt32   startIndex = tmpstr.Find("CAPABILITY", PR_TRUE/*aIgnoreCase*/);

1. Why do you copy the string before you even know you'll need it?
   Only create the temp string once you found the CAPABILITY response.
2. That's dangerous, since "CAPABILITY" might e.g. appear in a [ALERT] response.
   Actually, in the case of a CAPABILITY response, the serverResponse has to
   begin *exactly* with "* OK [CAPABILITY"!
3. And please leave a space before the comment, but only just a single space
   between type and variable.

>+    if (startIndex >= 0)
>+    {
>+      PRInt32   endIndex = tmpstr.FindChar(']', startIndex);

Only just a single space between type and variable, please.

>+      if (endIndex >= 0)
>+      {
>+        // Allocate the new buffer here -- it will be freed when the parser is destructed.
>+        char   *fakeServerResponse = (char*)PR_Malloc(PL_strlen(serverResponse));

And again.

>+        // Munge the greeting into something that would pass for an IMAP server's response
>+        // to a "CAPABILITY" command.

Please wrap comments after column 78 (here and elsewhere in your code),
if decently possible.

>+        strcpy(fakeServerResponse, "* ");
>+        strcat(fakeServerResponse, serverResponse + startIndex);
>+        fakeServerResponse[endIndex - startIndex + 2] = '\0';    // + 2 to account for the "* " string
>+        // Tell the response parser that we just issued a "CAPABILITY" and got the following back.
>+        GetServerStateParser().ParseIMAPServerResponse("1 capability", PR_TRUE, fakeServerResponse);

Any reason why to use a lowercase CAPABILITY here?

>+++ b/mailnews/imap/src/nsImapServerResponseParser.cpp	Wed Sep 03 12:51:10 2008 -0400
>@@ -170,7 +170,9 @@ void nsImapServerResponseParser::Initial
> //           response-data = "*" SP (resp-cond-state / resp-cond-bye /
> //                           mailbox-data / message-data / capability-data) CRLF
> //           continue-req    = "+" SP (resp-text / base64) CRLF
>-void nsImapServerResponseParser::ParseIMAPServerResponse(const char *currentCommand, PRBool aIgnoreBadAndNOResponses)
>+void nsImapServerResponseParser::ParseIMAPServerResponse(const char *currentCommand,
>+                                                         PRBool aIgnoreBadAndNOResponses,
>+                                                         char *capaInGreeting)

Since you're touching this, please make it fully adhere to our naming scheme,
which requires the "a" prefix for all method arguments.
And please use understandable names like "aGreetingWithCapability".

>+      if (capaInGreeting)
>+        fCurrentLine = capaInGreeting;

Who is going to free the former fCurrentLine?

>+++ b/mailnews/imap/src/nsImapServerResponseParser.h	Wed Sep 03 12:52:45 2008 -0400
>+  virtual void ParseIMAPServerResponse(const char *currentCommand,
>+                                       PRBool aIgnoreBadAndNOResponses,
>+                                       char *capaInGreeting = NULL);

See above.
Attachment #336852 - Flags: review?(mnyromyr) → review-
(Assignee)

Comment 23

9 years ago
Created attachment 340200 [details] [diff] [review]
Patch that addresses Karsten's feedback in comment #22.

This should hopefully address all of the concerns in the previous comment.
Attachment #336852 - Attachment is obsolete: true
Attachment #340200 - Flags: superreview?(bienvenu)
Attachment #340200 - Flags: review?(mnyromyr)
Attachment #336852 - Flags: superreview?(bienvenu)

Comment 24

9 years ago
Comment on attachment 340200 [details] [diff] [review]
Patch that addresses Karsten's feedback in comment #22.

> +++ mailnews/imap/src/nsImapProtocol.cpp	Wed Sep 24 16:09:39 2008
>    if (!PL_strncasecmp(serverResponse, "* OK", 4))

While this hard-coded length is probably acceptable in situations
where string and length are used only once, it's usually a bad idea,
and especially so in your code below.
Please make the entire EstablishServerConnection method define (and
undef later!) the strings and their lengths as preprocessor macros, eg.
  #define ESC_LENGTH(x) (sizeof(x) - 1)
  #define ESC_CAPABILITY_OK           "* OK ["
  #define ESC_CAPABILITY_OK_LEN       ESC_LENGTH(ESC_CAPABILITY_OK)
  #define ESC_CAPABILITY_GREETING     (ESC_CAPABILITY_OK "CAPABILITY")
  #define ESC_CAPABILITY_GREETING_LEN ESC_LENGTH(ESC_CAPABILITY_GREETING)
That way, changes to the actual string won't require extensive
searches and thoughts like "is this hard-coded 16 a length or where
does it come from?". The actual value of EST_CAPA_GREETING_LEN etc.
will be computed at compile time!

> +        strcpy(fakeServerResponse, "* ");
> +        strcat(fakeServerResponse, serverResponse + 6); // +6 to skip "* OK ["
> +        fakeServerResponse[endIndex - 4] = '\0';    // -6 for "* OK [" and + 2 for "* "

See above.

> +++ mailnews/imap/src/nsImapServerResponseParser.cpp	Wed Sep 24 15:03:57 2008
> +        if (fCurrentLine)
> +          PR_Free(fCurrentLine);

You can use just PR_FREEIF(fCurrentLine).

> @@ -1965,17 +1974,17 @@
> -    else if (!PL_strcasecmp(fNextToken, "NOMODSEQ]"))
> +    else if (!PL_strcasecmp(fNextToken, "NOMODSEQ"))

I see no justification for this change, and it wasn't in the first
patch either. Drop it, unless you have a very compelling explanation.
;-)


r=me with these changes.
Attachment #340200 - Attachment description: Patch that addresses Karstern's feedback in comment #22. → Patch that addresses Karsten's feedback in comment #22.
Attachment #340200 - Flags: review?(mnyromyr) → review+

Comment 25

9 years ago
> > +++ mailnews/imap/src/nsImapServerResponseParser.cpp	Wed Sep 24 15:03:57 2008
> > +        if (fCurrentLine)
> > +          PR_Free(fCurrentLine);
> 
> You can use just PR_FREEIF(fCurrentLine).

Or you can just use PR_Free, since free checks for null input :-)

> 
> > @@ -1965,17 +1974,17 @@
> > -    else if (!PL_strcasecmp(fNextToken, "NOMODSEQ]"))
> > +    else if (!PL_strcasecmp(fNextToken, "NOMODSEQ"))
> 
> I see no justification for this change, and it wasn't in the first
> patch either. Drop it, unless you have a very compelling explanation.
> ;-)
> 
Karsten, thanks very much for catching that - it should stay NOMODSEQ] - that was a recent fix I made.

Comment 26

9 years ago
> > > +        if (fCurrentLine)
> > > +          PR_Free(fCurrentLine);
> > 
> > You can use just PR_FREEIF(fCurrentLine).
> 
> Or you can just use PR_Free, since free checks for null input :-)

Sure, free() does, but what would be the use of having PR_Free/PR_FREEIF then?
I supposed that an abstraction layer like this would have these different functions for reason, else we could just write free() in the first place...


> 
> > 
> > > @@ -1965,17 +1974,17 @@
> > > -    else if (!PL_strcasecmp(fNextToken, "NOMODSEQ]"))
> > > +    else if (!PL_strcasecmp(fNextToken, "NOMODSEQ"))
> > 
> > I see no justification for this change, and it wasn't in the first
> > patch either. Drop it, unless you have a very compelling explanation.
> > ;-)
> > 
> Karsten, thanks very much for catching that - it should stay NOMODSEQ] - that
> was a recent fix I made.

Comment 27

9 years ago
PR_FREEIF nulls the pointer after freeing the memory. That's the real difference...
(Reporter)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 28

9 years ago
Created attachment 347762 [details] [diff] [review]
 Patch that addresses Karsten's feedback in comment #24.

The only changes in this patch are the changes that Karsten suggested.
Attachment #340200 - Attachment is obsolete: true
Attachment #347762 - Flags: superreview?(bienvenu)
Attachment #347762 - Flags: review?(mnyromyr)
Attachment #340200 - Flags: superreview?(bienvenu)

Updated

9 years ago
Attachment #347762 - Flags: review?(mnyromyr) → review+

Comment 29

9 years ago
Comment on attachment 347762 [details] [diff] [review]
 Patch that addresses Karsten's feedback in comment #24.

> void nsImapProtocol::EstablishServerConnection()
...
>   if (!PL_strncasecmp(serverResponse, "* OK", 4))
...
>   else if (!PL_strncasecmp(serverResponse, "* PREAUTH", 9))

I'd like these two occasions to follow the sizeof macro pattern, too, for consistency's sake - it's in the same function.

>+      if (aGreetingWithCapability)
>+      {
>+        PR_Free(fCurrentLine);

I'd still prefer PR_FREEIF, since PR_Free is just a macro, too, which might change unexpectdly, but I leave that to David's discretion.

r=me with that on checkin, no need for a new patch just for this.
(Assignee)

Comment 30

9 years ago
Created attachment 348071 [details] [diff] [review]
Patch that addresses Karsten's feedback in comment #29.

Minor changes for Karsten. Continuing r=mnyromyr@tprac.de.
Attachment #347762 - Attachment is obsolete: true
Attachment #348071 - Flags: superreview?(bienvenu)
Attachment #347762 - Flags: superreview?(bienvenu)

Comment 31

9 years ago
Comment on attachment 348071 [details] [diff] [review]
Patch that addresses Karsten's feedback in comment #29.

thx for doing this, Dale. 

Can you add a comment as to why we're using an allocated string here? I think this comment isn't quite right:

+        // Allocate the new buffer here -- it will be freed when the parser
+        // is destructed.

the parser will free the string, when done parsing, but the parser itself isn't destroyed.

I think you can use an nsDependentCString wrapper here, instead of tmpStr:

+      nsCAutoString tmpstr(serverResponse);
+      PRInt32 endIndex = tmpstr.FindChar(']', ESC_CAPABILITY_GREETING_LEN);


This would all be so much nicer if the imap parser used nsCStrings!
(Assignee)

Comment 32

9 years ago
Created attachment 348986 [details] [diff] [review]
New patch that fixes bogus comment

Continuing r=mnyromyr@tprac.de.
Attachment #348071 - Attachment is obsolete: true
Attachment #348071 - Flags: superreview?(bienvenu)

Comment 33

9 years ago
fix checked in, with a minor tweak to avoid an extra long line...thx, Dale.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Target Milestone: --- → mozilla1.9.1b2
(Reporter)

Comment 34

9 years ago
Thanks guys, works flawlessly.
Status: RESOLVED → VERIFIED
Product: Core → MailNews Core

Comment 35

9 years ago
I think this is going to break stuff. Newer IMAP servers (e.g. Dovecot v1.2) send a reduced list of capabilities in the greeting CAPABILITY and then send an updated full list of capabilities in tagged OK reply for LOGIN/AUTHENTICATE. Looking at your patch it seems you only handle the untagged OK reply, but not the tagged OK reply. You really should be using a generic resp-code parser that handles them no matter where and when they happen..

Comment 36

9 years ago
Timo - yes, you're talking about bug 470650, I believe.
Depends on: 470650
(Reporter)

Comment 37

9 years ago
David what decision is made you still parse both capabilities? I'm talking about comment #14 in bug 470650

Comment 38

9 years ago
we now parse the capability response whether or not we issued a capability command. I don't think that matches any of the choices in that comment.
You need to log in before you can comment on or make changes to this bug.