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

ASSIGNED
Assigned to

Status

MailNews Core
Import
ASSIGNED
14 years ago
9 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 obsolete attachments)

(Assignee)

Description

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

Comment 1

14 years ago
Created attachment 136956 [details] [diff] [review]
(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.
(Assignee)

Updated

14 years ago
Attachment #136956 - Flags: superreview?(sspitzer)
Attachment #136956 - Flags: review?(ducarroz)
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

14 years ago
Created attachment 136971 [details] [diff] [review]
(Av1b) <nsMsgUtils.cpp>

'r=?': (see comment 0 and comment 1)
Can you (super-)review, compile, test, check it in ?
Attachment #136956 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #136956 - Flags: superreview?(sspitzer)
Attachment #136956 - Flags: review?(ducarroz)
(Assignee)

Comment 3

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


(see comment 2)
Attachment #136971 - Flags: superreview?(sspitzer)
Attachment #136971 - Flags: review?(ducarroz)
(Assignee)

Comment 4

14 years ago
Created attachment 136974 [details] [diff] [review]
(Av1c) <nsMsgUtils.cpp>
(Assignee)

Updated

14 years ago
Attachment #136971 - Attachment is obsolete: true
Attachment #136971 - Flags: superreview?(sspitzer)
Attachment #136971 - Flags: review?(ducarroz)
(Assignee)

Comment 5

14 years ago
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 ?
Attachment #136974 - Flags: superreview?(sspitzer)
Attachment #136974 - Flags: review?(ducarroz)

Updated

14 years ago
Attachment #136974 - Flags: review?(ducarroz) → review?(bienvenu)

Comment 6

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

Comment 7

14 years ago
Created attachment 137055 [details] [diff] [review]
(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 ?
(Assignee)

Updated

14 years ago
Attachment #137055 - Flags: superreview?(sspitzer)
Attachment #137055 - Flags: review?(bienvenu)
(Assignee)

Updated

14 years ago
Attachment #136974 - Attachment is obsolete: true
Attachment #136974 - Flags: superreview?(sspitzer)
Attachment #136974 - Flags: review?(bienvenu)
(Assignee)

Updated

14 years ago
Attachment #136956 - Attachment description: (Av1) <nsMsgUtils.cpp> patch → (Av1) <nsMsgUtils.cpp>
(Assignee)

Updated

14 years ago
Attachment #136974 - Attachment description: (Av1c) <nsMsgUtils.cpp> patch → (Av1c) <nsMsgUtils.cpp>
(Assignee)

Updated

14 years ago
Attachment #136971 - Attachment description: (Av1b) <nsMsgUtils.cpp> patch → (Av1b) <nsMsgUtils.cpp>

Comment 8

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

Comment 9

14 years ago
Created attachment 137955 [details] [diff] [review]
(Av1e) <nsMsgUtils.cpp>

(==) Av1d, with trunk update.
(Assignee)

Updated

14 years ago
Attachment #137055 - Attachment is obsolete: true
Attachment #137055 - Flags: superreview?(sspitzer)
Attachment #137055 - Flags: review?(bienvenu)
(Assignee)

Comment 10

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


I have no compiler: Could you compile/test/review it ? Thanks.
Attachment #137955 - Flags: review?(bienvenu)
(Assignee)

Updated

14 years ago
Target Milestone: --- → mozilla1.7alpha
(Assignee)

Updated

14 years ago
Target Milestone: mozilla1.7alpha → ---
Product: MailNews → Core

Comment 11

10 years ago
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.
Attachment #137955 - Attachment is obsolete: true
Attachment #137955 - Flags: review?(bienvenu)

Updated

10 years ago
QA Contact: nbaca → import
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.