File picker crashes on long file names

VERIFIED FIXED in mozilla0.9.5

Status

Core Graveyard
File Handling
P3
major
VERIFIED FIXED
17 years ago
2 years ago

People

(Reporter: bobj, Assigned: Bill Law)

Tracking

({crash})

Trunk
mozilla0.9.5
x86
Windows 2000
crash

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PDT+, URL)

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
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

17 years ago
Confirmed with mozilla trunk build 102404 on NT4.  Over to mscott.  
Assignee: asa → mscott
Component: Browser-General → Networking

Comment 2

17 years ago
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.

Assignee: mscott → pavlov

Comment 3

17 years ago
Bill?  Any idea here.  I'm not a windows expert what-so-ever.
(Assignee)

Comment 4

17 years ago
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?
Assignee: pavlov → law

Updated

17 years ago
Keywords: qawanted
(Assignee)

Comment 5

17 years ago
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.
Severity: normal → major
Keywords: crash
Summary: cannot save file → File picker crashes on long file names
Target Milestone: --- → mozilla0.9.5
(Assignee)

Comment 6

17 years ago
Created attachment 50799 [details]
Test case; Watch Out!  Trying to save link on this test page will probably crash.
(Assignee)

Updated

17 years ago
Attachment #50799 - Attachment description: Test case; Watch Out! Might crash! → Test case; Watch Out! Trying to save link on this test page will probably crash.
(Assignee)

Comment 7

16 years ago
Created attachment 50964 [details] [diff] [review]
Simple fix
(Assignee)

Comment 8

16 years ago
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).
Keywords: nsbranch

Comment 9

16 years ago
Comment on attachment 50964 [details] [diff] [review]
Simple fix

sr=mscott

Nice catch Bill.
Attachment #50964 - Flags: superreview+

Comment 10

16 years ago
PDT+ pending r=.
Whiteboard: PDT+

Comment 11

16 years ago
r=sgehani
(Assignee)

Comment 12

16 years ago
Fix checked in on branch.  Waiting for trunk to open.

Comment 13

16 years ago
Comment on attachment 50964 [details] [diff] [review]
Simple fix

r=cmanske
Attachment #50964 - Flags: review+
(Assignee)

Comment 14

16 years ago
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 15

16 years ago
->file handling
Component: Networking → File Handling
QA Contact: doronr → sairuh
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!]
Keywords: qawanted
(Assignee)

Comment 17

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

Comment 19

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

Comment 20

16 years ago
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

16 years ago
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.
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. :)]
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.