>From cygwin-developers-return-6502-cgf=redhat.com@nowhere.com Mon Mar 10 10:34:22 2003 Date: Mon, 10 Mar 2003 10:36:26 -0500 From: "Pierre A. Humblet" MIME-Version: 1.0 To: cygwin-developers Subject: unlink Status: RO Content-Length: 2611 Lines: 59 Chris, It's a great idea to use FILE_FLAG_DELETE_ON_CLOSE for unlink, but a few bells rang when I read the code. 1) ntsec /> mkdir testdir /> chown 1002:1005 testdir /> chmod +rwxt testdir /> touch testdir/testfile /> chmod 577 testdir/testfile /> rm -f testdir/testfile rm: cannot unlink `testdir/testfile': Permission denied The problem is the DELETE flag in alloc_sd (and sec_acl). Corinna has put effort into that and when I first saw that code I thought it was an excellent idea. With the benefit of hindsight I now think it's counterproductive. POSIX semantics do not support a delete permission in the Windows sense. Not allowing DELETE in one spot forces us to *automatically* add it in unlink, so it doesn't help at all from a Cygwin user point of view. On the other hand, if a Windows user goes out of Cygwin to remove DELETE permission on a file, then one could argue that unlink should fail. Some other action should be required from the user, unlink shouldn't go out of its way to delete the file. This is symmetric to refusing to delete a file in a non-writable directory even though Windows allows it (err on the side of caution). So I would propose to remove the DELETE code from alloc_sd and sec_acl and to leave unlink as it is, in that respect. 2) Why is wincap.has_delete_on_close needed? All Windows versions have delete_on_close capability. This is a snippet on WinME with a file on a shared directory mounted on Win98. 152 325366 [main] rm 36050599 unlink: _unlink (e:\xx) 6422 331788 [main] rm 36050599 unlink: 1 = CloseHandle (0xCC) 1237 333025 [main] rm 36050599 unlink: CreateFile (FILE_FLAG_DELETE_ON_CLOSE) succeeded 158 333183 [main] rm 36050599 unlink: 0 = unlink (/e/xx) However Win95 does not have FILE_SHARE_DELETE, so if the file is already opened the CreateFile fails (and wincap.has_delete_on_close won't be looked up). Is there reason to believe that a file won't be deleted if the CreateFile succeeds? It would violate MS semantics. I couldn't create such a case, but I don't have access to many types of shared drive. Perhaps the test after the CloseHandle could be if (win32_name.isremote () && GetFileAttributes (win32_name) != INVALID_FILE_ATTRIBUTES) then handle CreateFile (FILE_FLAG_DELETE_ON_CLOSE) failed 3) I don't understand why there are so many SetFileAttributes after CreateFile succeeds. If the file will be deleted, its attributes are don't care. If it won't, then we undo twice what we just did before the CreateFile. Also it's an easy test to decide if the SetFileAttribute before CreateFile is needed. Pierre >From cygwin-developers-return-6505-cgf=redhat.com@nowhere.com Mon Mar 10 10:59:24 2003 Date: Mon, 10 Mar 2003 16:58:55 +0100 From: "Corinna Vinschen" To: cygwin-developers Subject: Re: unlink Message-ID: <20030310155855.GB1193@cygbert.vinschen.de> Reply-To: cygwin-developers Mail-Followup-To: "cygwin-developers" References: <3E6CB0FA.2312CCD0@ieee.org> In-Reply-To: <3E6CB0FA.2312CCD0@ieee.org> User-Agent: Mutt/1.4i Status: RO Content-Length: 3034 Lines: 79 On Mon, Mar 10, 2003 at 10:36:26AM -0500, Pierre A. Humblet wrote: > So I would propose to remove the DELETE code from alloc_sd and sec_acl > and to leave unlink as it is, in that respect. Like this? Index: sec_acl.cc =================================================================== RCS file: /cvs/src/src/winsup/cygwin/sec_acl.cc,v retrieving revision 1.29 diff -p -u -r1.29 sec_acl.cc --- sec_acl.cc 9 Mar 2003 20:31:07 -0000 1.29 +++ sec_acl.cc 10 Mar 2003 15:52:35 -0000 @@ -119,19 +119,13 @@ setacl (const char *file, int nentries, DWORD allow; /* Owner has more standard rights set. */ if ((aclbufp[i].a_type & ~ACL_DEFAULT) == USER_OBJ) - allow = (STANDARD_RIGHTS_ALL & ~DELETE) - | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA; + allow = STANDARD_RIGHTS_ALL | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA; else allow = STANDARD_RIGHTS_READ | FILE_READ_ATTRIBUTES | FILE_READ_EA; if (aclbufp[i].a_perm & S_IROTH) allow |= FILE_GENERIC_READ; if (aclbufp[i].a_perm & S_IWOTH) - { - allow |= STANDARD_RIGHTS_WRITE | FILE_GENERIC_WRITE; - /* Owner gets DELETE right, too. */ - if ((aclbufp[i].a_type & ~ACL_DEFAULT) == USER_OBJ) - allow |= DELETE; - } + allow |= STANDARD_RIGHTS_WRITE | FILE_GENERIC_WRITE; if (aclbufp[i].a_perm & S_IXOTH) allow |= FILE_GENERIC_EXECUTE; if ((aclbufp[i].a_perm & (S_IWOTH | S_IXOTH)) == (S_IWOTH | S_IXOTH)) Index: security.cc =================================================================== RCS file: /cvs/src/src/winsup/cygwin/security.cc,v retrieving revision 1.139 diff -p -u -r1.139 security.cc --- security.cc 9 Mar 2003 20:31:07 -0000 1.139 +++ security.cc 10 Mar 2003 15:52:35 -0000 @@ -1644,12 +1644,12 @@ alloc_sd (__uid32_t uid, __gid32_t gid, int ace_off = 0; /* Construct allow attribute for owner. */ - DWORD owner_allow = (STANDARD_RIGHTS_ALL & ~DELETE) + DWORD owner_allow = STANDARD_RIGHTS_ALL | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA; if (attribute & S_IRUSR) owner_allow |= FILE_GENERIC_READ; if (attribute & S_IWUSR) - owner_allow |= FILE_GENERIC_WRITE | DELETE; + owner_allow |= FILE_GENERIC_WRITE; if (attribute & S_IXUSR) owner_allow |= FILE_GENERIC_EXECUTE; if ((attribute & (S_IFDIR | S_IWUSR | S_IXUSR)) > 2) Why is wincap.has_delete_on_close needed? Hmm, I can't answer this one. Historical reasons? Bad experience? > 3) I don't understand why there are so many SetFileAttributes after CreateFile > succeeds. If the file will be deleted, its attributes are don't care. If it won't, > then we undo twice what we just did before the CreateFile. > Also it's an easy test to decide if the SetFileAttribute before CreateFile is needed. Chris and I are through this attribute hell this weekend. To make it short: If the file is hardlinked more than once, the other links would miss an attribute after removing this one link. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Developer Red Hat, Inc. >From cygwin-developers-return-6508-cgf=redhat.com@nowhere.com Mon Mar 10 11:18:16 2003 Message-ID: <3E6CBB60.FAC2C62D@ieee.org> Date: Mon, 10 Mar 2003 11:20:48 -0500 From: "Pierre A. Humblet" To: cygwin-developers Subject: Re: unlink References: <3E6CB0FA.2312CCD0@ieee.org> <20030310155855.GB1193@cygbert.vinschen.de> Status: RO Content-Length: 1060 Lines: 29 Corinna Vinschen wrote: > > On Mon, Mar 10, 2003 at 10:36:26AM -0500, Pierre A. Humblet wrote: > > So I would propose to remove the DELETE code from alloc_sd and sec_acl > > and to leave unlink as it is, in that respect. > > Like this? Wow. Yes. > > 2) Why is wincap.has_delete_on_close needed? > > Hmm, I can't answer this one. Historical reasons? Bad experience? > > > 3) I don't understand why there are so many SetFileAttributes after CreateFile > > succeeds. If the file will be deleted, its attributes are don't care. If it won't, > > then we undo twice what we just did before the CreateFile. > > Also it's an easy test to decide if the SetFileAttribute before CreateFile is needed. > > Chris and I are through this attribute hell this weekend. To make it > short: If the file is hardlinked more than once, the other links would > miss an attribute after removing this one link. I think I see. When a file is hard linked, SetFileAttributes affects *all* links. Correct? Still, it's easy to decide if the calls are needed. Pierre >From cygwin-developers-return-6507-cgf=redhat.com@nowhere.com Mon Mar 10 11:17:25 2003 Date: Mon, 10 Mar 2003 11:16:28 -0500 From: "Christopher Faylor" To: cygwin-developers Subject: Re: unlink Message-ID: <20030310161628.GG23549@redhat.com> Reply-To: cygwin-developers Mail-Followup-To: cygwin-developers References: <3E6CB0FA.2312CCD0@ieee.org> Content-Length: 876 Lines: 25 On Mon, Mar 10, 2003 at 10:36:26AM -0500, Pierre A. Humblet wrote: >Chris, > >It's a great idea to use FILE_FLAG_DELETE_ON_CLOSE for unlink, >but a few bells rang when I read the code. AFAIK, I didn't add anything here. I just changed the order. unlink has always used FILE_FLAG_DELETE_ON_CLOSE. >2) Why is wincap.has_delete_on_close needed? > >All Windows versions have delete_on_close capability. This is a snippet >on WinME with a file on a shared directory mounted on Win98. It doesn't work quite right on Windows 98. I tested it extensively a while ago and it seemed unreliable. Maybe it is ok in the current incarnation. I guess I could remove that test and see if anyone hollers. >3) I don't understand why there are so many SetFileAttributes after CreateFile > succeeds. If the file will be deleted, its attributes are don't care. Think 'hard links'. cgf