Closed
Bug 369108
Opened 18 years ago
Closed 12 years ago
Win32's nsIFile::Create and nsIFile::CreateUnique ignore permissions argument
Categories
(Core Graveyard :: File Handling, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla19
People
(Reporter: mkmelin, Assigned: bbondy)
References
Details
Attachments
(1 file, 1 obsolete file)
2.44 KB,
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
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.
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.
Updated•17 years ago
|
Assignee: file-handling → nobody
QA Contact: ian → file-handling
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → netzen
Assignee | ||
Updated•13 years ago
|
Summary: make nsIFile file permissions respect readonly setting → Win32's nsIFile::Create and nsIFile::CreateUnique ignore permissions argument
Assignee | ||
Comment 2•13 years ago
|
||
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.
Attachment #557057 -
Flags: review?(benjamin)
Updated•13 years ago
|
Attachment #557057 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #557057 -
Attachment is obsolete: true
Attachment #678004 -
Flags: review+
Assignee | ||
Comment 4•12 years ago
|
||
Target Milestone: --- → mozilla19
Comment 5•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
Ya sorry this is a really old patch that got lost in the shuffle, didn't notice before landing it.
Comment 8•11 years ago
|
||
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?
Flags: needinfo?(netzen)
Assignee | ||
Comment 9•11 years ago
|
||
(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.
Flags: needinfo?(netzen)
Comment 10•11 years ago
|
||
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?
Flags: needinfo?(netzen)
Assignee | ||
Comment 11•11 years ago
|
||
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.
Flags: needinfo?(netzen)
Updated•9 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•