Last Comment Bug 879957 - Avoid calling FileEncryptionStatusW
: Avoid calling FileEncryptionStatusW
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla24
Assigned To: Vladan Djeric (:vladan)
:
Mentors:
Depends on:
Blocks: start-faster 880296
  Show dependency treegraph
 
Reported: 2013-06-05 13:30 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2013-06-13 00:00 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Avoid calling FileEncryptionStatusW (2.22 KB, patch)
2013-06-05 13:30 PDT, Jeff Muizelaar [:jrmuizel]
jmathies: review-
Details | Diff | Review
Don't call FileEncryptionStatusW (2.05 KB, patch)
2013-06-05 17:15 PDT, Vladan Djeric (:vladan)
no flags Details | Diff | Review
Alternate, more conservative patch (2.51 KB, patch)
2013-06-05 17:15 PDT, Vladan Djeric (:vladan)
no flags Details | Diff | Review
Untested patch (2.79 KB, patch)
2013-06-06 02:59 PDT, neil@parkwaycc.co.uk
jmathies: review+
Details | Diff | Review
Untested patch (-w) (2.53 KB, patch)
2013-06-06 03:02 PDT, neil@parkwaycc.co.uk
jmathies: feedback+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2013-06-05 13:30:03 PDT
Created attachment 758771 [details] [diff] [review]
Avoid calling FileEncryptionStatusW

This, and choosing the right flags, greatly simplifies the implementation of this function.

Calling FileEncryptionStatusW requires us to bring an feclient.dll
Comment 1 Jeff Muizelaar [:jrmuizel] 2013-06-05 13:32:01 PDT
Bug 199473 explains why this isn't needed.
Comment 2 Jim Mathies [:jimm] 2013-06-05 15:03:49 PDT
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.
Comment 3 Jeff Muizelaar [:jrmuizel] 2013-06-05 15:11:16 PDT
(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 Jim Mathies [:jimm] 2013-06-05 15:30:32 PDT
(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
Comment 5 Vladan Djeric (:vladan) 2013-06-05 17:14:09 PDT
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)
Comment 6 Vladan Djeric (:vladan) 2013-06-05 17:15:13 PDT
Created attachment 758903 [details] [diff] [review]
Don't call FileEncryptionStatusW
Comment 7 Vladan Djeric (:vladan) 2013-06-05 17:15:57 PDT
Created attachment 758905 [details] [diff] [review]
Alternate, more conservative patch
Comment 8 Jim Mathies [:jimm] 2013-06-05 17:41:04 PDT
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.
Comment 9 Vladan Djeric (:vladan) 2013-06-05 18:38:50 PDT
(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 neil@parkwaycc.co.uk 2013-06-06 02:46:18 PDT
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 neil@parkwaycc.co.uk 2013-06-06 02:50:33 PDT
(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 neil@parkwaycc.co.uk 2013-06-06 02:59:44 PDT
Created attachment 759044 [details] [diff] [review]
Untested patch

I'll attach a -w diff that makes what's going on more obvious.
Comment 13 neil@parkwaycc.co.uk 2013-06-06 03:02:02 PDT
Created attachment 759046 [details] [diff] [review]
Untested patch (-w)
Comment 14 Jim Mathies [:jimm] 2013-06-06 04:25:47 PDT
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.
Comment 15 Jim Mathies [:jimm] 2013-06-06 04:28:09 PDT
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 neil@parkwaycc.co.uk 2013-06-06 07:36:21 PDT
(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 Jim Mathies [:jimm] 2013-06-06 08:08:03 PDT
> > 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 Jim Mathies [:jimm] 2013-06-06 08:17:42 PDT
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 Jim Mathies [:jimm] 2013-06-06 08:24:40 PDT
Note vdjeric's patch catches that case.
Comment 20 neil@parkwaycc.co.uk 2013-06-06 09:28:47 PDT
(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 Jim Mathies [:jimm] 2013-06-06 10:04:11 PDT
(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.
Comment 22 Vladan Djeric (:vladan) 2013-06-10 19:38:42 PDT
What's needed to land the f+'d patch?
Comment 23 neil@parkwaycc.co.uk 2013-06-11 03:25:19 PDT
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.
Comment 24 Jim Mathies [:jimm] 2013-06-11 04:16:43 PDT
Comment on attachment 759044 [details] [diff] [review]
Untested patch

Please update that bracing before this lands, thanks!
Comment 26 Ryan VanderMeulen [:RyanVM] 2013-06-11 18:10:46 PDT
https://hg.mozilla.org/mozilla-central/rev/b5fde8a18746

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