Respecting inherited Windows file permissions on file create
In a working copy, different directories may require different Windows permissions (access control lists). For example, some might contain code or data to be accessed by Windows services. Concretely, the Windows service might be a web server and the directory might contain HTML files. For convenience in this reproduction, assume the service runs as "LOCAL SERVICE", an account that comes with Windows. Of special note (but not shown in the following example), different portions of the working copy may require different permissions. For example, this might be because there are several different Windows services involved. For illustration, let's call the working copy root wc and a directory requiring the permissions wc\a. I don't expect (or event want) svn to store Windows permission information in its repository, but I find it undesirable that svn "arbitrarily" interferes with any manually applied permissions, particularly file permissions inherited from parent directories. svn seems to get itself into trouble when it performs what to the user is logically a file "create" through a physical file "move". This is because Windows has potentially surprising behavior in this case where it does not recalculate the inherited permissions on the file according to its new parent directory. This may appear to be an OS bug in the MoveFile/MoveFileEx API, but Microsoft blogger Raymond Chen does an excellent job explaining how things got to be this way (related to Windows "hard link" support!). The blog post is: "Moving a file does not recalculate inherited permissions" at http://blogs.msdn.com/b/oldnewthing/archive/2006/08/24/717181.aspx. Raymond Chen describes a solution: After performing the MoveFile/MoveFileEx, to "... recalculate [the file's access control entries] from the destination's inheritable properties ... [c]all the SetNamedSecurityInfo function, specifying that you want an empty, unprotected DACL." The reason I think this would be the correct handling is because the move is an implementation detail. The fix causes the permissions to end up to what they would have been had CreateFile been used instead of MoveFile. The follow shows how to reproduce. The call to icacls /reset is analogous to SetNamedSecurityInfo with a NULL DACL. The impact of this problem is that I have to follow svn update or revert operations below directories with inheritable access control entries with calls to the console command icacls /reset, even though the directory tree structure where the permissions are actually assigned was not changed by the svn operation. In practice, this is not lightning fast because I cannot trust the permissions in the subtree and recursively reset everything in the the working copy followed by re-assigning the permissions from scratch which on Windows is proportional to the number of files in the working copy. mkdir C:\FIXME cd /D C:\FIXME svnadmin create repos svn co file:///C:/FIXME/repos wc mkdir wc\a svn add wc\a @rem Grant "LOCAL SERVICE" (which exists on all Windows systems) read @rem and execute to directory a. Specify as inheritable to descendent @rem objects, both (files) (OI) and containers (subdirectories) (CI). icacls wc\a /grant "LOCAL SERVICE:(OI)(CI)(RX)" @rem Create file a.txt in directory a. @rem Display its ACL to verify that it has the above access, applied by @rem inheritance as indicated with (I). echo This is a.txt > wc\a\a.txt svn add wc\a\a.txt icacls wc\a\a.txt @rem wc\a\a.txt NT AUTHORITY\LOCAL SERVICE:(I)(RX) @remBUILTIN\Administrators:(I)(F) @remNT AUTHORITY\SYSTEM:(I)(F) @remBUILTIN\Users:(I)(RX) @remNT AUTHORITY\Authenticated Users:(I)(M) svn ci wc -m Test @rem Show what happens when svn "creates" a file in that directory. @rem The problem is it really does a move so does not follow the same @rem rules as the echo command above. Expect to see @rem "NT AUTHORITY\LOCAL SERVICE:(I)(RX)" but do not see it. del wc\a\a.txt svn up wc @rem Updating 'wc': @rem Restored 'wc\a\a.txt' @rem At revision 1. icacls wc\a\a.txt @rem wc\a\a.txt BUILTIN\Administrators:(I)(F) @remNT AUTHORITY\SYSTEM:(I)(F) @remBUILTIN\Users:(I)(RX) @remNT AUTHORITY\Authenticated Users:(I)(M) @rem Showing how to manually fix the permissions on the file. @rem Using icalcs /reset on a file will remove all permissions on the @ file and reset them according to inheritance rules. icacls wc\a\a.txt /reset icacls wc\a\a.txt @rem wc\a\a.txt NT AUTHORITY\LOCAL SERVICE:(I)(RX) @remBUILTIN\Administrators:(I)(F) @remNT AUTHORITY\SYSTEM:(I)(F) @remBUILTIN\Users:(I)(RX) @remNT AUTHORITY\Authenticated Users:(I)(M)
Re: predecessor count for the root node-revision is wrong message
Hello Daniel, Philip. I have been following the thread: "#4129 is reproducible Re: predecessor count for the root node-revision is wrong message". It looks like you all have it figured out now. Good job. Do you need any more information from me at this point? Thanks. Jason Wong.
Re: predecessor count for the root node-revision is wrong message
On Mon, Mar 19, 2012 at 1:56 PM, Daniel Shahaf wrote: > Jason Wong wrote on Mon, Mar 19, 2012 at 13:41:19 -0700: >> Hello Daniel, Philip. >> >> I have been following the thread: "#4129 is reproducible Re: >> predecessor count for the root node-revision is wrong message". >> It looks like you all have it figured out now. Good job. >> >> Do you need any more information from me at this point? Thanks. >> > > Thanks Jason. It would be useful if you could confirm that you do not > run into the error after rebuilding the server with r1302399 and > r1302613 applied. (If you run the test suite, apply r1302539 and > r1302591 too.) These revisions constitute the fix which is nominated > for inclusion in 1.6.18 and 1.7.5; see ^/subversion/branches/1.7.x/STATUS. > Hi Daniel. The developer who built the svn client is away and will probably not be back until later this week. What is your ETA for 1.7.5? Just wondering if that would released before the developer I have is back. Thanks. Jason > Cheers, > > Daniel > >> Jason Wong.
RE: Respecting inherited Windows file permissions on file create
> From: Ivan Zhakov > Date: Sat, September 28, 2013 2:46 pm > I see two possible solutions for this problem: > 1. Use hTemplate argument in CreateFile call when temporary file is created > 2. Created temporary file along original file to be replaced instead > of .svn/tmp in working copy root > > Option (1) is hard to because APR abstraction, while (2) should be > easier but solve only problem when file itself doesn't have specific > permissions. I think you are describing solutions to a different (harder) problem than I raised. You are trying to solve the case where the "disrespected" permissions were ones that were applied directly to the file that was later replaced by svn. My case was permissions implied by inheritance. I think that takes a different solution. I say that yours is harder, because the problem I described is solvable by adding a single call to SetNamedSecurityInfo with parameters whose values are easy to determine (i.e., mostly NULLs). If I follow correctly, neither of your two solutions would cause my sample reproduction code to succeed. What I would expect is defined to be identical to what would happen if svn always recreated files from scratch at their final location using default options, without any filesystem "move" calls. (Whether that is, in fact, a viable option to allow the user to configure may be another angle to consider.) For what it is worth, it is less common to assign permissions on individual files than to inherit them from directories. But in either case, I think inserting the call to SetNamedSecurtyInfo is an improvement. Users should be more understanding that the permissions were reset to inheritance than they were copied from some hidden place without respect to inheritance defined on the file's final location parent directory permissions. If I understand correctly, use of SetNamedSecurityInfo will cause the system to behave as if the move optimization (I'm assuming it is some kind of optimization) was not in place and that the file was instead written from scratch with the default security parameters on Windows.
Re: Respecting inherited Windows file permissions on file create
> Subject: Re: Respecting inherited Windows file permissions on file create > From: Ivan Zhakov > Date: Sat, September 28, 2013 3:29 pm > Solution (2) is create temporary file in > wc\a folder. In this case it will get proper inherited permissions > from wc\a folder. You may try this solution in your test by modified > permissions of wc\.svn\tmp directory. Okay, now I see. That sounds like it would work, but would it make any patch more or less invasive than it needs to be? I really don't mean to argue, but you seem to have not considered if inserting a SetNamedSecurityInfo API call right after the file move call would work (and be less invasive than other solutions), as the Microsoft blogger I referenced previously described. But hey if the tests pass, why should I complain! I could further describe ways to make this work "correctly" using Windows APIs, including the issue with permissions that were assigned directly on the file itself. These are probably not going to fit so cleanly with the abstraction layer in place. And they are not going to be as simple as calling one function with a bunch of NULL params as would be the case for SetNamedSecurityInfo. Note, it is not feasible for me to set up a Windows svn development environment to actually work on the svn code myself. So I would like some guidance as to how to best assist with this, or if necessary better explain this issue. (i.e., if I haven't adequately explained that it is bad form to leave inheritable permissions unpropagated on Windows. The Explorer shell has poor support for this case. Users can see surprising things if they try to troubleshoot merely through the Explorer GUI.)
Re: Respecting inherited Windows file permissions on file create
> From: Branko ÄŒibej > Date: Sun, 29 Sep 2013 07:14:07 +0200 > This is not in fact a bug in Subversion > The solution that jiggles the security descriptor is not acceptable. I note that svn is not touching the place where the inherit is actually assigned (a directory), yet it is interfering with it. This isn't a nice thing to do on Windows. It is nearly introducing an "inconsistency" in the file system. Just my opinion, but I feel that any time this occurs on Windows may be a bug. Whatever you call it, the effect on the user is the same. Actually, I thought it was a bug in the Windows OS until I came across the blog post I referenced. (If this all doesn't shock you, you might not be a Windows programmer!) If you want to be permissions agnostic on Windows, then I think you will want the file to end up with the default creation permissions, instead of arguably inconsistent ones that arise from the move gotcha. I think this would be an improvement from being permission hostile in this case, and ease scenarios which I hope should be valid, such as running a web server against parts of the working copy. Like svn, most Windows programs do not concern themselves with permissions. The right thing" just happens by default. The solution Ivan mentioned of creating a temp file in the same directory as the final file is common (e.g., Microsoft Office. See "Data Integrity" and "Saved Files (Same Directory as the Saved File)" in http://support.microsoft.com/kb/211632). That way, of course, has its pros and cons, too. There are numerous ways to address this. Crossing fingers that some of them are deemed acceptable and worth the effort. I did file issue #4432 as instructed. (Sorry about the double spacing due to me guessing wrong about how that text box worked.) Thanks for your consideration. I will be happy to answer any questions you might have.
Re: Respecting inherited Windows file permissions on file create
> Subject: Re: Respecting inherited Windows file permissions on file > create > From: Branko Čibej > Date: Mon, September 30, 2013 6:07 am > Whew. I'd really prefer not to second-guess Windows ACL inheritance like > this, Not to beat a dead horse, but if you call SetNamedSecurityInfo after the move, then most of the time things will end up "right" and once in a while they will end up no worse than what you have today. I don't know if SetNamedSecurityInfo is optimized for the case where it performs a no op, but you can code that optimization yourself to avoid any performance regression. (Have not researched how easy that would be. Nor the corner case where you have (F) Full Control access to .svn directory but not the final directory. Though as the file's creator/owner I think you still will be able to effect the ACL change.) (Sorry, I haven't figured out why my mail program Reply to All doesn't work quite right yet...)
Re: Subversion checked-out files not indexed in Windows search
On 3/13/2014 9:08 AM, Branko Čibej wrote:
I understand that; it simply means that when we install a file from
.svn/tmp into the working copy proper, we have to change that
attribute; either before or after the file is moved into place.
When this came up on ACLs, there seemed to be a lot of resistance to
changing the file after it was in place. Kind of switching the subject,
but if you're okay with changing files after they are in place, consider
also propagating the ACL at that time too. As has been pointed out,
below is not the best possible solution as the file momentarily has the
undesired (but current-behavior) ACL and so there could be a problem on
abort. But I think it is an improvement over the current behavior, and
it is simple to implement.
This seems to give the desired effect:
DWORD ret;
ACL emptyAcl;
if (!InitializeAcl(&emptyAcl, sizeof(ACL), ACL_REVISION)) {
return 1;
}
// UNPROTECTED_SACL_SECURITY_INFORMATION ensures to enable inheritance.
// An empty DACL must be specified, not a NULL pointer.
ret = SetNamedSecurityInfo(fileName, SE_FILE_OBJECT,
DACL_SECURITY_INFORMATION | UNPROTECTED_SACL_SECURITY_INFORMATION,
NULL, NULL, &emptyAcl, NULL);
if (ret != ERROR_SUCCESS) {
return 1;
}
I think also a check that the file supports ACLs (e.g., because FAT/etc.
systems do not).
Thanks, I'll try not to harp on this subject again in the future. It's
just the fact that SVN does not do this (even as an option that can be
turned on) happens to be my #1 issue with svn.
Re: Subversion checked-out files not indexed in Windows search
On 3/17/2014 5:40 AM, Bert Huijben wrote: And updating these attributes one at a time during checkout would be huge (not a small) performance killer during checkout, as both operations would be updating the MFT. We should try to perform as much operations as possible while having the files open anyway, as each file open (which at the kernel level includes operations as setting properties) will involve virus scanners, etc. With the current trunk code the best solution might be to just remove the setting of the 'don't index' property, as the current code already moves the files in place while still open and 100% locked from indexers. (Those indexers really slowed us down on Windows XP and 7 around the time that we introduced this flag as the indexer opened the file between us creating and moving the file) Bert As far as the ACLs, I think Windows Explorer uses this technique, so if it causes problems with scanners, these problems happen just working with files in Explorer too. It is more complexity, but you could check if the 'attributes' are incorrect and only make the API call if there is something to change. Thus, only those users affected by the problem incur any decrease in performance. (This is assuming the API's in question are not already optimized for this case; if I recall correctly, I don't think these kinds of APIs edit the modified date in any case, for example.)
