173668 - With forward slash "/" as first character in URL field , pressing return key will result in a crash [@ CleanPath]

Status: VERIFIED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[Chris Petersen]

Build: 2002-10-09-04
Platform: OS 10.2.1
Expected Results: Application should display a message like URL is not valid
What I got: Application crashs

Steps to reproduce:

1) Type a valid url address in url field
2) Type a forward slash /  as the first character in url address
3) Press return key
4) Application crashes

Comment 1

[Chris Petersen]

Attachment: stack trace

Comment 2

[Mike Pinkerton (not reading bugmail)]

i got a better stack

#0  0x9024a048 in CleanPath(char const*, char*) ()
#1  0x9024b4c0 in canonpath(char const*, char*, unsigned long) ()
#2  0x90232558 in PathGetObjectInfo(char const*, unsigned long, VolumeInfo**,
unsigned long*, unsigned long*, char*, unsigned long*, unsigned char*) ()
#3  0x90234b48 in FSPathMakeRefWithOptions(unsigned char const*, unsigned long,
FSRef*, unsigned char*) ()
#4  0x9014ac44 in CFURLGetFSRef ()
#5  0x9014ab4c in CFURLGetFSRef ()
#6  0x018141ec in nsLocalFile::InitWithNativePath(nsACString const&)
(this=0x1693fd0, filePath=@0xbfffd730) at
nsLocalFileOSX.cpp:1279/Volumes/Data/Source/cocoazilla/mozilla/xpcom/io/
#7  0x01814014 in nsLocalFile::InitWithPath(nsAString const&) (this=0x1693fd0,
filePath=@0xbfffd930) at
nsLocalFileOSX.cpp:1249/Volumes/Data/Source/cocoazilla/mozilla/xpcom/io/
#8  0x01817a50 in NS_NewLocalFile (path=@0xbfffd930, followLinks=0,
result=0xbfffd840) at
nsLocalFileOSX.cpp:2175/Volumes/Data/Source/cocoazilla/mozilla/xpcom/io/
#9  0x0a8445a0 in nsDefaultURIFixup::ConvertFileToStringURI(nsString&,
nsCString&) (this=0x16272c0, aIn=@0xbfffd930, aOut=@0xbfffd9c0) at
nsDefaultURIFixup.cpp:386/Volumes/Data/Source/cocoazilla/mozilla/docshell/base/
#10 0x0a8443a0 in nsDefaultURIFixup::FileURIFixup(unsigned short const*,
nsIURI**) (this=0x16272c0, aStringURI=0xbfffda90, aURI=0xbfffde80) at
nsDefaultURIFixup.cpp:317/Volumes/Data/Source/cocoazilla/mozilla/docshell/base/
#11 0x0a843690 in nsDefaultURIFixup::CreateFixupURI(unsigned short const*,
unsigned, nsIURI**) (this=0x16272c0, aStringURI=0xbfffe180, aFixupFlags=1,
aURI=0xbfffde80) at
nsDefaultURIFixup.cpp:93/Volumes/Data/Source/cocoazilla/mozilla/docshell/base/
#12 0x0a83081c in nsDocShell::CreateFixupURI(unsigned short const*, nsIURI**)
(this=0xc63ea60, aStringURI=0xbfffe180, aURI=0xbfffde80) at
nsDocShell.cpp:4746/Volumes/Data/Source/cocoazilla/mozilla/docshell/base/
#13 0x0a825850 in nsDocShell::LoadURI(unsigned short const*, unsigned, nsIURI*,
nsIInputStream*, nsIInputStream*) (this=0xc63ea60, aURI=0xbfffe180,
aLoadFlags=0, aReferingURI=0x0, aPostStream=0x0, aHeaderStream=0x0) at
nsDocShell.cpp:2338/Volumes/Data/Source/cocoazilla/mozilla/docshell/base/
#14 0x0aea02b4 in nsWebBrowser::LoadURI(unsigned short const*, unsigned,
nsIURI*, nsIInputStream*, nsIInputStream*) (this=0x16a20f0, aURI=0xbfffe180,
aLoadFlags=0, aReferingURI=0x0, aPostDataStream=0x0, aExtraHeaderStream=0x0) at
nsWebBrowser.cpp:610/Volumes/Data/Source/cocoazilla/mozilla/embedding/browser/webBrowser/
#15 0x00016314 in -[CHBrowserView loadURI:referrer:flags:] (self=0x162a970,
_cmd=0x70b34, urlSpec=0xf21f00, referrer=0x0, flags=0) at
src/embedding/CHBrowserView.mm:230/Volumes/Data/Source/cocoazilla/mozilla/chimera/

as you can see, we think we're trying to load a local file and crash way down in
the OS. We should ensure that we only pass valid paths to the OS. I'd imagine
that either or both PPEmbed and mach-o mozilla have this problem.

