Bug 250000 (quarterMillion)

Remove string copying in |nsIMAPGenericParser::AdvanceTokenizerStartingPoint()|

VERIFIED FIXED

Status

MailNews Core
Networking: IMAP
VERIFIED FIXED
14 years ago
9 years ago

People

(Reporter: Hans-Andreas Engel, Assigned: Hans-Andreas Engel)

Tracking

({perf})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

14 years ago
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

14 years ago
quarter million bug count milestone :D
(Assignee)

Comment 2

14 years ago
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.
(Assignee)

Comment 3

14 years ago
Created attachment 152424 [details] [diff] [review]
Remove strdup from AdvanceTokenizerStartingPoint; copy only one char.

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

Updated

14 years ago
Attachment #152424 - Flags: superreview?(mscott)
Attachment #152424 - Flags: review?(bienvenu)
(Assignee)

Updated

14 years ago
Alias: quarterMillion
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Assignee: bienvenu → Hans-A.Engel
Status: ASSIGNED → NEW
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Attachment #152424 - Flags: review?(bienvenu) → review?(dmose)

Comment 4

14 years ago
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...
Attachment #152424 - Flags: review?(dmose) → review?(bienvenu)

Comment 5

14 years ago
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

14 years ago
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();
   }
(Assignee)

Comment 7

14 years ago
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).
(Assignee)

Comment 8

14 years ago
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.
Attachment #152424 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #152424 - Flags: superreview?(mscott)
Attachment #152424 - Flags: review?(bienvenu)
(Assignee)

Comment 9

14 years ago
Created attachment 159437 [details] [diff] [review]
Remove strdup from AdvanceTokenizerStartingPoint; copy only one char. Corrected assertions.
(Assignee)

Updated

14 years ago
Attachment #159437 - Flags: review?(bienvenu)

Comment 10

14 years ago
I'll try running with this patch.

Comment 11

14 years ago
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.
(Assignee)

Comment 12

14 years ago
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!
(Assignee)

Updated

13 years ago
Attachment #159437 - Flags: superreview?(dmose)
Product: MailNews → Core

Updated

13 years ago
Attachment #159437 - Flags: review?(bienvenu) → review+
Comment on attachment 159437 [details] [diff] [review]
Remove strdup from AdvanceTokenizerStartingPoint; copy only one char. Corrected assertions.

sr=dmose
Attachment #159437 - Flags: superreview?(dmose) → superreview+
Fix landed by timeless: Bug 250000 Remove string copying in
|nsIMAPGenericParser::AdvanceTokenizerStartingPoint()|
patch by Hans-A.Engel@unibas.ch r=bienvenu sr=dmose
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
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 Hans-A.Engel@unibas.ch r=bienvenu sr=dmose
Status: RESOLVED → VERIFIED
(Assignee)

Updated

12 years ago
Blocks: 313038
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.