57443 - File picker crashes on long file names

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P3)

Description

[bobj]

With build 2000102008 (2000-10-20-09-MN6) on US W2K.
I tried to save a file from the above URL.
It popped up a blank browser window, and the "Downloading" dialog with
the radio buttons:
  [ ] Open using
  [x] Save to disk
When I hit OK, the dialog window goes away and nothing else.
I expected to see the "Save As" dialog to pop-up.

Comment 1

[Asa Dotzler [:asa]]

Confirmed with mozilla trunk build 102404 on NT4.  Over to mscott.  

Comment 2

[Scott MacGregor]

Looks like this is a bug in the file picker code. Sending over to Pavlov.

The file in question has a ridiculously long file name. We attempt to invoke the
file picker with this as the suggested file name. The file picker croaks when it
makes an OS call to  GetSaveFileName in nsFilePicker.cpp:

result = ::GetSaveFileName(&ofn);

The native dialog croaks and returns a cancel code instead of showing the dialog.

So we abort the download.

Comment 3

[Stuart Parmenter]

Bill?  Any idea here.  I'm not a windows expert what-so-ever.

Comment 4

[Bill Law]

It sounds like Windows doesn't like for the "save to" file name to be an invalid
Windows file name.  We should probably truncate it somehow when that happens.

I can't reproduce the problem at the moment because there doesn't seem to be any
files at that URL.  Can somebody suggest a way to reproduce it?

I'll take this bug but I really need a test case.  Would any i-drive site
produce similar problems?

Comment 5

[Bill Law]

This is a nasty bug.  The code at:
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsFilePicker.cpp#88
will overrun the fileBuffer if strlen(mDefault.get())>MAX_PATH.  Bad stuff 
happens.

Setting target milestone and adding crash keyword.  Also, I changed the summary.

BTW, I built a test case and will attach it.  I think idrive.com is history.

Comment 6

[Bill Law]

Attachment: Test case; Watch Out! Trying to save link on this test page will probably crash.

Comment 7

[Bill Law]

Attachment: Simple fix

Comment 8

[Bill Law]

Nominating due to potential security hole.  This bug involves a stack buffer 
overrun and thus permits web content to place arbitrary data (perhaps with 
restrictions) on the stack.

The fix is simple and safe.

Note that this fixes the buffer overrun but doesn't fully fix the problem.  The 
file picker passes too long a file name to the OS file picker, which then fails 
with an "invalid parameter" error.  We treat that the same as the user pressing 
Cancel on the file picker (so nothing happens).

Comment 9

[Scott MacGregor]

Comment on attachment 50964 [details] [diff] [review]
Simple fix

sr=mscott

Nice catch Bill.

Comment 10

[selmer (gone)]

PDT+ pending r=.

Comment 11

[Samir Gehani]

r=sgehani

Comment 12

[Bill Law]

Fix checked in on branch.  Waiting for trunk to open.

Comment 13

[Charles Manske]

Comment on attachment 50964 [details] [diff] [review]
Simple fix

r=cmanske

Comment 14

[Bill Law]

Fixed on trunk.

Comment 15

[benc]

->file handling

Comment 16

[sairuh (rarely reading bugmail)]

what i see when i do "save link as" in the attached testcase is a dialog saying
"there was an error writing to the target location", and the file is not saved.
is this expected behavior? [no crash, though!]

Comment 17

[Bill Law]

Yes, we ultimate get some other error but the fix only prevents the crash.

Perhaps we need another bug to deal with figuring out how to handle this situation?

Comment 18

[sairuh (rarely reading bugmail)]

okay, good to know! however, now when i test this on different platforms i see
different results [all commercial builds]:

* linux - 2001.10.08.04-0.9.2-branch and 2001.10.10.08-trunk: actually this is
the platform i was using wrt my 2001-10-11 12:42 comments.

* winNT - 2001.10.08.05-0.9.4-branch and 2001.10.09.09-trunk: after selecting
"save link as" from the context menu, no dialog appears. the only thing that
happens is that the link turns the active color and gets the dotted focus ring...

* mac os 10.1 - 2001.10.11.04-0.9.4-branch and 2001.10.10.20-trunk: i am somehow
able to save the link [downloads as a text file].

in any case, no crashes.

if this is still expected [in spite of platform diffs], i'll go ahead and vrfy this.

Comment 19

[Bill Law]

I had only tried Windows (since this was a Win32 file picker problem and the
code is Win32 only).  What you report is what I saw (and reported above: silent
failure).  There *should* be a bug for that, too.

The behavior on other platforms is an issue with their file pickers.  Sounds
like Linux may need a bug also (or maybe the message we get is OK?).

Comment 20

[Bill Law]

I forgot to mention:  Great testing!  Once again you went above and beyond the
call of duty (trying things on all platforms for a Win32 only bug; I didn't even
think to see if the other platforms had their own crashers).

Comment 21

[lchiang]

Sarah - let's file a new bug as Bill suggests for the items you saw and then
mark this one verified.  Thanks for the great work.

Comment 22

[sairuh (rarely reading bugmail)]

okay, marking vrfy'd.

spun off bug 105151 for the win32 silent failure.

spun off bug 105155 for the error encountered on linux.

[no problems for mac there. :)]