Comment 3

[Mike Pinkerton (not reading bugmail)]

the talkback report at

http://heatwave.nscp.aoltw.net:8080/reports/singleincidentinfo.cfm?dynamicBBID=10127

has the correct stack as well. way to go simon and bryner!!!

Comment 4

[Conrad Carlen (not reading bugmail)]

I tried /www.mozilla.org and got a nice "The file //www.mozilla.org could not be
found..." dialog. However, /file:///X causes this crash. AFAICT, beginning with
'/' isn't a problem, it must begin with /file://.

Comment 5

[Mike Pinkerton (not reading bugmail)]

no, you have to give it a protocol

/http://www.mozilla.org

that will crash.

Comment 6

[Conrad Carlen (not reading bugmail)]

Attachment: patch

Fixes it. Patch could be a few lines shorter if I could use this:
http://lxr.mozilla.org/seamonkey/source/string/public/nsReadableUtils.h#224
but, for some reason, the source param isn't const.

Need r= for Chimera and trunk.

Comment 7

[jag (Peter Annema)]

"oops". I'll ask alecf why it's not const, but I can't see any reason for that.
I'll try and address that ASAP, but I'm wondering if scanning for "://" is
enough. Could it be "//" or ":/" by itself are enough to crash?

Comment 8

[Conrad Carlen (not reading bugmail)]

Attachment: better patch

Thanks Jag - turns out "//" was the minimum needed to crash. Needs r=.

Comment 9

[jag (Peter Annema)]

Hmmm, so on linux "/tmp//foopy" becomes file:///tmp//foopy and shows me
/tmp/foopy's contents. Should the same be true here instead of saying "invalid
path"? Also, cd /tmp//foopy on Mac OS X takes me to /tmp/foopy.

Comment 10

[Conrad Carlen (not reading bugmail)]

> Should the same be true here instead of saying "invalid path"?

If we don't say "invalid path," we crash and it's in OS code which can't be fixed.

Comment 11

[jag (Peter Annema)]

I meant, could we do something like replace all // with /?

Comment 12

[Conrad Carlen (not reading bugmail)]

But, is that right here?
If you say InitWithNativePath(), and then GetNativePath(), shouldn't that return
an identical string? Just because the terminal app fixes up input, does that
mean nsIFile should?

Comment 13

[jag (Peter Annema)]

On Unix the // stays as-is (see comment 9), even though it's treated as /. I'm
not sure if that's a bug or a feature :-) You could just fix it up before
handing it to the OS I guess, without storing the fixed up version yourself.

Comment 14

[jag (Peter Annema)]

Btw, I just fixed that const source thing.

Comment 15

[Conrad Carlen (not reading bugmail)]

That's good :-) I'd still like to use it in the way I originally wanted to,
though. I'm still of the opinion that nsIFile shouldn't fix paths given to
initWithNativePath - they should be valid native paths, or else it's an error.
CC'ing some other nsIFile API folks for their opinion.

Comment 16

[Darin Fisher]

InitWithNativePath already strips trailing / (and on unix does ~/ substitution),
so there is no guarantee that GetNativePath will return what you passed into
InitWithNativePath.

Comment 17

[Doug Turner (:dougt)]

what darin said.  not playing nicely with as many poorly formated paths leads to
path fixup everwhere except where it should be.

Comment 18

[Conrad Carlen (not reading bugmail)]

Attachment: more tolerant patch

This one converts runs of '/'s into 1. I found that on OSX, runs of '/' are OK
if they are between actual path nodes. For example, "/tmp//foopy" is OK but
"/tmp/fo//opy" crashes.

Comment 19

[jag (Peter Annema)]

Comment on attachment 105411 [details] [diff] [review]
more tolerant patch

You can merge the declaration and the assignment into one, like this:

nsCAutoString fixedPath(filePath);

sr=jag either way.

Comment 20

[Matt Carrell]

*** Bug 179967 has been marked as a duplicate of this bug. ***

Comment 21

[Steve Dagley]

Comment on attachment 105411 [details] [diff] [review]
more tolerant patch

Is it just me or are the CFURL routines really fragile?

Comment 22

[Conrad Carlen (not reading bugmail)]

Having no milestone, this fell off my radar. Setting milestone and will check in
today.

Comment 23

[Conrad Carlen (not reading bugmail)]

Fixed on trunk & Chimera branch.

Comment 24

[Chris Petersen]

Verified in the 2002-12-04-05 NB . The crash is no longer occuring and now
displays a dialog "The file 'url' cannot be found. Please check the location and
try again."

Comment 25

[Chris Petersen]

*** Bug 184129 has been marked as a duplicate of this bug. ***