879957 - Avoid calling FileEncryptionStatusW

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: 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]]

Bug 199473 explains why this isn't needed.

Comment 2

[Jim Mathies [:jimm]]

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]]

(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]]

(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)]

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)]

Attachment: Don't call FileEncryptionStatusW

Comment 7

[Vladan Djeric (:vladan)]

Attachment: Alternate, more conservative patch

Comment 8

[Jim Mathies [:jimm]]

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)]

(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

[[e-mail]]

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

[[e-mail]]

(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

[[e-mail]]

Attachment: Untested patch

I'll attach a -w diff that makes what's going on more obvious.

Comment 13

[[e-mail]]

Attachment: Untested patch (-w)

Comment 14

[Jim Mathies [:jimm]]

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]]

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

[[e-mail]]

(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]]

> > 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]]

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]]

Note vdjeric's patch catches that case.

Comment 20

[[e-mail]]

(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]]

(In reply to [e-mail] 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)]

What's needed to land the f+'d patch?

Comment 23

[[e-mail]]

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]]

Comment on attachment 759044 [details] [diff] [review]
Untested patch

Please update that bracing before this lands, thanks!

Comment 25

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fde8a18746

Comment 26

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/b5fde8a18746