nsStdURL::SetSpec can trash memory

RESOLVED FIXED in mozilla0.9.6

Status

()

RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: sfraser_bugs, Assigned: gordon)

Tracking

Trunk
mozilla0.9.6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
nsStdURL::SetSpec has some bad logic for stripping CR and LF from url strings. 
This loop:

    while (midPtr && (*midPtr != '\0')) {
        while ((*midPtr == '\r') || (*midPtr == '\n') || (*midPtr == '\t')) { // 
if linebreak or tab
            if (!copyToPtr)
                copyToPtr = midPtr; // start copying
            midPtr++; // skip linebreak and tab
        }
        if (copyToPtr) { // if copying
            *copyToPtr = *midPtr;
            copyToPtr++;
        }
        midPtr++;
    }

misbehaves when a url string has one or more trailing CRs, like:
http://foo.bar.com\r\r, since it fails to detect when midPtr has reached the end 
of the string (it increments to the character past the \0 before testing).
(Reporter)

Comment 1

17 years ago
Created attachment 50810 [details] [diff] [review]
Cleaned up the code that removes bad chars from the url string
(Reporter)

Comment 2

17 years ago
The patch is a much cleaner version of this code, that (I think) does exactly the 
same thing.

Comment 3

17 years ago
The patch looks good to me on first sight, but I can't apply it on linux, seems
to be made on the mac. Is there a way to convert the patch or attach a new
version with char(10) instead of char(13) as eol indicators?
(Reporter)

Comment 4

17 years ago
You'll have to convert the linebreaks on the patch using 'tr', or some small perl 
script.

Comment 5

17 years ago
Comment on attachment 50810 [details] [diff] [review]
Cleaned up the code that removes bad chars from the url string

tr is working fine, and so is the patch.
Attachment #50810 - Flags: review+
(Reporter)

Comment 7

17 years ago
darin, sr please?

Comment 8

17 years ago
Comment on attachment 50810 [details] [diff] [review]
Cleaned up the code that removes bad chars from the url string

looks like nsStdURL.cpp uses 4 spaces of indentation... fix that, and sr=darin
Attachment #50810 - Flags: needs-work+
(Assignee)

Comment 9

17 years ago
Simon, would you like me to finish this up and get it in?
Assignee: neeti → gordon
Target Milestone: --- → mozilla0.9.6
(Reporter)

Comment 10

17 years ago
Yes please.
(Assignee)

Comment 11

17 years ago
Created attachment 54115 [details] [diff] [review]
update of simon's patch for recent file changes, and incorporating white space changes suggested by darin.
(Reporter)

Comment 12

17 years ago
Comment on attachment 54115 [details] [diff] [review]
update of simon's patch for recent file changes, and incorporating white space changes suggested by darin.

r=sfraser
Attachment #54115 - Flags: review+

Comment 13

17 years ago
Comment on attachment 54115 [details] [diff] [review]
update of simon's patch for recent file changes, and incorporating white space changes suggested by darin.

sr=darin (nice job cleaning this up)
Attachment #54115 - Flags: superreview+
(Assignee)

Comment 14

17 years ago
revised patch checked in. Marking FIXED.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.