Sys.isexecutable() unreliable on Windows

[staticfloat]

The current implementation of Sys.isexecutable() is broken on Windows; it does not properly query the relevant ACLs, and simply returns true if the file exists.

It is necessary for us to emulate the proper behavior here as git tree hashing requires knowing if a file is executable or not, and the native git tools on Windows do "the right thing", so in order to hash to the same value we will need to do the right thing as well.

The logic to do this is complex, but reducable. An example from cygwin is given here: https://github.com/Alexpux/Cygwin/blob/9c84bfd47922aad4881f80243320422b621c95dc/winsup/cygwin/sec_acl.cc#L636-L644

[musm]

@staticfloat I have a solution here, utilizing SHGetFileInfo (https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shgetfileinfoa) By obtaining the file's SHGFI_EXETYPE (https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shgetfileinfoa#return-value)

It can provide fine grain information on whether the file is:

Windows application.
MS-DOS .exe or .com file
Console application or .bat file

I didn't really look at the logic here, but it seems much complex than using the Windows API directly:

The logic to do this is complex, but reducable. An example from cygwin is given here: https://github.com/Alexpux/Cygwin/blob/9c84bfd47922aad4881f80243320422b621c95dc/winsup/cygwin/sec_acl.cc#L636-L644

Should I open a PR with the stated changes?

[StefanKarpinski]

@vtjnash, does this seem like a reasonable approach to you?

[staticfloat]

If the docs are accurate then yes, this does seem reasonable to me. Good find, @musm! I think a small PoC would be really helpful to both myself and Jameson, and doesn't look too hard to put together.

[musm]

I can just open a PR with a working implementation which I am close to finishing

[vtjnash]

This would require having the isexecutable check calling the AccessCheck function (https://docs.microsoft.com/en-us/windows/win32/secauthz/verifying-client-access-with-acls-in-c--). We could also implement this in uv_fs_access, and then use that as a cross-platform solution.

[staticfloat]

@vtjnash which would you prefer I do? I know you have performance concerns about this, would you prefer that we do this in C whenever we call uv_fs_access() or should it be controllable from Julia somehow?

[vtjnash]

Oh, I don't care about performance of this. If you mean TOCTOU, this is instantly invalid, so it could never be never fast enough. Implementation in C might be easier because it takes so many arguments, or maybe not.

[staticfloat]

Okay; I'll work up a patch to libuv and request your attention when it's ready

[staticfloat]

WIP branches:

• Julia: https://github.com/JuliaLang/julia/tree/sf/uv_fs_access
• LibUV: https://github.com/JuliaLang/libuv/tree/sf/win_acl

So far I'm having a hard time testing this; I actually am not sure what "executable" means in Windows; when I create normal files on Windows, right-click on them in Explorer and go to the Security tab, they have the Read & Execute permission enabled for the Administrators group. Does that mean that by default, most files have the executable permission set?

[StefanKarpinski]

What does git do on Windows? Or is that a meaningless question because it always runs in some UNIX emulation environment? The property we want (for ourselves—not sure if this is appropriate for libuv) is that when we unpack something, we can compute the git hash correctly. Frankly, we could make everything executable and just use some other bit of irrelevant metadata, but it seems like there must be a bit that also means the right thing to the operating system.

[staticfloat]

Okay I've actually got it working, but now I have to fix uv_fs_chmod() to also set the ACL when we are attempting to do so, otherwise we have no way of controlling the executable bit on Windows.

[StefanKarpinski]

Brilliant!

[staticfloat]

Major progress. I bashed my head against the wall for a few hours, wondering why on earth certain API calls were failing, until I realized that half of the calls in win32 return 0 on success, whereas the other half return 0 on error. After rolling my eyes hard enough to critically fail my next saving throw, I got chmod() working, for a very specific value of "working". You can try this out on your favorite windows machine by building the Julia branch. You can also see the libuv diff.

The implementation is fairly straightforward: I augment fs__access() on windows to use CheckAccess() to determine if we have executable access to a file, and I augment fs__chmod() on windows to set FILE_EXECUTE ACL permissions for the current user appropriately. This allows us to do the following:

$ cat test.jl
rm("foo")

touch("foo")
@show Sys.isexecutable("foo")
chmod("foo", 0o0666)
@show Sys.isexecutable("foo")
chmod("foo", 0o0777)
@show Sys.isexecutable("foo")

$ ~/src/julia/usr/bin/julia.exe test.jl
Sys.isexecutable("foo") = true
Sys.isexecutable("foo") = false
Sys.isexecutable("foo") = true

Party time right? Nope, not quite yet. Observe:

$ touch foo
$ ls -la foo
-rw-r--r--+ 1 Administrator None 0 Dec 17 04:21 foo
$ ~/src/julia/usr/bin/julia.exe -e '@show Sys.isexecutable("foo")'
Sys.isexecutable("foo") = false

So far so good, however:

$ ~/src/julia/usr/bin/julia.exe -e 'chmod("foo", 0o0777)'
$ ~/src/julia/usr/bin/julia.exe -e '@show Sys.isexecutable("foo")'
Sys.isexecutable("foo") = false

What happens is that the touch command in mingw64 initializes an explicit deny on executable permissions for the Administrators group:

Apparently, DENY ACL entries have priority over ALLOW ACL entries, which means that you need to remove the Administrators ACL DENY entry that conflicts with the Administrator ACL ALLOW entry for it to take effect. Wonderful.

I could write a tool that iterates over the current ACL, finds conflicting entries, and modifies them, but I want to make sure that that is the correct way of doing it before chipping away at it, since it is tiresome and annoying. I am glad that this is half-way working though.

So, current caveats for this solution:

• chmod() can be defeated by conflicting ACL entries, in a very non-obvious way.
• This only pays attention to S_IXUSR, it doesn't do anything with S_IXGRP or S_IXOTH. Doing S_IXOTH would actually be quite easy, and S_IXGRP would involve just looking up the owning group and modifying that as well, I think.

[staticfloat]

A week of vacation did wonders for my desire to return to this, and I think I managed to get something fully working today: JuliaLang/libuv#6

I did end up writing the loops to iterate over all ACL entries, find SID's that the current user belongs to (that aren't the user's own SID nor the user's primary group SID) and apply the "group" mode to those groups. It's possibly slightly heavier-handed than it should be (it always sets everything explicitly, doesn't bother with determining the possible ACL inheritance flows to minimize the number of explicit entries) but that's totally fine by me. We could upgrade fs__stat() to also give us something reasonable now that we know how this stuff works, but since that's not blocking anything, I vote just test this current implementation as best we can, and move on. :)

[martinholters]

Fixed by #35625?

[StefanKarpinski]

Seems so, but would be good to get @staticfloat's explicit approval.

[visr]

From #35625

(Jeff) fixes #33212?
(Elliot) Yes. Finally. :)

Since #35625 is on the 1.5 milestone, will it get added to that release branch still? (#35846)

[staticfloat]

Yep! I'm calling this fixed! :D

Hmmm, I don't think so. I think we decided that we're still not certain enough of the functionality here, and upstream has some notes on the implementation, so it may get tweaked. I'll remove it from the milestone.