248933 - IMAP parser: end of line is not handled correctly while parsing quoted/literal strings

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Hans-Andreas Engel]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040622
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040622

The logic of the imap parser is to set an 'end-of-line' flag if |fNextToken|
corresponds to the end of the line.  However, in the current implementation of
|CreateQuoted| and |CreateLiteral|, the 'end-of-line' flag is set according to
the token *after* |fNextToken| (i.e., this code checks if
|fCurrentTokenPlaceHolder| is at the end of the line).  This should be made
consistent.

The correct parser usage is to call |CreateQuoted|/|CreateLiteral|, then
|GetNextToken| and finally to check for the end-of-line condition.  This is
currently implemented this way in all the imap parser code, so no change is
required outside |CreateQuoted|/|CreateLiteral|.

Due to this incorrect end-of-line handling, a kludge was used in
nsImapServerResponseParser.cpp, where the |fAtEndOfLine| was reset by hand. 
This kludge can be removed while fixing this bug.

Finally, in |CreateLiteral| for some special cases, the behavior beyond the
end-of-line issue is also broken.

In addition, there is a bug in |CreateLiteral| when a 'static buffer' is used
and the current line does not contain the whole literal (a very unlikely case).
 For example, if the current line is "{9}\r\n\nxyz\r\n", |CreateLiteral| will 
return the string "\nxyz\r\nxyz"; i.e., partially duplicating the input.  This
bug can easily be fixed while fixing the bugs described above.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Hans-Andreas Engel]

Attachment: fixing CreateQuoted and CreateLiteral in IMAPGenericParser, removing kludge in ImapServerResponseParser

These issues outlined above should be fixed with this patch.

Note that we can remove two extra lines, because
|AdvanceTokenizerStartingPoint| recovers all 'token boundaries' and so the
condition |if (!*fCurrentTokenPlaceHolder)| is always false.

Comment 2

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

Confirming.  While I can't read code, someone should look at the patch...Hans,
feel free to set reviews.

Comment 3

[Henrik Gemal]

Hans: Get David Bienvenu to review it.

Comment 4

[Hans-Andreas Engel]

Comment on attachment 151877 [details] [diff] [review]
fixing CreateQuoted and CreateLiteral in IMAPGenericParser, removing kludge in ImapServerResponseParser

This breaks for  the very pathological case "{4}\r\nL2\r\nA3\r\n". Note, ther
is no space before A3, thus it probably not a valid imap input.  But let us fix
this anyway.  New patch forthcoming.

Comment 5

[Hans-Andreas Engel]

Attachment: fixing CreateQuoted and CreateLiteral in IMAPGenericParser, removing kludge in ImapServerResponseParser, v2

see comment #1 and comment #4

Added 2x |AdvanceTokenizerStartingPoint(0)| to the patch.  This handles more
(hopefully all) special cases correctly.

Comment 6

[David :Bienvenu]

I'm on vacation - I'll have to try this when I get back. It might take me a week
to get to this - thx for looking into it.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 151896 [details] [diff] [review]
fixing CreateQuoted and CreateLiteral in IMAPGenericParser, removing kludge in ImapServerResponseParser, v2

sr=dmose

Comment 8

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

Timeless checked in the 2nd patch, marking FIXED.