Closed
Bug 879957
Opened 11 years ago
Closed 11 years ago
Avoid calling FileEncryptionStatusW
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: jrmuizel, Assigned: vladan)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 3 obsolete files)
2.79 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
2.53 KB,
patch
|
jimm
:
feedback+
|
Details | Diff | Splinter Review |
This, and choosing the right flags, greatly simplifies the implementation of this function. Calling FileEncryptionStatusW requires us to bring an feclient.dll
Attachment #758771 -
Flags: review?(jmathies)
Assignee | ||
Updated•11 years ago
|
Blocks: start-faster
Reporter | ||
Comment 1•11 years ago
|
||
Bug 199473 explains why this isn't needed.
No longer blocks: start-faster
Comment 2•11 years ago
|
||
Comment on attachment 758771 [details] [diff] [review] Avoid calling FileEncryptionStatusW This reintroduces bug 199473 for both copy and move operations. Move operations off an encrypted file system onto a system that doesn't support encryption (like fat32) will fail with this patch. This was the problem bug 199473 fixed. For copy operations you've taken out COPY_FILE_ALLOW_DECRYPTED_DESTINATION, so those will now fail as well. Also you stripped out the special handling for CIFS when calling move.
Attachment #758771 -
Flags: review?(jmathies) → review-
Updated•11 years ago
|
OS: Mac OS X → Windows 7
Reporter | ||
Comment 3•11 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #2) > Comment on attachment 758771 [details] [diff] [review] > Avoid calling FileEncryptionStatusW > > This reintroduces bug 199473 for both copy and move operations. Move > operations off an encrypted file system onto a system that doesn't support > encryption (like fat32) will fail with this patch. This was the problem bug > 199473 fixed. Andrew from bug 199473 claimed that was not the case. Vladan tested and Andrew was wrong. He's going to post a proper patch. > For copy operations you've taken out > COPY_FILE_ALLOW_DECRYPTED_DESTINATION, so those will now fail as well. Also > you stripped out the special handling for CIFS when calling move. Shouldn't the CIFS case be handled by MOVEFILE_COPY_ALLOWED?
Comment 4•11 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #3) > (In reply to Jim Mathies [:jimm] from comment #2) > > Comment on attachment 758771 [details] [diff] [review] > > Avoid calling FileEncryptionStatusW > > > > This reintroduces bug 199473 for both copy and move operations. Move > > operations off an encrypted file system onto a system that doesn't support > > encryption (like fat32) will fail with this patch. This was the problem bug > > 199473 fixed. > > Andrew from bug 199473 claimed that was not the case. Vladan tested and > Andrew was wrong. He's going to post a proper patch. Andrew was banned from bugzilla because he constantly insisted he was right about everything, even when he wasn't. > > For copy operations you've taken out > > COPY_FILE_ALLOW_DECRYPTED_DESTINATION, so those will now fail as well. Also > > you stripped out the special handling for CIFS when calling move. > > Shouldn't the CIFS case be handled by MOVEFILE_COPY_ALLOWED? According to msdn [1], it's supported for V3. We would have to figure out some way to set up a test environment for it to see if it works with V2, which Microsoft rolled out in Vista. Another alternative: stop using nsILocalFileWin for internal file operation where we are knowledgeable about what we are doing. This code is used by everything and everybody, as such, it has a lot of failure / fall back cases in it. Changing something in here can break stuff in wild a wooly ways. [1] http://msdn.microsoft.com/en-us/library/windows/desktop/aa365240%28v=vs.85%29.aspx
Assignee | ||
Comment 5•11 years ago
|
||
A bit of background for posterity, in case we need to revisit this: I noticed feclient.dll was being loaded early during Firefox startup, triggered by SafeOutputStream doing a file rename. I dug in and found that the call to FileEncryptionStatusW was responsible, and I'd like to avoid or postpone calling it until necessary. I posted two patches. The first patch uses MOVEFILE_COPY_ALLOWED in the initial MoveFileExW attempt and doesn't call FileEncryptionStatusW at all. The second patch is a more conservative change: it tries MoveFileExW first without MOVEFILE_COPY_ALLOWED, and if that failes it behaves the same as the current code. I tested both patches with Windows7 by copying an encrypted file to an SD card (FAT32) and a network share (SMB 2.2)
Assignee: nobody → vdjeric
Blocks: start-faster
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #758771 -
Attachment is obsolete: true
Attachment #758903 -
Flags: review?(jmathies)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #758905 -
Flags: review?(jmathies)
Comment 8•11 years ago
|
||
Comment on attachment 758903 [details] [diff] [review] Don't call FileEncryptionStatusW Review of attachment 758903 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/io/nsLocalFileWin.cpp @@ +1836,5 @@ > } > } > > if (!move) > copyOK = ::CopyFileExW(filePath.get(), destPath.get(), NULL, NULL, NULL, dwCopyFlags); I know this isn't your code, but this block has annoyed me forever. Could you please brace this code properly? @@ +1839,5 @@ > if (!move) > copyOK = ::CopyFileExW(filePath.get(), destPath.get(), NULL, NULL, NULL, dwCopyFlags); > else { > + copyOK = ::MoveFileExW(filePath.get(), destPath.get(), > + MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED); In this patch, does the addition of MOVEFILE_COPY_ALLOWED solve the SMB problem? You don't appear to be doing any special handling otherwise.
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #8) > I know this isn't your code, but this block has annoyed me forever. Could > you please brace this code properly? Sure > In this patch, does the addition of MOVEFILE_COPY_ALLOWED solve the SMB > problem? You don't appear to be doing any special handling otherwise. Yes. When copying an un-encrypted file from an NTFS disk to an SMB share on Windows 7, it fails with ERROR_NOT_SAME_DEVICE without the flag, and succeeds with the flag.
Comment 10•11 years ago
|
||
Comment on attachment 758905 [details] [diff] [review] Alternate, more conservative patch >+ copyOK = ::MoveFileExW(filePath.get(), destPath.get(), >+ MOVEFILE_REPLACE_EXISTING); >+ // MoveFileExW will fail when: >+ // 1) copying the source file to a different volume (e.g. this could be an SMBV2 mapped drive) >+ // 2) moving an encrypted file to a filesystem that does not support encryption >+ if (!copyOK && >+ (GetLastError() == ERROR_ENCRYPTION_FAILED || >+ GetLastError() == ERROR_NOT_SAME_DEVICE)) { [I wonder whether you can actually get ERROR_ENCRYPTION_FAILED if you don't use MOVEFILE_COPY_ALLOWED.] >+ DWORD status; >+ if (FileEncryptionStatusW(filePath.get(), &status) >+ && status == FILE_IS_ENCRYPTED) >+ { >+ // Note: We don't check for encryption with FileEncryptionStatusW before trying MoveFileEx >+ // because it requires loading feclient.dll, which hurts startup. >+ dwCopyFlags |= COPY_FILE_ALLOW_DECRYPTED_DESTINATION; >+ } Not being familiar with bug 199473, why do we need to test for the encryption status before adding the decryption flag?
Comment 11•11 years ago
|
||
(In reply to comment #10) > Not being familiar with bug 199473, why do we need to test for the > encryption status before adding the decryption flag? Wait, I just answered my own question. Bug 199473 only used a copy when the file was encrypted, since MoveFileExW with MOVEFILE_COPY_ALLOWED worked in all other cases. Bug 545650 then unbuffered the network move. But now, apart from the decryption flag, we're using similar code for encrypted and unencrypted cross-device moves. So we can just remove the encryption test and unconditionally set the flag.
Comment 12•11 years ago
|
||
I'll attach a -w diff that makes what's going on more obvious.
Attachment #759044 -
Flags: feedback?(vdjeric)
Comment 13•11 years ago
|
||
Attachment #759046 -
Flags: feedback?(jmuizelaar)
Updated•11 years ago
|
Attachment #759046 -
Attachment is patch: true
Updated•11 years ago
|
Attachment #759044 -
Attachment is obsolete: true
Attachment #759044 -
Flags: feedback?(vdjeric)
Updated•11 years ago
|
Attachment #758905 -
Attachment is obsolete: true
Attachment #758905 -
Flags: review?(jmathies)
Updated•11 years ago
|
Attachment #759046 -
Flags: feedback?(jmuizelaar) → feedback?(jmathies)
Comment 14•11 years ago
|
||
Comment on attachment 759046 [details] [diff] [review] Untested patch (-w) Review of attachment 759046 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/io/nsLocalFileWin.cpp @@ +1842,2 @@ > copyOK = ::MoveFileExW(filePath.get(), destPath.get(), > MOVEFILE_REPLACE_EXISTING); Could we use MOVEFILE_COPY_ALLOWED here and ditch the smb block below? We need to check support for MOVEFILE_COPY_ALLOWED as well, like COPY_FILE_ALLOW_DECRYPTED_DESTINATION it may have been introduced later than our lowest supported platform.
Attachment #759046 -
Flags: feedback?(jmathies) → feedback+
Comment 15•11 years ago
|
||
Comment on attachment 759046 [details] [diff] [review] Untested patch (-w) Review of attachment 759046 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/io/nsLocalFileWin.cpp @@ +1837,5 @@ > } > > if (!move) > copyOK = ::CopyFileExW(filePath.get(), destPath.get(), NULL, NULL, NULL, dwCopyFlags); > else { Also, Neil, if you want to pick this bug up, can you do me a favor and clean up the bracing in this code block? Also there's a ton of whitespace in here that can be removed.
Comment 16•11 years ago
|
||
(In reply to Jim Mathies from comment #14) > Could we use MOVEFILE_COPY_ALLOWED here and ditch the smb block below? Not unless you want to regress bug 545650. (In reply to Jim Mathies from comment #15) > Also, Neil, if you want to pick this bug up, can you do me a favor and clean > up the bracing in this code block? if (!move) { copyOK = ::CopyFileExW(filePath.get(), destPath.get(), NULL, NULL, NULL, dwCopyFlags); } else { copyOK = ::MoveFileExW(filePath.get(), destPath.get(), MOVEFILE_REPLACE_EXISTING); Or did you mean something else?
Comment 17•11 years ago
|
||
> > Also, Neil, if you want to pick this bug up, can you do me a favor and clean
> > up the bracing in this code block?
> if (!move)
> {
> copyOK = ::CopyFileExW(filePath.get(), destPath.get(), NULL, NULL, NULL,
> dwCopyFlags);
> }
> else
> {
> copyOK = ::MoveFileExW(filePath.get(), destPath.get(),
> MOVEFILE_REPLACE_EXISTING);
Yes, that's it.
Comment 18•11 years ago
|
||
So, for sanity, looks like your patch is doing the following: Move: ------------------- try MoveFileEx(MOVEFILE_REPLACE_EXISTING) if fails, and GetLastError() == ERROR_NOT_SAME_DEVICE try CopyFileEx(COPY_FILE_ALLOW_DECRYPTED_DESTINATION|COPY_FILE_NO_BUFFERING) Copy: ------------------- CopyFileEx(COPY_FILE_ALLOW_DECRYPTED_DESTINATION|COPY_FILE_NO_BUFFERING) Where COPY_FILE_NO_BUFFERING is only added if we detect a remote src/dest. For Move, looks like you are missing the decryption case. From a move from an encrypted src to a fat32 dest, the error would be ERROR_ENCRYPTION_FAILED.
Comment 19•11 years ago
|
||
Note vdjeric's patch catches that case.
Comment 20•11 years ago
|
||
(In reply to Jim Mathies from comment #18) > For Move, looks like you are missing the decryption case. From a move from > an encrypted src to a fat32 dest, the error would be ERROR_ENCRYPTION_FAILED. You don't get ERROR_NOT_SAME_DEVICE in that case?
Comment 21•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #20) > (In reply to Jim Mathies from comment #18) > > For Move, looks like you are missing the decryption case. From a move from > > an encrypted src to a fat32 dest, the error would be ERROR_ENCRYPTION_FAILED. > > You don't get ERROR_NOT_SAME_DEVICE in that case? Ah yes you're right. With MOVEFILE_COPY_ALLOWED you would get ERROR_ENCRYPTION_FAILED. Without that you get ERROR_NOT_SAME_DEVICE.
Assignee | ||
Comment 22•11 years ago
|
||
What's needed to land the f+'d patch?
Comment 23•11 years ago
|
||
Comment on attachment 759044 [details] [diff] [review] Untested patch Patch passed try server (after retriggering randomorange on one test). https://tbpl.mozilla.org/?tree=Try&rev=421597f50df9 Try server's post to bugzilla didn't seem to work though.
Attachment #759044 -
Attachment is obsolete: false
Attachment #759044 -
Flags: review?(jmathies)
Comment 24•11 years ago
|
||
Comment on attachment 759044 [details] [diff] [review] Untested patch Please update that bracing before this lands, thanks!
Attachment #759044 -
Flags: review?(jmathies) → review+
Updated•11 years ago
|
Attachment #758903 -
Attachment is obsolete: true
Attachment #758903 -
Flags: review?(jmathies)
Comment 26•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b5fde8a18746
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•