199473 - Download of file to a disk may fail if temporary directory used by the download manager is encrypted.

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[ddtizilla]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021212
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021212


If the temporary directory ($TMP & $TEMP) used by the download manager uses
encryption (EFS) then the download fails if the destination directory is not an
NTFS folder.  This eror results: "xxxfile could not be saved, because an unknown
error occurred. Sorry about that.  Try saving to a different location."

The problem is due to the fact that when you move a file from EFS, Windows
requires that the destination folder resides on a file system that supports EFS
(i.e. NTFS).


Reproducible: Always

Steps to Reproduce:
1. Encrypt $TMP folder.  It needs to be on NTFS file system.
2. Download any file to FAT16 or FAT32 file system.  Download fails because
FAT16 and FAT32 don't support EFS
3. Download to NTFS; download succeeds because encryption attributes from
temporary file is retained in the destination NTFS.
4. Do not encrypt directory as in 1. above, then problem does not exist.


Actual Results:  
step 2. results in an error: "xxxfile could not be saved, because an unknown
error occurred. Sorry about that.  Try saving to a different location."

Expected Results:  
Store temporary file in the same directory as destination and rename/move when
download is completed.

Comment 1

[Torben]

Related to (and would be fixed by) bug 69938. Not marking as dupe since there
might be other ways to fix this too.

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is actually a networking bug, I would say.  nsIFile should handle this
transparently to consumers....

Comment 3

[Cees T.]

Only downloading to the destination would lose the performance benefit of downloading while the user is choosing the destination.

If Windows' move function doesn't want to work, we could simply do it ourselves:
1. Create file on FAT32.
2. Read and write blocks of data from temp file to destination.
3. Delete temp file.

Comment 4

[Steve Harper]

Hello,

