250000 - (quarterMillion) Remove string copying in |nsIMAPGenericParser::AdvanceTokenizerStartingPoint()|

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Hans-Andreas Engel]

In |nsIMAPGenericParser::AdvanceTokenizerStartingPoint()|, the Field
|fLineOfTokens| is always re-initialized by making a fresh copy of
|fStartOfLineOfTokens|.  This is done to recover end-of-token boundaries which
were set to '\0' by |Imapstrtok_r|.

Actually, only the last removed boundary will be used while further parsing the
line.  This means that we just need to recover this boundary, e.g., by copying
the corresponding char from |fStartOfLineOfTokens| to |fLineOfTokens|.  Copying
the whole line becomes unnecessary.

Patch forthcoming.

Comment 1

[Oleg Sidletskiy]

quarter million bug count milestone :D

Comment 2

[Hans-Andreas Engel]

Let us have a look where |nsIMAPGenericParser::AdvanceTokenizerStartingPoint()|
is called.

i) The following invocations occur just after an |AdvanceToNextLine()|, where a
fresh line is obtained and |Imapstrtok_r| is called only once.  So there is at
most one boundary char overwritten.
http://lxr.mozilla.org/seamonkey/source/mailnews/imap/src/nsIMAPGenericParser.cpp#584
http://lxr.mozilla.org/seamonkey/source/mailnews/imap/src/nsImapServerResponseParser.cpp#2740

ii) The remaining calls are made with an argument
  bytesToAdvance == (fNextToken - fLineOfTokens) + c,
where c>=0.  Thus, new position of |fLineOfTokens| is
  fLineOfTokens + (fNextToken - fLineOfTokens) + c  ==  fNextToken + c
i.e., it is alway at or beyond fNextToken.  At most one boundary character was
deleted after fNextToken.
http://lxr.mozilla.org/seamonkey/source/mailnews/imap/src/nsIMAPGenericParser.cpp#494
http://lxr.mozilla.org/seamonkey/source/mailnews/imap/src/nsIMAPGenericParser.cpp#586

Therefore, at most one boundary character was deleted after the new
|fLineOfTokens| position, and it is sufficient to recover only a single 
character.

Comment 3

[Hans-Andreas Engel]

Attachment: Remove strdup from AdvanceTokenizerStartingPoint; copy only one char.

Copying only one char is sufficient, as explained in comment 2.

Comment 4

[David :Bienvenu]

Comment on attachment 152424 [details] [diff] [review]
Remove strdup from AdvanceTokenizerStartingPoint; copy only one char.

I really need to review the imap changes. Feel free to ping me again...I'll try
running with this patch...

Comment 5

[David :Bienvenu]

Comment on attachment 152424 [details] [diff] [review]
Remove strdup from AdvanceTokenizerStartingPoint; copy only one char.

need a ; at the end of this line to make this compile:

