101651 - nsStdURL::SetSpec can trash memory

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Simon Fraser [no longer active]]

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).

Comment 1

[Simon Fraser [no longer active]]

Attachment: Cleaned up the code that removes bad chars from the url string

Comment 2

[Simon Fraser [no longer active]]

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

Comment 3

[Andreas Otte]

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?

Comment 4

[Simon Fraser [no longer active]]

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

Comment 5

[Andreas Otte]

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.

Comment 6

[Andreas Otte]

r=[e-mail]

Comment 7

[Simon Fraser [no longer active]]

darin, sr please?

Comment 8

[Darin Fisher]

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

Comment 9

[gordon]

Simon, would you like me to finish this up and get it in?

Comment 10

[Simon Fraser [no longer active]]

Yes please.

Comment 11

[gordon]

Attachment: update of simon's patch for recent file changes, and incorporating white space changes suggested by darin.

Comment 12

[Simon Fraser [no longer active]]

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

Comment 13

[Darin Fisher]

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)

Comment 14

[gordon]

revised patch checked in. Marking FIXED.