Last Comment Bug 259708 - Trying to save file from data: protocol wipes every file in target directory not marked read-only
: Trying to save file from data: protocol wipes every file in target directory ...
Status: RESOLVED FIXED
[sg:fix]
: dataloss, fixed-aviary1.0, testcase
Product: Toolkit
Classification: Components
Component: Download Manager (show other bugs)
: unspecified
: x86 All
: -- critical with 1 vote (vote)
: ---
Assigned To: Ben Goodger (use ben at mozilla dot org for email)
: Ali Ebrahim
Mentors:
Depends on: 259890 262478
Blocks: 248511 sbb+
  Show dependency treegraph
 
Reported: 2004-09-15 19:40 PDT by Alex Vincent [:WeirdAl]
Modified: 2008-07-31 01:47 PDT (History)
17 users (show)
bugzilla: blocking‑aviary1.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase HTML page (2.38 KB, text/html)
2004-09-15 19:46 PDT, Alex Vincent [:WeirdAl]
no flags Details
Ported patch (1.01 KB, patch)
2004-09-24 21:02 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details | Diff | Splinter Review
patch (4.61 KB, patch)
2004-09-29 12:48 PDT, Ben Goodger (use ben at mozilla dot org for email)
shaver: review+
Details | Diff | Splinter Review

Description Alex Vincent [:WeirdAl] 2004-09-15 19:40:10 PDT
Testcase coming up in a few minutes.

Mozilla/5.0 (Windows; U; Win98; rv:1.7.3) Gecko/20040913 Firefox/0.10

Basically, I had written an article for my blog (not yet posted) whereby a short
script would convert a textarea's contents into a data: URI for saving.

So with a content-type of application/octet-stream, Firefox would try to save
it.  Unfortunately, it would never go beyond the "Starting" phase of the "download".

When you hit cancel, bye-bye directory's contents, except for the first file.

Repeated testcase in Mozilla 1.8a3:  did NOT see the same results.  Example
worked perfectly.
Comment 1 Alex Vincent [:WeirdAl] 2004-09-15 19:46:13 PDT
Created attachment 159051 [details]
Testcase HTML page

Steps to reproduce:

(1) Tools, Options, Downloads.	Set the directory to a brand-new one.
(2) In said download directory, manually create three or four new files,
perhaps a directory or two.
(3) Open attachment, scroll to bottom and click "Save File" button.
(4) Mozilla Firefox asks you whether you want to open the file or save it...
choose "Save to disk".
(5) Download Manager appears with "Starting" message for Downloads directory
(this is probably NOT what should happen.  It should show an actual file being
downloaded.)
(6) Hit Cancel link in Download Manager.

Expected result:  Download cancels safely.
Actual result:	All but one file in your downloads directory are completely
gone, unrecoverable.

Reproducible:  Always
Comment 2 Alex Vincent [:WeirdAl] 2004-09-15 19:47:19 PDT
blocking1.0? because of traumatic dataloss.  Yes, it's a rare scenario, but it's
very evil.
Comment 3 Alex Vincent [:WeirdAl] 2004-09-15 22:01:44 PDT
Further testing shows the only reason any file in the target directory survived
-- for that matter, the target directory itself -- is because said file was
marked read-only.
Comment 4 Boris Zbarsky [:bz] 2004-09-16 11:51:09 PDT
Sounds like some of the (forked) front-end code is making silly assumptions
about the ability to get a non-empty filename off a URI.

Since it's forked, I happen to not really care.
Comment 5 Alex Vincent [:WeirdAl] 2004-09-21 20:00:10 PDT
I've tracked the problem back to
http://lxr.mozilla.org/aviarybranch/source/uriloader/exthandler/nsExternalHelperAppService.cpp
.  I think it unusual that we don't get a file picker to find out the preferred
filename for this file.
Comment 6 Christian :Biesinger (don't email me, ping me on IRC) 2004-09-22 07:09:46 PDT
whoa, what exactly in that file does this?
Comment 7 Boris Zbarsky [:bz] 2004-09-22 07:35:34 PDT
I'm betting that the temp file is being set to the directory (by the Firefox
download manager breakage) and then that PR_TRUE in the delete that you fixed on
trunk recently kicks in.
Comment 8 Alex Vincent [:WeirdAl] 2004-09-22 19:34:38 PDT
nsExternalAppHandler::InitializeDownload is suspicious, insomuch as it is the 
only code which refers to an aDownload object.

