789816 - Clean up the copy/move file code in nsLocalFileWin

Status: NEW

(Core :: XPCOM, defect)

Description

[Jim Mathies [:jimm]]

I'll pull the win2k code out of the work in bug 788212 and repost it here.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

Comment 2

[[e-mail]]

Comment on attachment 659599 [details] [diff] [review]
patch

>+        if (encryptStatus == FILE_IS_ENCRYPTED) {
If FileEncryptionStatusW failed then encryptStatus is undefined. Maybe it would be better to test for dwCopyFlags & COPY_FILE_ALLOW_DECRYPTED_DESTINATION?

>     else if (move && !skipNtfsAclReset)
>     {
>         // Set security permissions to inherit from parent.
>         // Note: propagates to all children: slow for big file trees
[Unrelated note: if we end up copying the file, do we still need to do this?]

Comment 3

[Masatoshi Kimura [:emk]]

>+    DWORD dwVersion = GetVersion();
>+    DWORD dwMajorVersion = (DWORD)(LOBYTE(LOWORD(dwVersion)));
>+    bool isVistaAndUp = (dwMajorVersion > 5);
Just use IsVistaOrLater().

Comment 4

[Masatoshi Kimura [:emk]]

>+    // Check encryption status: if the src is encrypted, we'll designate that
>+    // the target can be unencrypted on the copy if the target drive does not
>+    // support encryption.
>+    DWORD encryptStatus;
>+    if (FileEncryptionStatusW(filePath.get(), &encryptStatus) &&
>+        encryptStatus == FILE_IS_ENCRYPTED) {
>+        // Note this flag is incompatible with os <= XPSP1. We don't
>+        // support these so it's not addressed here.
>+        dwCopyFlags |= COPY_FILE_ALLOW_DECRYPTED_DESTINATION;
>+    }
Is this test required at all while we don't support <= XPSP1? It would be enough to always set COPY_FILE_ALLOW_DECRYPTED_DESTINATION.

Comment 5

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #4)
> >+    // Check encryption status: if the src is encrypted, we'll designate that
> >+    // the target can be unencrypted on the copy if the target drive does not
> >+    // support encryption.
> >+    DWORD encryptStatus;
> >+    if (FileEncryptionStatusW(filePath.get(), &encryptStatus) &&
> >+        encryptStatus == FILE_IS_ENCRYPTED) {
> >+        // Note this flag is incompatible with os <= XPSP1. We don't
> >+        // support these so it's not addressed here.
> >+        dwCopyFlags |= COPY_FILE_ALLOW_DECRYPTED_DESTINATION;
> >+    }
> Is this test required at all while we don't support <= XPSP1? It would be
> enough to always set COPY_FILE_ALLOW_DECRYPTED_DESTINATION.
Ah, encryptStatus  was used below. Please disregard this comment.

Comment 6

[Masatoshi Kimura [:emk]]

But FileEncryptionStatusW() will only be required for the move branch. We can always set COPY_FILE_ALLOW_DECRYPTED_DESTINATION for the !move branch. We shouldn't slow down the !move path unnecessarily.

Comment 7

[[e-mail]]

(In reply to Masatoshi Kimura from comment #6)
> But FileEncryptionStatusW() will only be required for the move branch.
Nice idea; this would fix my (first) query from comment #2 too.

Comment 8

[Jim Mathies [:jimm]]

Thanks for the suggestions.

I've been testing the move and file apis. I think we can simplify this even more.

Comment 9

[Jim Mathies [:jimm]]

Attachment: base patch

Base patch with additional changes suggested here.

Comment 10

[Jim Mathies [:jimm]]

Attachment: added opt patch

Comment 11

[Jim Mathies [:jimm]]

Attachment: test app cpp

Comment 12

[Jim Mathies [:jimm]]

When the src is encrypted:

same volume: MoveFileEx will move the file and keep the encryption
different volume: MoveFileEx will fail with ERROR_NOT_SAME_DEVICE

I tested this by creating an encrypted folder on my ntfs drive. MoveFileEx had no issues with moving files out of the encrypted folder, it kept the encryption and moved the file. However when trying to move the file to a fat32 usb fob, it failed ERROR_NOT_SAME_DEVICE, at which point the copy / decrypt option was needed.

Comment 13

[[e-mail]]

What's the expected behaviour though? See bug 199473, r=jimm...

Comment 14

[Jim Mathies [:jimm]]

I'm a little concerned about ignoring behavior on os <= XPSP1. Isn't it possible some mozilla apps or 3rd party projects are still being used on these os? Or have we introduced code that would cause failures preventing this?

I know we have a check in the installer for xpsp2 (bug 668574). But I'm not sure about all mozilla apps.

Comment 15

[[e-mail]]

Comment on attachment 659654 [details] [diff] [review]
base patch

>+static bool
>+IsFileEncrypted(nsString *aFilePath)
Passing an nsString* is probably the worst choice of parameter type.
const nsString& is typical although const some_char_type* also works here.

>+    DWORD dwCopyFlags = 0;
You could initialise this to COPY_FILE_ALLOW_DECRYPTED_DESTINATION since you always want it set.

>+        dwCopyFlags = COPY_FILE_NO_BUFFERING;
[Would need to change this to |= of course]

Comment 16

[Jim Mathies [:jimm]]

(In reply to [e-mail] from comment #13)
> What's the expected behaviour though? See bug 199473, r=jimm...

To move the file, one way or another.

Comment 17

[Jim Mathies [:jimm]]

(In reply to [e-mail] from comment #15)
> Comment on attachment 659654 [details] [diff] [review]
> base patch
> 
> >+static bool
> >+IsFileEncrypted(nsString *aFilePath)
> Passing an nsString* is probably the worst choice of parameter type.
> const nsString& is typical although const some_char_type* also works here.

If we take the second patch, this can come out, otherwise I can update it to use const nsString&.

> 
> >+    DWORD dwCopyFlags = 0;
> You could initialise this to COPY_FILE_ALLOW_DECRYPTED_DESTINATION since you
> always want it set.

I wanted the comment on COPY_FILE_ALLOW_DECRYPTED_DESTINATION separate from the variable declaration. *shrug*

Comment 18

[Jim Mathies [:jimm]]

We can also get rid of dwMoveFlags.

Comment 19

[Jim Mathies [:jimm]]

Attachment: base patch

Comment 20

[Jim Mathies [:jimm]]

Attachment: plus patch

Comment 21

[[e-mail]]

(In reply to Jim Mathies from comment #14)
> I'm a little concerned about ignoring behavior on os <= XPSP1. Isn't it
> possible some mozilla apps or 3rd party projects are still being used on
> these os? Or have we introduced code that would cause failures preventing
> this?
I thought that the VC10 CRT requires XPSP2 or later.

(In reply to Jim Mathies from comment #16)
> (In reply to comment #13)
> > What's the expected behaviour though? See bug 199473, r=jimm...
> To move the file, one way or another.
Ah, so the reason the old code failed was that it tried to move the file, found it was a different device, tried to copy the file (via the MOVEFILE_COPY_ALLOWED flag), and that failed because it couldn't maintain encryption. The patch in bug 199473 always copies encrypted files, but I assume the destination file still remains encrypted if at all possible? I wonder what the error from MoveFileEx was in that case, I guess it was probably something unhelpful.

(In reply to Jim Mathies from comment #17)
> If we take the second patch, this can come out
Good point.

> I wanted the comment on COPY_FILE_ALLOW_DECRYPTED_DESTINATION separate from
> the variable declaration. *shrug*
Well, perhaps put it before the optional COPY_FILE_NO_BUFFERING block?

Comment 22

[Jim Mathies [:jimm]]

(In reply to [e-mail] from comment #21)
> (In reply to Jim Mathies from comment #14)
> > I'm a little concerned about ignoring behavior on os <= XPSP1. Isn't it
> > possible some mozilla apps or 3rd party projects are still being used on
> > these os? Or have we introduced code that would cause failures preventing
> > this?
> I thought that the VC10 CRT requires XPSP2 or later.

Yes, I believe so. We officially depreciated so <= xpsp1 when we switched to msvc10.

> 
> (In reply to Jim Mathies from comment #16)
> > (In reply to comment #13)
> > > What's the expected behaviour though? See bug 199473, r=jimm...
> > To move the file, one way or another.
> Ah, so the reason the old code failed was that it tried to move the file,
> found it was a different device, tried to copy the file (via the
> MOVEFILE_COPY_ALLOWED flag), and that failed because it couldn't maintain
> encryption. The patch in bug 199473 always copies encrypted files, but I
> assume the destination file still remains encrypted if at all possible?

Yes it does. The test app I posted can be used to confirm this.

> > I wanted the comment on COPY_FILE_ALLOW_DECRYPTED_DESTINATION separate from
> > the variable declaration. *shrug*
> Well, perhaps put it before the optional COPY_FILE_NO_BUFFERING block?

will do.

I think we are leaning toward the "plus patch" at this point.

Comment 23

[Jim Mathies [:jimm]]

(In reply to [e-mail] from comment #2)
> >     else if (move && !skipNtfsAclReset)
> >     {
> >         // Set security permissions to inherit from parent.
> >         // Note: propagates to all children: slow for big file trees
> [Unrelated note: if we end up copying the file, do we still need to do this?]

Not sure, for reference it was added in bug 224692. Looking at the code, I'm confused as to why we call GetNamedSecurityInfoW on destPath?

Comment 24

[[e-mail]]

(In reply to Jim Mathies from comment #23)
> Not sure, for reference it was added in bug 224692. Looking at the code, I'm
> confused as to why we call GetNamedSecurityInfoW on destPath?
We've already moved the file at this point, and we want to reset the inherited permissions to match the new location. However if you don't read the existing permissions then you lose all the directly applied permissions. This originally bit me when renaming a file without moving it to a different folder, although I believe that particular case is now fixed by the skipNtfsAclReset.

Comment 25

[Jim Mathies [:jimm]]

Attachment: patch

Comment 26

[Jim Mathies [:jimm]]

We could clean this up even more by combining the two copy calls, but maybe that's going to far. :)

if move
 try move

if move and failed or copy
 try copy

if move and succeeded
 delete src

Comment 27

[[e-mail]]

Comment on attachment 659669 [details] [diff] [review]
patch

>+    // By default allow decryption on file copies. Prior to XPSP2, this was
>+    // the default bahaviour for CopyFileEx. This flag is incompatible with
>+    // os <= XPSP1 which we no longer support.
>+    dwCopyFlags |= COPY_FILE_ALLOW_DECRYPTED_DESTINATION;
Out of interest, where do you get the XPSP1 limitation? I can only find a 2K/XP distinction, and no mention of service packs.