369108 - Win32's nsIFile::Create and nsIFile::CreateUnique ignore permissions argument

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Magnus Melin [:mkmelin]]

When setting file permissions to 0400 (or anything else where user w-bit is not set), the file should be set readonly on windows. 

This spun off bug 280419 comment #15.

Comment 1

[Tal Cohen]

MSDN documents the Win32 function SetFileAttributes:

BOOL SetFileAttributes(
  LPCTSTR lpFileName,
  DWORD dwFileAttributes
);

dwFileAttributes can be set to FILE_ATTRIBUTE_READONLY.

See http://msdn2.microsoft.com/en-us/library/aa365535.aspx for details.

Alternatively, the CreateFile function also has a dwFlagsAndAttributes parameter that can accept the same constant. See http://msdn2.microsoft.com/en-us/library/aa363858.aspx for details.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch for not ignoring permission flags on Create

This should be the last fix for a while in the series of win32 nsIFile / nsILocalFile.
Sorry for the reviews :)

Regarding the fix:
If any write flag is set (user, group, other), we will not set the read only flag. 

Below is the only change in the patch that has substance.  
The other changes were just renaming wrongly named variables (the name 'flag' was used for the create disposition when it's not a flag at all).
There are a lot of other style things wrong in the file, but I don't want to break hg annotate since they are not actively harmful.

// If no write permissions are requested, and if we are possibly creating
// the file, then set the new file as read only.
if (!(mode & (PR_IWUSR | PR_IWGRP | PR_IWOTH)) &&
    disposition != OPEN_EXISTING) {
    attributes |= FILE_ATTRIBUTE_READONLY;
}


And code to test it works correctly:

var file = Components.classes["@mozilla.org/file/local;1"].
           createInstance(Components.interfaces.nsILocalFile);
file.initWithPath("C:\\test.txt");
file.create(0, 0777); // file is created with no read only flag
// Read only file
file.initWithPath("C:\\test-ro.txt");
file.create(0, 0);  // file is created with a read only flag


Note: You can also set the permissions with file.permissions = 0777; and that always worked even before this patch.  Create's permissions works consistently with file.permissions.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - rebased

Comment 4

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8b2c25cef597

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/8b2c25cef597

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 678004 [details] [diff] [review]
Patch v2 - rebased

Review of attachment 678004 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsLocalFileWin.cpp
@@ +593,4 @@
>      int32_t access = 0;
> +
> +    PRInt32 disposition = 0;
> +    PRInt32 attributes = 0;

In the future, please avoid PR* types.

Comment 7

[Brian R. Bondy [:bbondy]]

Ya sorry this is a really old patch that got lost in the shuffle, didn't notice before landing it.

Comment 8

[Robert Utasi [:hunboy]]

Brian,

by doing regression range at chatzilla by Bug 1030029 I've landed here finally.

As I see, the obsolete patch was landed instead of the newer one, and I guess this causes regression trouble at chatzilla. Our current problem is: it doesn't delete the temp file, while that is read only, and dies during plugin install.

My questions are:
1. Do I parse well the problem, if wrong patch was landed with unexpected readonly feature?
2. Do you know any workaround idea which one handles it from FF19-to fixing it, at chatzilla?

Comment 9

[Brian R. Bondy [:bbondy]]

(In reply to Robert Utasi [:hunboy] from comment #8)
> Brian,
> 
> by doing regression range at chatzilla by Bug 1030029 I've landed here
> finally.
> 
> As I see, the obsolete patch was landed instead of the newer one, 

I don't think so. Both versions of the patch are the same changes. 
One is rebased version of the other.


> and I guess this causes regression trouble at chatzilla. 

I think the nature of the change (honoring read only on Windows) 
caused the regression and not the landing of a wrong patch.

> Our current problem is:
> it doesn't delete the temp file, while that is read only, and dies during
> plugin install.
> 
> My questions are:
> 1. Do I parse well the problem, if wrong patch was landed with unexpected
> readonly feature?

No, it would have been backed out in that case.

> 2. Do you know any workaround idea which one handles it from FF19-to fixing
> it, at chatzilla?

You can adjust the file permissions before deleting it perhaps.

Comment 10

[Robert Utasi [:hunboy]]

Brian,

+      attributes |= FILE_FLAG_DELETE_ON_CLOSE;
+    }
+
+    // If no write permissions are requested, and if we are possibly creating
+    // the file, then set the new file as read only.
+    // The flag has no effect if we happen to open the file.
+    if (!(mode & (PR_IWUSR | PR_IWGRP | PR_IWOTH)) &&
+        disposition != OPEN_EXISTING) {
+        attributes |= FILE_ATTRIBUTE_READONLY;

I found this part from the obsolete and landed patch which one is not in the new patch, and I can see in the current repo. This part is suspicious to me. Was it intentional?

Comment 11

[Brian R. Bondy [:bbondy]]

I think the change was meant to be in the patch v2.
I suspect the uploaded patch v2 wasn't hg qref'ed but the landed one was.
Note that patchv1 was the reviewed one and the only intent on patch v2 was to rebase.