Improve fs__chmod() and fs__access() to honor win32 ACLs
#6
Conversation
By utilizing the `AccessCheck()` ACL API, we can interrogate the filesystem for executable permissions on a particular file for the given process/user. As this operation is slightly expensive, it is only done if the user requests it by setting the `mode` parameter with `X_OK`. In particular, this implementation checks for the `FILE_GENERIC_EXECUTE` permission within the ACL entries of the given file.
|
Epic! |
staticfloat
force-pushed
the
sf/win_acl
branch
3 times, most recently
from
c950de3 to
57a03db
Compare
|
@vtjnash so I have tried out the |
This implementation of `fs__chmod()` maps the familiar `owner`, `group`, `other` triplet of permissions within a POSIX `mode` parameter to ACL entries involving the current user, the current user's primary group as well as any groups the user may belong to that already have ACL entries within the given file object, and the `Everyone` group. We create new ACL entries explicitly allowing and denying the relevant permissions for each of these security entities, and apply the new ACL to the given file object.
|
This is ready to be merged. I rebased out a few whitespace errors and I have confirmed that on Windows, this now passes the "permissions blind test" given in this PR: https://github.com/JuliaLang/Pkg.jl/pull/1573/files I'll merge this presently, then build some new LibUV binaries and run the full test suite with it on the buildbots with a PR to Julia base. |
|
Awesome this is a great fix. @staticfloat did you consider upstreaming this to the libuv project? It seems it would be beneficial to have this not only on the julia fork but more broadly. |
|
Yes, I think we eventually should. There's an open question on whether we want to put the work in to fix |
|
Typically we disallow merging here now unless it’s been accepted upstream, so we should at least wait to release until that is done. |
|
If we disallow merging here until something has been accepted upstream, why maintain a fork? So that we can get patches that have been accepted but not merged upstream before they're merged? |
I agree—why indeed. We’re getting close on this front, though, finally. Only really a couple more fixes to upstream to close this era out. |
|
Would you prefer that I revert this, then bundle a patch in JuliaLang/julia and on Yggdrasil? |
|
No, we're even stronger against manually maintaining patch files, if we can avoid it. Those preferably need to be even closer to being merged upstream, or fixing a serious regression, before we consider adopting them. |
ERROR: LeakSanitizer: detected memory leaks
```
Direct leak of 432 byte(s) in 9 object(s) allocated from:
#0 0x1062eedc2 in __sanitizer_mz_calloc+0x92 (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x46dc2)
#1 0x7fff20171eb6 in _malloc_zone_calloc+0x3a (libsystem_malloc.dylib:x86_64+0x1beb6)
#2 0x7fff203ac180 in _CFRuntimeCreateInstance+0x124 (CoreFoundation:x86_64h+0x4180)
#3 0x7fff203ab906 in __CFStringCreateImmutableFunnel3+0x84d (CoreFoundation:x86_64h+0x3906)
#4 0x7fff203ab0a1 in CFStringCreateWithCString+0x48 (CoreFoundation:x86_64h+0x30a1)
#5 0x1056f63e1 in uv__get_cpu_speed darwin.c:267
#6 0x1056f491e in uv_cpu_info darwin.c:338
```
PR-URL: libuv#3098
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Given the building blocks here, we should also be able to upgrade
fs__stat()to give reasonable results on Windows as well, but since it's not blocking anything, that's going to have to wait for a very, VERY, rainy day.