For that matter, I should clarify that it very well could be 
nsDownloadManager.cpp .  I thought nsExternalAppHandler was responsible because 
I didn't see it specify or request a filename right away.
Comment 9 Darin Fisher 2004-09-24 16:05:55 PDT
(In reply to comment #7)
> I'm betting that the temp file is being set to the directory (by the Firefox
> download manager breakage) and then that PR_TRUE in the delete that you fixed on
> trunk recently kicks in.

we should make sure that the patch for bug 259890 lands on the aviary and 1.7
branches.
Comment 10 Alex Vincent [:WeirdAl] 2004-09-24 20:40:48 PDT
:( Note bug 259890 comment 7.  The patch for Mozilla trunk will not apply to
Firefox 1.0 branch.
Comment 11 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2004-09-24 21:02:23 PDT
Created attachment 160030 [details] [diff] [review]
Ported patch

Untested, cargo-cult.
Comment 12 Boris Zbarsky [:bz] 2004-09-24 21:05:40 PDT
Comment on attachment 160030 [details] [diff] [review]
Ported patch

r+sr=bzbarsky
Comment 13 Boris Zbarsky [:bz] 2004-09-24 21:26:13 PDT
Comment on attachment 160030 [details] [diff] [review]
Ported patch

Actually, clearing reviews.  This patch is on the wrong bug.  It may well fix
this bug (in that the directory will no longer be wiped), but even with it in
you won't be able to save...
Comment 14 Alex Vincent [:WeirdAl] 2004-09-24 21:37:32 PDT
Comment on attachment 160030 [details] [diff] [review]
Ported patch

obsoleting, as this is being covered by bug 259890
Comment 15 Vladimir Vukicevic [:vlad] [:vladv] 2004-09-24 22:02:50 PDT
I just committed the patch for 259890, which might also fix this -- can someone
confirm that it does with tomorrow's nightly, and resolve this if so?
Comment 16 Alex Vincent [:WeirdAl] 2004-09-25 08:44:59 PDT
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.3) Gecko/20040925 Firefox/0.10
is still showing this bug!

ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/sweetlou-0.9/Firefox-win32.zip
dated 9/25/04  	2:35:00 PM
Comment 17 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2004-09-25 10:38:20 PDT
Oh, I wasn't doing the right steps to reproduce the bug.  I can reproduce it
with my PR1.0 build, but my aviary-1.0 nightly with my patch isn't failing to
download - it downloads successfully and it's so fast I can't hit cancel.
Comment 18 Alex Vincent [:WeirdAl] 2004-09-25 20:25:11 PDT
confirmed, this bug is still present in 09/25 builds.  :-(
Comment 19 Ben Goodger (use ben at mozilla dot org for email) 2004-09-29 12:20:33 PDT
Asa suggested this become a security-sensitive bug due to possible side-effects.
Comment 20 Asa Dotzler [:asa] 2004-09-29 12:27:41 PDT
Tracy, can you try to figure out when this was first introduced and which
products/releases it impacts?
Comment 21 Ben Goodger (use ben at mozilla dot org for email) 2004-09-29 12:48:14 PDT
Created attachment 160553 [details] [diff] [review]
patch

patch
Comment 22 Tracy Walker [:tracy] 2004-09-29 13:35:39 PDT
This bug affects releases at least back to Firefox 0.8
I can't reproduce this on Mozilla builds.  Tested 1.8a4 and 1.7

On Firefox 0.8 and 0.9  I didn't have to cancel the download.  It was too fast
to cancel anyhow.  But the folder I selected to download to was overwritten by
the downloaded testfile (it kept the Folder name).

Mozilla builds installed the file into the selected Folder without data loss.
Comment 23 Mike Shaver (:shaver -- probably not reading bugmail closely) 2004-09-29 14:57:02 PDT
Comment on attachment 160553 [details] [diff] [review]
patch

r=shaver.
Comment 24 Alex Vincent [:WeirdAl] 2004-09-29 15:12:17 PDT
For the record, though I may be too late, I was able to reproduce this bug also
on Linux with a debug-built Firefox.
Comment 25 Alex Vincent [:WeirdAl] 2004-09-29 15:47:55 PDT
The patch does fix this bug, but I get an exception running my testcase:

*** exception in validateLeafName: [Exception... "Component returned failure
code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.append]" 
nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)"  location: "JS frame ::
file:///home/ajvincent/mozilla/dist/bin/components/nsHelperAppDlg.js ::
anonymous :: line 211"  data: no]
Comment 26 Alex Vincent [:WeirdAl] 2004-09-29 21:21:33 PDT
fixed Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.3) Gecko/20040929 Firefox/0.10
as of a sweetlou build timestamped 2004092917

