Open Bug 1654819 Opened 4 years ago Updated 1 year ago

DownloadIntegration.downloadDone creates an empty file when a file was deleted after a download (ADS :Zone.Identifier)

Categories

(Toolkit :: Downloads API, defect, P3)

defect

Tracking

()

People

(Reporter: robwu, Unassigned)

References

(Blocks 1 open bug)

Details

See https://bugzilla.mozilla.org/show_bug.cgi?id=1316394#c126 for context.

When a file has been downloaded, the file could be gone. The logic at https://searchfox.org/mozilla-central/rev/227f22acef5c4865503bde9f835452bf38332c8e/toolkit/components/downloads/DownloadIntegration.jsm#579-616 assumes that the file is still there, but this is not guaranteed. As a result, upon attempting to write to the Alternative Data Stream, an empty file may be created.

The logic also smells to me - why would it only write the Zone Identifier after the completion of the download? I would expect it to be attached to the file before writing any more data to the file, in case the download got interrupted.

downloadDone is apparently invoked when a download succeeds, thus I think it expects the file to be there.
When you say it could be gone, what is removing it, is this a race condition due to asynchronicity? It sounds like one from the original comment.

For the Zone Identifier, I agree it could probably be written earlier, likely it doesn't matter much if the file has a .part extension, and it should be moved if the file is then renamed at the end of the download. So it was maybe made for simplicity?

Severity: -- → S3
Priority: -- → P3

(In reply to Marco Bonardo [:mak] from comment #1)

downloadDone is apparently invoked when a download succeeds, thus I think it expects the file to be there.

This expectation is natural, but the method is asynchrous and there is no guarantee that the file still exists at that point.

When you say it could be gone, what is removing it, is this a race condition due to asynchronicity? It sounds like one from the original comment.

In the test, this is a race condition (between the downloads implementation writing the ADS and the test removing the file).
Although the test can try to work around this by using internal APIs (such as download.whenSucceeded()), I believe that this should be fixed in the downloads core (because this bug also occurs in non-test scenarios, such as extensions).

We'd still check that the file exist in an async way though, so while that would reduce the likelihood of this happening, it sounds like it wouldn't completely solve the problem, unless we create the alternative stream sooner (as you suggested). I assume NTFS moves the alternate stream along with the file, if it's moved or renamed, so it wouldn't be so bad.

This comment has become rather large. It starts with a summary of the background of this mark-of-the-web (MOTW) that is written via the ADS.
Then I delve into an analysis of the reliability of the MOTW/ADS in Firefox in relation to its purpose.
In the end I propose an implementation strategy to resolve this bug.

While looking into this, I came across Chrome's implementation, where they explicitly write the ADS after the download/rename, because otherwise AV scanners would scan an incomplete file, at https://bugs.chromium.org/p/chromium/issues/detail?id=127999
In Firefox, that concern is likely not relevant because Firefox does not invoke the IAttachmentExecute interface (details in bug 959187 and its external references, including https://mail.mozilla.org/pipermail/firefox-dev/2013-August/000848.html ), but writes the ADS directly. Still, writing it too early may potentially cause regressions.

In the current implementation, there may be a situation where the ADS (with the Mark-of-the-Web) is not set when the file is renamed before the download has been completed. There is quite some code between renaming the file and adding the MOTW:

The implementation/timing of the MoTW/ADS writing is not perfect. Ideally it should be as close as possible as the rename to the final file name (and on the File thread), but I did not spend enough time on looking at the downloads implementation to see if it is safe to do so (e.g. missing one case can result in an absent MoTW; better late than never!). But I digress, this is a pre-existing issue, and certainly not going to be made worse by what I'm going to propose below.

The winAllowLengthBeyondMaxPathWithCaveats option is only used by this MOTW logic, we could update the implementation to confirm the existence of the file before https://searchfox.org/mozilla-central/rev/851bbbd9d9a38c2785a24c13b6412751be8d3253/toolkit/components/osfile/modules/osfile_win_front.jsm#379-388
The window between the file existence check and writing the ADS would be so small, that it is unlikely for the file to be removed in between, and thus bug 1316394 would be fixed by that.

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