Clean up the copy/move file code in nsLocalFileWin

NEW
Unassigned

Status

()

6 years ago
4 years ago

People

(Reporter: jimm, Unassigned)

Tracking

Trunk
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
I'll pull the win2k code out of the work in bug 788212 and repost it here.
(Reporter)

Updated

6 years ago
Assignee: nobody → jmathies
(Reporter)

Comment 1

6 years ago
Created attachment 659599 [details] [diff] [review]
patch
(Reporter)

Updated

6 years ago
Attachment #659599 - Flags: review?(neil)

Comment 2

6 years ago
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?]
>+    DWORD dwVersion = GetVersion();
>+    DWORD dwMajorVersion = (DWORD)(LOBYTE(LOWORD(dwVersion)));
>+    bool isVistaAndUp = (dwMajorVersion > 5);
Just use IsVistaOrLater().
>+    // 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.
(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.
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

6 years ago
(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.
(Reporter)

Comment 8

6 years ago
Thanks for the suggestions.

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

Comment 9

6 years ago
Created attachment 659654 [details] [diff] [review]
base patch

Base patch with additional changes suggested here.
Attachment #659599 - Attachment is obsolete: true
Attachment #659599 - Flags: review?(neil)
(Reporter)

Comment 10

6 years ago
Created attachment 659655 [details] [diff] [review]
added opt patch
(Reporter)

Comment 11

6 years ago
Created attachment 659656 [details]
test app cpp
(Reporter)

Comment 12

6 years ago
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.
What's the expected behaviour though? See bug 199473, r=jimm...
(Reporter)

Comment 14

6 years ago
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 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]
(Reporter)

Comment 16

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #13)
> What's the expected behaviour though? See bug 199473, r=jimm...

To move the file, one way or another.
(Reporter)

Comment 17

6 years ago
(In reply to neil@parkwaycc.co.uk 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*
(Reporter)

Comment 18

6 years ago
We can also get rid of dwMoveFlags.
(Reporter)

Comment 19

6 years ago
Created attachment 659659 [details] [diff] [review]
base patch
Attachment #659654 - Attachment is obsolete: true
Attachment #659655 - Attachment is obsolete: true
(Reporter)

Comment 20

6 years ago
Created attachment 659660 [details] [diff] [review]
plus patch
(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?
(Reporter)

Comment 22

6 years ago
(In reply to neil@parkwaycc.co.uk 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.
(Reporter)

Comment 23

6 years ago
(In reply to neil@parkwaycc.co.uk 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?
(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.
(Reporter)

Comment 25

6 years ago
Created attachment 659669 [details] [diff] [review]
patch
Attachment #659659 - Attachment is obsolete: true
Attachment #659660 - Attachment is obsolete: true
(Reporter)

Comment 26

6 years ago
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
(Reporter)

Updated

6 years ago
Attachment #659669 - Flags: review?(neil)
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.
(Reporter)

Updated

5 years ago
Attachment #659669 - Flags: review?(neil)
(Reporter)

Updated

4 years ago
Assignee: jmathies → nobody
You need to log in before you can comment on or make changes to this bug.