+
+  NS_ASSERTION(bytesToAdvance <= strlen(fLineOfTokens), "cannot advance beyond
end of fLineOfTokens")

Comment 6

[David :Bienvenu]

When I run with this patch (and this patch only, not your other patches), I hit
the following assertion:

1424[4542158]: 45384f8:tuschin.blackcatnetworks.co.uk:A:SendData: 4 lsub ""
"INBOX.*"

1424[4542158]: ReadNextLine [stream=4542908 nb=48 needmore=0]
1424[4542158]: 45384f8:tuschin.blackcatnetworks.co.uk:A:CreateNewLineFromSocket:
* LSUB (\HasNoChildren) "." "INBOX.new folder"

1424[4542158]: ReadNextLine [stream=4542908 nb=45 needmore=0]
1424[4542158]: 45384f8:tuschin.blackcatnetworks.co.uk:A:CreateNewLineFromSocket:
* LSUB (\HasNoChildren) "." "INBOX.a.b.c.d"

1424[4542158]: ReadNextLine [stream=4542908 nb=42 needmore=0]
1424[4542158]: 45384f8:tuschin.blackcatnetworks.co.uk:A:CreateNewLineFromSocket:
* LSUB (\HasNoChildren) "." "INBOX.a\\b"

1424[4542158]: ###!!! ASSERTION: cannot advance beyond end of fLineOfTokens:
'bytesToAdvance <= strlen(fLineOfTokens)', file
c:/thunderbird/mozilla/mailnews/imap/src/nsIMAPGenericParser.cpp, line 335
1424[4542158]: ###!!! Break: at file
c:/thunderbird/mozilla/mailnews/imap/src/nsIMAPGenericParser.cpp, line 335
1424[4542158]: ReadNextLine [stream=4542908 nb=46 needmore=0]
1424[4542158]: 45384f8:tuschin.blackcatnetworks.co.uk:A:CreateNewLineFromSocket:
* LSUB (\HasNoChildren) "." "INBOX.sub-of a"

1424[4542158]: ReadNextLine [stream=4542908 nb=42 needmore=0]
1424[4542158]: 45384f8:tuschin.blackcatnetworks.co.uk:A:CreateNewLineFromSocket:
* LSUB (\HasNoChildren) "." "INBOX.Junk"

this is against a Courier IMAP server. With another of your patches, I'd had
similar problems with this server, though I think that was a crash. I think it
was this patch, which may have been a work in progress; I can't really
remember...I don't know if the Courier server is generating something odd (I
don't think so, though - it looks pretty normal), or if your patch is exposing a
bug in the parser, or if there's just a problem with the patch. I haven't had
time to dig deeper into it.

C:\thunderbird\mozilla\mailnews\imap\src>more < parsediffs.txt
Index: nsImapServerResponseParser.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/imap/src/nsImapServerResponseParser.cpp,v
retrieving revision 1.112
diff -u -w -r1.112 nsImapServerResponseParser.cpp
--- nsImapServerResponseParser.cpp      9 Jun 2004 15:31:50 -0000       1.112
+++ nsImapServerResponseParser.cpp      5 Aug 2004 15:26:33 -0000
@@ -912,13 +912,6 @@
   else
   {
     boxname = CreateAstring();
-    // handle a literal ending the line here
-    if (fTokenizerAdvanced)
-    {
-      fTokenizerAdvanced = PR_FALSE;
-      if (!PL_strcmp(fCurrentTokenPlaceHolder, CRLF))
-        fAtEndOfLine = PR_FALSE;
-    }
     fNextToken = GetNextToken();
   }

Comment 7

[Hans-Andreas Engel]

It seems that DEBUG was not defined when I tested my patch, attachment 152424 [details] [diff] [review]. 
(I should have used an |NS_ASSERTION(0)| to ensure that I was testing with
assertions enabled!)

I will have a look into this issue and check why the assertion was violated.

Further, I think that the compiler should have complained about the missing ';'
even if DEBUG is not defined. (I have filed bug 260209 on this issue).

Comment 8

[Hans-Andreas Engel]

Comment on attachment 152424 [details] [diff] [review]
Remove strdup from AdvanceTokenizerStartingPoint; copy only one char.

The assertion code of attachment 152424 [details] [diff] [review] is broken. Namely,
|strlen(fLineOfTokens)| does not give information about the total length of
|fLineOfTokens|, since |fLineOfTokens| is chopped into tokens.

Comment 9

[Hans-Andreas Engel]

Attachment: Remove strdup from AdvanceTokenizerStartingPoint; copy only one char. Corrected assertions.

Comment 10

[David :Bienvenu]

I'll try running with this patch.

Comment 11

[David :Bienvenu]

this patch also doesn't compile - there's no ';' at the end of the assertion.
Are you sure you have assertions turned on? It needs to be a debug build for
assertions to get compiled.

Comment 12

[Hans-Andreas Engel]

I temporarily added |NS_ASSERTION(0, always hit this assertion);| to the code
and checked that this assertion is hit indeed, indicating that I am using a
debug build.
 
> (...) there's no ';' at the end of the assertion. (...)
Perhaps there was a mix-up in the applied patches?
 
In the revised patch (attachment 159437 [details] [diff] [review]), there is a seimcolon,
+  NS_ASSERTION(bytesToAdvance + (fLineOfTokens-fStartOfLineOfTokens) =
+    (int32)strlen(fCurrentLine), cannot advance beyond end of fLineOfTokens);
 
Thank you for running with this patch applied!

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 159437 [details] [diff] [review]
Remove strdup from AdvanceTokenizerStartingPoint; copy only one char. Corrected assertions.

sr=dmose

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Fix landed by timeless: Bug 250000 Remove string copying in
|nsIMAPGenericParser::AdvanceTokenizerStartingPoint()|
patch by [e-mail] r=bienvenu sr=dmose

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using LXR for code-inspection:

1.42	timeless%mozdev.org	2004-11-23 10:48	 	Bug 250000 Remove string copying in
|nsIMAPGenericParser::AdvanceTokenizerStartingPoint()|
patch by [e-mail] r=bienvenu sr=dmose