227679 - <nsMsgUtils.cpp>: Fix improper handling of |end| in escaping "From " line functions

Status: ASSIGNED

(MailNews Core :: Import, defect)

Description

[Serge Gautherie (:sgautherie)]

Spun off from bug 119441 attachement 66959.

May be I haven't worked with pointers for too long,
or that code is incorrect.

Anyway, I'll post a patch, and welcome comments.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <nsMsgUtils.cpp>

The initial assumption is that |end| points beyond the last buffer character,
then |*end| must not be accessed.

Actually, I kind of "rewrote" the 2 involved functions.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b) <nsMsgUtils.cpp>

'r=?': (see comment 0 and comment 1)
Can you (super-)review, compile, test, check it in ?

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 136971 [details] [diff] [review]
(Av1b) <nsMsgUtils.cpp>


(see comment 2)

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1c) <nsMsgUtils.cpp>

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 136974 [details] [diff] [review]
(Av1c) <nsMsgUtils.cpp>


'r=?': (see comment 0 and comment 1)
Can you (super-)review, compile, test, check it in ?

Comment 6

[David :Bienvenu]

+  // Optimization: direct 'F' test, followed by |strncmp()| call.
+  if ((end - start >= 5) && (*start == 'F') && !strncmp(start + 1, "rom ", 4))
     rv = PR_TRUE;
+
   return rv;

can't this just be:

 return ((end - start >= 5) && (*start == 'F') && !strncmp(start + 1, "rom ", 4));

and get rid of rv completely?

if nsIFileSpec &pDst is a ref ptr, then this shouldn't compile, right?

+      rv = pDst->Write(">", 1, &written);

pDst.Write(">", 1, &written);

Is this the code that could access one byte past the end? :
-    while ((pChar < end) && (*pChar != nsCRT::CR) && (*(pChar+1) != nsCRT::LF))

seems like there could be a safer fix if that's the only actual problem...

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av1d) <nsMsgUtils.cpp>

Patch Av1c, plus comment 6 suggestions.


The 2 memory access issues were:

|-  if ( (*start == 'F') && (end-start > 4) && !strncmp(start, "From ", 5) )|
We may have |start == end|, then |end-start| check must come first.

|-    while ((pChar < end) && (*pChar != nsCRT::CR) && (*(pChar+1) !=
nsCRT::LF))|
Yes, we may have |(pChar+1) == end|.
Also, the |0CR && 1LF| seems odd... while |0CR || 1LF| is more explicit.


All the rest is rewriting/"optimization"...
What safer fix are you thinking about ?

Comment 8

[David :Bienvenu]

by safer, I meant "less lines of code changed", not that your changes are
particularly dangerous.  I haven't had a chance to compile or test your changes
but I'll try to do it soon.

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av1e) <nsMsgUtils.cpp>

(==) Av1d, with trunk update.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 137955 [details] [diff] [review]
(Av1e) <nsMsgUtils.cpp>


I have no compiler: Could you compile/test/review it ? Thanks.

Comment 11

[David :Bienvenu]

Comment on attachment 137955 [details] [diff] [review]
(Av1e) <nsMsgUtils.cpp>

This patch is obsolete since the file spec removal happened.  If someone wants to resurrect, and compile and test their changes, then we could move forward. From what I can tell, this code is only used by import.