Last Comment Bug 199473 - Download of file to a disk may fail if temporary directory used by the download manager is encrypted.
: Download of file to a disk may fail if temporary directory used by the downlo...
Status: RESOLVED FIXED
: helpwanted
Product: Core
Classification: Components
Component: Networking: File (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla1.9.3a5
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on: 69938 destroys_encrypted 788212
Blocks: 129923
  Show dependency treegraph
 
Reported: 2003-03-27 08:20 PST by ddtizilla
Modified: 2013-06-05 13:13 PDT (History)
13 users (show)
khuey: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A patch that uses CopyFileW with the COPY_FILE_ALLOW_DECRYPTED_DESTINATION flag to perform the move. (1.88 KB, patch)
2010-05-01 16:56 PDT, Steve Harper
no flags Details | Diff | Splinter Review
A patch that uses CopyFileW with the COPY_FILE_ALLOW_DECRYPTED_DESTINATION flag to perform the move. (1.12 KB, patch)
2010-05-01 17:08 PDT, Steve Harper
no flags Details | Diff | Splinter Review
Patch with ammended formatting (1.47 KB, patch)
2010-05-08 12:58 PDT, Steve Harper
no flags Details | Diff | Splinter Review
Patch with ammended formatting (1.47 KB, patch)
2010-05-08 13:00 PDT, Steve Harper
jmathies: review+
Details | Diff | Splinter Review

Description ddtizilla 2003-03-27 08:20:24 PST
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 2004-01-12 02:23:59 PST
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 [:bz] (still a bit busy) 2004-01-12 09:25:09 PST
This is actually a networking bug, I would say.  nsIFile should handle this
transparently to consumers....
Comment 3 Cees T. 2007-04-18 07:05:38 PDT
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 2010-05-01 16:54:44 PDT
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 2010-05-01 16:56:34 PDT
Created attachment 442949 [details] [diff] [review]
A patch that uses CopyFileW with the COPY_FILE_ALLOW_DECRYPTED_DESTINATION flag to perform the move.
Comment 6 Steve Harper 2010-05-01 17:08:06 PDT
Created attachment 442951 [details] [diff] [review]
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-05-01 17:17:30 PDT
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 2010-05-02 05:05:40 PDT
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 [:bsmedberg] 2010-05-03 08:41:47 PDT
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] 2010-05-03 10:02:58 PDT
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 2010-05-08 12:58:20 PDT
Created attachment 444273 [details] [diff] [review]
Patch with ammended formatting
Comment 12 Steve Harper 2010-05-08 13:00:17 PDT
Created attachment 444274 [details] [diff] [review]
Patch with ammended formatting
Comment 13 Steve Harper 2010-05-08 13:01:45 PDT
Hope this patch is ok
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-05-23 10:52:35 PDT
http://hg.mozilla.org/mozilla-central/rev/f4f9d45743a8

Thanks for the patch!
Comment 15 Andrew 2012-08-07 13:39:29 PDT
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 2012-08-07 14:13:31 PDT
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 2012-08-09 10:43:51 PDT
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 2012-08-12 08:46:54 PDT
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 2012-08-25 16:32:18 PDT
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.

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