Skip to content

files extracted from zips are missing execute permissions #21044

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
travisstaloch opened this issue Aug 12, 2024 · 7 comments
Closed

files extracted from zips are missing execute permissions #21044

travisstaloch opened this issue Aug 12, 2024 · 7 comments
Labels
contributor friendly This issue is limited in scope and/or knowledge of Zig internals. enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@travisstaloch
Copy link
Contributor

Zig Version

0.14.0-dev.875+ebd0c6ffd

Steps to Reproduce and Observed Behavior

This issue was reported here. That project uses the package manager to download zips from https://github.com/protocolbuffers/protobuf/releases/. When I unzip one of the zip files manually, the permissions look fine.

-r-xr-xr-x 1 17M May 22 11:45 protoc

But with the package manager I guess they turn into the following as reported in the issue above.

-rw-r--r-- protoc

I asked @marler8997 about this and he said

I don't think I implemented the extension for unix file permissions.

So it sounds like this just hasn't been implemented yet. zip support was added in #19729.

Expected Behavior

Execute permissions should be preserved.

@travisstaloch travisstaloch added the bug Observed behavior contradicts documented or intended behavior label Aug 12, 2024
@travisstaloch travisstaloch changed the title extracted zip files are missing execute permissions files extracted from zips are missing execute permissions Aug 12, 2024
@andrewrk andrewrk added the contributor friendly This issue is limited in scope and/or knowledge of Zig internals. label Aug 12, 2024
@andrewrk andrewrk added this to the 0.14.0 milestone Aug 12, 2024
@andrewrk andrewrk added standard library This issue involves writing Zig code for the standard library. enhancement Solving this issue will likely involve adding new logic or components to the codebase. and removed bug Observed behavior contradicts documented or intended behavior labels Aug 12, 2024
@andrewrk
Copy link
Member

To be clear, the expected behavior from the package manager is to ignore permissions from the zip file and set the execute bit based on ELF header and shebang line alone (see #17463). I set the milestone to 0.14.0 in case that is not implemented correctly.

As for implementing extensions in the standard library, that's a separate issue.

@owenbrooks
Copy link

For MacOS, that zig-protobuf project downloads the protoc binary in Mach-O file format, so it doesn't get set to executable. Is it worth adding Mach-O header detection to the isExecutable check in addition to ELF and shebang?

@fabioarnold
Copy link
Contributor

Is it worth adding Mach-O header detection to the isExecutable check in addition to ELF and shebang?

I opened a PR here: #21555

@andrewrk andrewrk modified the milestones: 0.14.0, 0.16.0 Feb 9, 2025
@mlafeldt
Copy link
Contributor

@fabioarnold I was excited about this change and tried it with Zig 0.14.0. However, it seems that I'm holding it wrong.

.duckdb_cli_1_2_1 = .{
    .url = "https://github.com/duckdb/duckdb/releases/download/v1.2.1/duckdb_cli-osx-universal.zip",
    .hash = "N-V-__8AAKDAHwa3isStyx3RY4D3NzBMIEdXJyQsBB67L4AG",
    .lazy = true,
},
const duckdb_cli = b.lazyDependency("duckdb_cli_1_2_1", .{}) orelse return;
const cmd = Step.Run.create(b, "duckdb shell");
cmd.addFileArg(duckdb_cli.path("duckdb"));

Gives me:

error: failed to spawn and capture stdio from /Users/mathias/.cache/zig/p/N-V-__8AAKDAHwa3isStyx3RY4D3NzBMIEdXJyQsBB67L4AG/duckdb: AccessDenied

The binary is Mach-O:

❯ file /Users/mathias/.cache/zig/p/N-V-__8AAKDAHwa3isStyx3RY4D3NzBMIEdXJyQsBB67L4AG/duckdb
/Users/mathias/.cache/zig/p/N-V-__8AAKDAHwa3isStyx3RY4D3NzBMIEdXJyQsBB67L4AG/duckdb: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit executable x86_64] [arm64]
/Users/mathias/.cache/zig/p/N-V-__8AAKDAHwa3isStyx3RY4D3NzBMIEdXJyQsBB67L4AG/duckdb (for architecture x86_64):  Mach-O 64-bit executable x86_64
/Users/mathias/.cache/zig/p/N-V-__8AAKDAHwa3isStyx3RY4D3NzBMIEdXJyQsBB67L4AG/duckdb (for architecture arm64):   Mach-O 64-bit executable arm64

But it wasn't made executable:

❯ ls -lh /Users/mathias/.cache/zig/p/N-V-__8AAKDAHwa3isStyx3RY4D3NzBMIEdXJyQsBB67L4AG/duckdb
-rw-r--r--@ 1 mathias  staff    98M Mar 10 18:29 /Users/mathias/.cache/zig/p/N-V-__8AAKDAHwa3isStyx3RY4D3NzBMIEdXJyQsBB67L4AG/duckdb

Expected outcome:

❯ chmod +x /Users/mathias/.cache/zig/p/N-V-__8AAKDAHwa3isStyx3RY4D3NzBMIEdXJyQsBB67L4AG/duckdb

❯ /Users/mathias/.cache/zig/p/N-V-__8AAKDAHwa3isStyx3RY4D3NzBMIEdXJyQsBB67L4AG/duckdb
-- Loading resources from /Users/mathias/.duckdbrc
v1.2.1 8e52ec4395
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
🟡◗ 

Any idea what the problem might be?

@mlafeldt
Copy link
Contributor

mlafeldt commented Mar 10, 2025

Could this be an endianess problem?

❯ xxd /Users/mathias/.cache/zig/p/N-V-__8AAKDAHwa3isStyx3RY4D3NzBMIEdXJyQsBB67L4AG/duckdb | head -n1
00000000: cafe babe 0000 0002 0100 0007 0000 0003  ................

This seems to match FAT_CIGAM but not FAT_MAGIC, but I might be wrong based on how readInt actually works.


Update: This seems to be the root cause indeed.

I wrote a small program that checks the result of https://github.com/ziglang/zig/blob/0.14.0/src/Package/Fetch.zig#L1849-L1853 on my M1 Pro. It won't be true unless I add another condition: magic_number == std.macho.FAT_CIGAM.

(I find this strange given that readInt takes endianess into account.)

@owenbrooks
Copy link

This is because the point of the CIGAM constants is to detect when the endianness of the encoded file does not match the host endianness. See https://stackoverflow.com/a/44591490

So Fetch.zig should also check the CIGAM constants.

@mlafeldt
Copy link
Contributor

@owenbrooks Yeah, thanks. Opened a new PR: #23193

@andrewrk andrewrk modified the milestones: 0.16.0, 0.14.1 Mar 11, 2025
andrewrk pushed a commit that referenced this issue Mar 11, 2025
Fetch: enhance Mach-O executable detection for modern Macs

closes #21044
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor friendly This issue is limited in scope and/or knowledge of Zig internals. enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

No branches or pull requests

5 participants