I have submitted a patch to fix this issue. CopyFileEx can use the flag COPY_FILE_ALLOW_DECRYPTED_DESTINATION. This means that If windows cannot encrypt the file on the destination volume it will store it on the disk unencrypted(See http://msdn.microsoft.com/en-us/library/aa363852(VS.85).aspx for more detail)

I do not have a FAT32 drive to test this patch on.

Thanks,
Steve

Comment 5

[Steve Harper]

Attachment: A patch that uses CopyFileW with the COPY_FILE_ALLOW_DECRYPTED_DESTINATION flag to perform the move.

Comment 6

[Steve Harper]

Attachment: A patch that uses CopyFileW with the COPY_FILE_ALLOW_DECRYPTED_DESTINATION flag to perform the move.

Sorry this is the correct patch, the previous patch contained some changes from a previous defect i was working on.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 442951 [details] [diff] [review]
A patch that uses CopyFileW with the COPY_FILE_ALLOW_DECRYPTED_DESTINATION flag to perform the move.

Hey Steve, thanks for the patch.  In Mozilla all patches have to be reviewed by a module owner or peer so I've asked Benjamin Smedberg to review this as he is the owner for the xpcom code.

I wonder if the patch would be simpler if instead of moving the file differently you just called DecryptFile on it?

Comment 8

[Steve Harper]

Yes you may be correct. Will this function decrypt the file and replace it with the encrypted one? I have not used this function before and there is no parameter you can pass to the function in order to specify where the resultant decrypted file should go. 

The patch I posted will not change the encryption on the source file, and will keep the file safely encrypted if its copied to another place on the same volume. It will only decrypt the file if the drive does not have EFS.

Comment 9

[Benjamin Smedberg]

Comment on attachment 442951 [details] [diff] [review]
A patch that uses CopyFileW with the COPY_FILE_ALLOW_DECRYPTED_DESTINATION flag to perform the move.

Delegating review to jmathies. We can't mochitest this (since the build slaves use FAT32), but we should probably have a litmus test for this.

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 442951 [details] [diff] [review]
A patch that uses CopyFileW with the COPY_FILE_ALLOW_DECRYPTED_DESTINATION flag to perform the move.

This looks ok to me Steve. Nits however on formatting - please repost sans tab spacing and match up new code with existing formatting.

We have a mozilla code formatting guideline doc here - 
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

Comment 11

[Steve Harper]

Attachment: Patch with ammended formatting

Comment 12

[Steve Harper]

Attachment: Patch with ammended formatting

Comment 13

[Steve Harper]

Hope this patch is ok

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/f4f9d45743a8

Thanks for the patch!

Comment 15

[Andrew]

You may remove the flag about encryption, and the CopyFileEx altogether.
I have tested MoveFileExW. It works fine between file systems, one of which does support encryption, and the other does not.
There was no need for the patch.
Revert it.

You rather introduced one more, and malicious bug. It triggered another bug, which destroys data.

See the report, and attachment:
https://bugzilla.mozilla.org/show_bug.cgi?id=674742#c38

Comment 16

[Andrew]

You patch has one more bug: in case a "move operation" is not requested, you call CopyFile without the flag about encryption.
You only "patched" the case for "move".

Maybe Windows has patched their MoveFileEx since then. Maybe that it why MoveFileEx now works, and your patch no longer necessary.

A user should update one's OS.

Comment 17

[Andrew]

I proposed a patch for that.
See my attachment to the dependent bug 674742.
The summary of patches is in section "Solution".

Comment 18

[Steve Harper]

When I submitted the patch MoveFileExW didn't work as expected when copying files from an encrypted volume. I only made changes to the code when performing a "move operation" as this was all that was required in-order to fix this particular issue.

If we can rely on users keeping there OS up to date then we could remove the code.

Comment 19

[Andrew]

Then I ask Bugzilla to remove this patch.
Before doing so, check if the bug is really gone in all OS.
Or simply remove it without checks. If users encounter the bug again, they may file it here. If they won't, it would be a successful check.

How do you remove a patch?
Do you file a new patch to replace it?
Or do you revert to old code?

Your patch was a bug in itself.
Very poor job.
I described it in bug 674742, and in comment #16 above.
In short, you failed to:
- apply it to both cases (move, and copy).
- check, if CopyFileExW actually accepts your flag. You could check this both through documentation, and through a return value.

The progress of your patch proved a big failure, and disregard of rules of Bugzilla, which Bugzilla itself pretends to follow meticulously. See bug 674742.
There everyone brought up any bureaucratic hurdle to prevent progress.

Here someone filed the bug in 2003.
After 7 years Steve simply made a patch.
Nobody bothered to reproduce the bug. Not even after the 7 years to see if OS is patched.
No "automated tests", no "litmus" tests, no tests on your own computers, or anywhere.
All the reviewers, and supervisors simply committed the "patch" blindly, without analyzing the code.

This is so contrary to my experience.
When I submitted the bug 674742, everybody was asking to reproduce, and not doing anything themselves.

A look at your code would surely reveal a bug in it. So, you would make it to start with.
A look at the code related to my bug report, would spare me the effort and time. With that look they would have to examine your flawed code of a patch again.

In both cases nobody bothered to look at the code.
Only I did.
In your case they simply waved your patch on without any checks.
In my case I had to dig the guilty code by myself.
If I were to reproduce the bug, I would waste even more years.

Comment 20

[Worcester12345]

(In reply to Andrew from comment #19)
> Then I ask Bugzilla to remove this patch.
> Before doing so, check if the bug is really gone in all OS.
> Or simply remove it without checks. If users encounter the bug again, they
> may file it here. If they won't, it would be a successful check.
> 
> How do you remove a patch?
> Do you file a new patch to replace it?
> Or do you revert to old code?
> 
> Your patch was a bug in itself.
> Very poor job.
> I described it in bug 674742, and in comment #16 above.
> In short, you failed to:
> - apply it to both cases (move, and copy).
> - check, if CopyFileExW actually accepts your flag. You could check this
> both through documentation, and through a return value.
> 
> The progress of your patch proved a big failure, and disregard of rules of
> Bugzilla, which Bugzilla itself pretends to follow meticulously. See bug
> 674742.
> There everyone brought up any bureaucratic hurdle to prevent progress.
> 
> Here someone filed the bug in 2003.
> After 7 years Steve simply made a patch.
> Nobody bothered to reproduce the bug. Not even after the 7 years to see if
> OS is patched.
> No "automated tests", no "litmus" tests, no tests on your own computers, or
> anywhere.
> All the reviewers, and supervisors simply committed the "patch" blindly,
> without analyzing the code.
> 
> This is so contrary to my experience.
> When I submitted the bug 674742, everybody was asking to reproduce, and not
> doing anything themselves.
> 
> A look at your code would surely reveal a bug in it. So, you would make it
> to start with.
> A look at the code related to my bug report, would spare me the effort and
> time. With that look they would have to examine your flawed code of a patch
> again.
> 
> In both cases nobody bothered to look at the code.
> Only I did.
> In your case they simply waved your patch on without any checks.
> In my case I had to dig the guilty code by myself.
> If I were to reproduce the bug, I would waste even more years.

Ouch. 

So, is this bug actually fixed, or what?