We pass the testcase, and I can file a separate (non-blocking-aviary) bug for
fixing the download manager (or our help dialog) to get a filepicker so we save
the file properly.

Question, though:  are there any other protocols which can force a pseudo-download?

I'm leaving this bug as ASSIGNED, in case there are such protocols or resolving
as FIXED would open this bug to the public.  Until I am assured of such, I think
I should not file the spinoff bug for getting the filepicker.
Comment 27 Tracy Walker [:tracy] 2004-09-30 10:46:08 PDT
Verified fixed on Windows branch build 2004-09-30-08-0.9


fyi: I also tried some various scenarios with sending, saving and opening the
file on the Thunderbird branch and had no data loss.
Comment 28 Alex Vincent [:WeirdAl] 2004-09-30 18:45:47 PDT
Given that this bug is fixed, as tested by myself and Tracy Walker, could we
please re-expose this bug to the public?  I'd like to be able to talk about it
again.
Comment 29 Asa Dotzler [:asa] 2004-09-30 19:28:36 PDT
Alex, we're hustling to get a Firefox update in place, both an xpi patch and a
full 0.10.1 build. If all goes well, we'll be shipping that tomorrow. I'd like
to keep this closed until then. Is that OK with you?
Comment 30 Alex Vincent [:WeirdAl] 2004-09-30 20:15:07 PDT
Fine.  :) I'm just writing out some analysis of the bug's process now for
posting to my blog.
Comment 31 Christian :Biesinger (don't email me, ping me on IRC) 2004-10-01 07:09:33 PDT
Comment on attachment 160553 [details] [diff] [review]
patch

ok, so this patch cathes an exception and modifies the filename if it finds
one. (I do think "unnamed" should be localizable, btw).

but wouldn't a better fix be _not_ to recursively delete something that is
supposed to be a single file? it seems to me like there must be a remove(true)
somewhere that would better be a remove(false).
Comment 32 Daniel Veditz [:dveditz] 2004-10-01 08:57:00 PDT
Isn't the recursive delete what you fixed in bug 259890? Or did that turn out to
be unrelated?
Comment 33 Christian :Biesinger (don't email me, ping me on IRC) 2004-10-01 09:39:06 PDT
dveditz: comment 18 said that the patch there did not fix this bug; so it seems
unrelated
Comment 34 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2004-10-01 09:42:53 PDT
Isn't the cause of the problem more likely to be the Remove(PR_TRUE) here?
http://lxr.mozilla.org/aviarybranch/source/toolkit/components/downloads/src/nsDownloadManager.cpp#566
That checks if the file exists, and if it does, recursively deletes it.
Comment 35 Daniel Veditz [:dveditz] 2004-10-01 20:31:24 PDT
We would like to leave this bug security-sensitive for a couple of days after
the patch release to give people a chance to protect themselve before leaking
exploit details.
Comment 36 Christian :Biesinger (don't email me, ping me on IRC) 2004-10-04 01:49:01 PDT
(In reply to comment #31)
> but wouldn't a better fix be _not_ to recursively delete something that is
> supposed to be a single file? it seems to me like there must be a remove(true)
> somewhere that would better be a remove(false).

ok, fixed by
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/toolkit/mozapps/downloads/content&command=DIFF_FRAMESET&file=downloads.js&rev1=1.28&rev2=1.29&root=/cvsroot
(checked in as part of bug 262478)
Comment 37 Mike Shaver (:shaver -- probably not reading bugmail closely) 2004-10-04 21:53:41 PDT
Opening bug at reporter's request.
Comment 38 Alex Vincent [:WeirdAl] 2004-10-04 22:09:37 PDT
I filed bug 262949 per comment 31 of this bug, to see if we can get a better
filename than "unnamed".

Note You need to log in before you can comment on or make changes to this bug.