make packages fetched by zig set the executable bit based on ELF header and shebang line #17463
Comments
andrewrk
added
bug
|
For any potential contributors, the change to respect executable status when fetching Git packages can be made here: Lines 121 to 129 in 1f6d82e The flag |
|
i attempted this in #15382 this is the relevant code from tar.zig#L1028 in that pr. i tried linking to it but the file was too big. if (options.mode_mode == .executable_bit_only) {
if (std.fs.has_executable_bit) {
// TODO - not sure using file.mode() is correct but it seems to
// match gnu tar behavior on linux while using
// header.mode does not
const mode = try file.mode();
var modebits = std.StaticBitSet(32){ .mask = @intCast(mode) };
// copy the user exe bit to the group and other exe bits
// these bit indices count from the right:
// u g o
// rwx rwx rwx
// 876_543_210
// 0b000_000_000
const has_owner_exe_bit = modebits.isSet(6);
modebits.setValue(3, has_owner_exe_bit);
modebits.setValue(0, has_owner_exe_bit);
log.debug("mode old={o} new={o}", .{ mode, modebits.mask });
try file.chmod(modebits.mask);
}
} |
|
Note that |
|
Please help me understand why we need to read executable bit from the file system? We have information whether the file is executable while unpacking archive (both tar and git) we can pass list of executables or list of all files with modes to the hasher. That way hasher doesn't need to walk (if it has list of all files) and extract attributes from disk. We can set executable on file systems which support that, and skip on others while having same hash on both. |
|
Zig also needs the ability to compute the hash from a directory of files on disk.
Windows has had executable bit support for decades:
|
|
I tried to explain to myself how could we implement execute bit on windows. NTFS has ACL (access control list) for each file. Entries in ACL can grant or deny access to the file. They are usually inherited from parent folders but can be also set on individual files. Each ACL entry (ACE) is linked to some trustee (user or group) and has access mask. Many times that access mask is set to FullControl which toggles most of the bits on in that mask. Including file_execute bit. Here is example of reading file ACL. Initial ACL on my machine was: Access mask bits, zig constants ACL has 4 entries each of them with full control access mask. In this case all entries are inherited from parent folder. To somehow store execute bit on file system I can add one more entry to the file ACL. Second part of the example does that: Code is adding entry for Everyone with just file_execute bit set. When I need to read executable bit I should read file ACL iterate over entries, find entry with some magic trustee (Everyone or current user) and access mask which has only file execute bit set and others unset. For files which has executable bit set this way that entry will probably be first in the list, for all others we need to iterate through list. We can hack NTFS this way to store that bit but it seams to me soooo hacky, fragile and overly complicated. |
|
Note that cygwin supports fchmod with acl: |
|
Thanks @binarycraft007 for pointing to Cygwin. Cygwin tries to map posix file permissions to Windows ACL. It uses combination of allow and deny ACL entries in specific order. It is also little hacky because Windows prefer to have all deny ACL entries before all allow, and file explorer permissions interface will complain if it finds different order and offer to reorder, which will break Cygwin logic. In my example when I create file with Zig on my Windows system it initially inherits this ACL: Then in Cygwin I inspect that file and remove executable flag: That results in ACL: Cygwin has moved all entries from inherited to file specific. It relies on order and logic that entries are processed top down until explicit deny or allow for user or group in which is user is found. If I open and save this file permission in file explorer it will move all deny to the top. |
The requirement is that a fetched package can only fall into one of two categories:
here's the reasoning:
Given this, consider consider that not supporting executable bit on Windows file systems would mean that unpacking zig packages on posix systems would either mark all files as executable, or none of them as executable. With that in mind, do you still make the same recommendation (honest question)? |
|
Thanks for your time. Sorry for wasting it in persuading me. My first pragmatic recommendation is to relax 2. and remove the executable bit from the hash calculation. We will set it on posix and ignore on non-posix systems. The only downside I can imagine is that changing just the executable on some files will not make a different package and the package manager will skip replacing it with the new version. The second idea, hopefully, ticks all five points. We could save all extra information along with unpacked files. For example, if package files are stored in Besides solving this problem it will lead to some interesting optimization. I'm thinking about collecting file content hash during unpacking. There we already have all bytes piping to the file system. Unpacker (tar, git, file, ...) will produce a list with path|kind|mode|hash for each unpacked file. Later in the process filter will remove unwanted entries from the list. Hasher will use just a list, without walking the file system to calculate the resulting package hash. When we move files to the |
|
Neither of these tradeoffs are acceptable to me.
This is an ironclad rule which must not be broken.
This does not work because it is required to compute a hash based on a user's directory that does not have extra information stored. Also, I am vetoing this extra layer of complexity. I've seen this pattern many times; it creates more problems than it solves. This leaves only the set of following options to solve the problem:
Thank you for making the example code to explore the API. I agree it is unwieldy. I also have one more bit of data: using explorer.exe, I see that the "Deny Read & Execute" checkbox, when clicked, also checks the "Deny Read" checkbox which seems to imply that you cannot solely deny execution permissions. However, it does not look like this is how it works in the API, so I'm a bit confused on this point. Anyway, if we can't wrangle the Windows API, then I think the path forward is the second one: set the executable bit to 0 on posix for all files when unpacking. Nobody gets executable files in zig packages. The only use case I can think of this harming is shipping executables in zig packages. It would mean they could not be executed directly from the global package cache. Now that I'm thinking about it, I can think of a 5th option to add to the list: set the executable bit to 0 on posix for all files when unpacking, however, set the executable bit to 1 for all files that have the ELF magic header. The executable bit does not go into the hash, and is ignored when unpacking. I actually can't think of any use cases that would be harmed by this strategy - can anyone else? |
This sounds perfect to me.
The only real use case I can think of would be e.g. a package that wants to run a bash script as part of build logic, but that is non-portable anyway. Presumably the package's build.zig could mark the file as executable before running it, if needed, anyway. I can't think of a good reason to have executable files in packages at all, to be honest with you. My only care is to ensure the package hash is cross-platform, so any behavior that works everywhere I think is great. |
Perhaps for example a binary clang package, in the future so that the Zig compiler does not need to link LLVM. The more I think about it, the more I am convinced this is the path forward:
I think this solves all the problems. Just like you would not expect to have a directory missing the executable bit, you would not expect to see an ELF file missing the executable bit. |
andrewrk
changed the title
|
I explored Windows ACL a little bit more. Until now when creating files on Windows we didn't set ACL, file is inheriting it from parent. Parent usually has FullControl with all bits in access mask set. I modified createFile to accept posix like mode and create ACL rights from mode:
I create ACL entry by this mapping for current user, user default group and everyone. Resulting in three ACL entries. Those entries are set in createFileW. Now files has nice posix like permissions.
owner (current user) has GENERIC_READ | GENERIC_WRITE set This looks nice in Cygwin:
user: GENERIC_READ | GENERIC_WRITE | GENERIC_EXECUTE In Cygwin: For the package manager we can write files with mode 644 as on other systems, and executable with 755 and we can then easily distinguish between the two and read executable bit when needed. Note: |
Would be nice to come back to this at some point and remove that legacy. The purist in me doesn't like the idea of that making it into 1.0! :) |
|
Moving to 0.12 since it's needed to solve #16272. |
Based on file content. Detects elf magic header or shebang line. Executable bit is ignored in hash calculation, as it was before this. So packages hashes are not changed. Reference: ziglang#17463 (comment) Fixes: 17463 Test is here: https://github.com/ianic/zig-fetch-test/blob/7c4600d7bb263f9b72fe3d0b70071f42be89e25c/src/main.zig#L307 (if ziglang#19500 got accepted I'll move this test to the Fetch.zig)
Based on file content. Detects elf magic header or shebang line. Executable bit is ignored in hash calculation, as it was before this. So packages hashes are not changed. Reference: #17463 (comment) Fixes: 17463 Test is here: https://github.com/ianic/zig-fetch-test/blob/7c4600d7bb263f9b72fe3d0b70071f42be89e25c/src/main.zig#L307 (if #19500 got accepted I'll move this test to the Fetch.zig)
Extracted from #17392.
Depends on #17462.
This issue is to eliminate this TODO comment:
zig/src/Package/Fetch.zig
Lines 1058 to 1063 in f7bc55c
Currently, all the mode bits of all files in a package are ignored and a default mode is used instead. This is desired, except the executable bit should be set based on whether the file is an ELF file or has a shebang line, when unpacking on a posix operating system.
The text was updated successfully, but these errors were encountered: