Add `fchmodat` fallback on Linux when `flags` is nonzero. #17954

The-King-of-Toasters · 2023-11-10T12:32:38Z

After updating the syscall list, I researched why fchmodat2 was introduced. The fchmodat syscall doesn't take a flags argument, meaning this wrapper has been subtly broken since its introduction. This pr changes fchmodat to:

Use the libc implementation if linking with libc.
Use fchmodat if flags is zero.
Use fchmodat2 if available.
Use a workaround via procfs if all else fails.

The procfs workaround is what glibc and musl do. This pr copies the glibc logic as it requires the least amount of syscalls.

I've also replaced the fchmodat tests to check for this case and ensured they passed on Linux and other systems (FreeBSD 13, NetBSD 10).

The-King-of-Toasters · 2023-11-11T11:01:44Z

Dang, I have no idea why this new test is flaky. This shouldn't even throw an error given that we're not chmoding a regular file:

    try os.fchmodat(tmp.dir.fd, "regfile", 0o600, os.AT.SYMLINK_NOFOLLOW);

The-King-of-Toasters · 2023-11-15T13:54:55Z

I think I understand what's happening. CI is failing when linking with glibc, and the version being used is 2.31. Looking at the fchmodat impl, it's easy to see what's wrong:

int
fchmodat (int fd, const char *file, mode_t mode, int flag)
{
  if (flag & ~AT_SYMLINK_NOFOLLOW)
    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
#ifndef __NR_lchmod		/* Linux so far has no lchmod syscall.  */
  if (flag & AT_SYMLINK_NOFOLLOW)
    return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP);
#endif

  return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
}

lchmod was never a syscall in its own right, so this branch is always taken. I guess the solution is to skip over using libc entirely, or check if the glibc version is known to be bad (2.32 was the first to include the workaround)

rootbeer

I've got some minor comments and suggestions. Looks like a good direction to me. That said, this is just a drive-by review, I'm not a Zig maintainer or anything (so feel free to ignore me).

lib/std/os/test.zig

lib/std/os/linux.zig

rootbeer · 2023-11-15T18:26:57Z

lib/std/os/linux.zig

+    return syscall3(.fchmodat, @bitCast(@as(isize, fd)), @intFromPtr(path), mode);
+}
+
+pub fn fchmodat2(fd: i32, path: [*:0]const u8, mode: mode_t, flags: u32) usize {


Might be worth adding a comment about this fchmod vs the other one? (And maybe note which Linux versions they came in on?)

lib/std/os.zig

lib/std/os/linux.zig

lib/std/os/test.zig

src/link/Wasm.zig

lib/std/os.zig

rootbeer · 2023-11-15T18:56:19Z

lib/std/os.zig

+    // Fallback to changing permissions using procfs:
+    //
+    // 1. Open `path` as an `O.PATH` descriptor.
+    // 2. Stat the fd and check if it isn't a symbolic link.
+    // 3. Generate the procfs reference to the fd via `/proc/self/fd/{fd}`.
+    // 4. Pass the procfs path to `chmod` with the `mode`.


Maybe factor this implementation out into a fn fchmodat_fallback(..) function?

The-King-of-Toasters · 2023-12-20T11:02:25Z

I've rebased this PR atop of master due to the recent Atomic changes. I'd really like someone from the core team to give a simple yea/nay on the changes since it is a correctness fix.

The-King-of-Toasters · 2024-01-05T01:14:28Z

FYI: I authored a similar patch that was merged into Cosmopolitan a while ago. The difference was that atomic booleans aren't used, instead relying on ENOSYS being returned.

andrewrk

Thank you, this is nice work.

I have one very specific request: please take advantage of inline to make it so that when flags is comptime-known to be 0, the fallback code is not compiled.

Inside the body of the inline function it should compute the value of use_c and then call the appropriate function.

lib/std/os.zig

The-King-of-Toasters · 2024-01-11T01:53:59Z

I don't quite follow, do you mean to split the fallback code into an inline function, or to mark os.fchmodat inline? Since use_c is the conditional for the while loop, wouldn't this have the same effect?

andrewrk · 2024-01-11T02:33:02Z

I was going to link you to the relevant section of the language reference but I realized that the section was missing, so I created it: aba8d4f

Hopefully it should be clear after reading these new docs.

The-King-of-Toasters · 2024-01-11T08:10:56Z

Hopefully it should be clear after reading these new docs.

Not really? If fchmodat is marked inline, then use_c will be comptime true if flags == 0 and the rest of the code isn't generated. But wouldn't this needlessly bloat the binary size like you mentioned?

andrewrk · 2024-01-11T08:57:08Z

I am not suggesting to inline all the logic, only the use_c calculation.

This syscall was added to simplify the the libc implementations of fchmodat, as the original syscall does not take a `flags` argument. Another syscall, `map_shadow_stack`, was also added for x86_64.

The check for determining whether to use the fallback code has been moved into an inline function as per Andrew's comments in #17954.

Based on the Linux kernel fchmodat2 test

The-King-of-Toasters · 2024-01-11T10:34:26Z

I think I understand now, is this acceptable?

BTW #18131 gives a similar improvement to a function used here, might be worth adding to the review pile ;).

andrewrk · 2024-01-12T22:13:28Z

No, there is too much logic in the inline function. Do you want me to show you?

The-King-of-Toasters · 2024-01-13T01:20:50Z

Yes please. Edit: Unless you mean something like:

inline fn skipFchmodatFallback(flags: u32) bool {
    return flags == 0;
}

Honestly I'm more confused. Is there a way for me to verify that either of these approaches?

The check for determining whether to use the fallback code has been moved into an inline function as per Andrew's comments in #17954.

andrewrk · 2024-01-14T07:00:35Z

It looks like you've unchecked the box "allow maintainers to push commits to this branch" so I moved the PR here: #18547

Have a look and let me know if it is satisfactory

The check for determining whether to use the fallback code has been moved into an inline function as per Andrew's comments in ziglang#17954.

rootbeer reviewed Nov 15, 2023

View reviewed changes

andrewrk requested changes Jan 9, 2024

View reviewed changes

lib/std/os.zig Outdated Show resolved Hide resolved

The-King-of-Toasters added 4 commits January 11, 2024 21:23

Add fchmodat2 to the Linux syscall list

de7c394

This syscall was added to simplify the the libc implementations of fchmodat, as the original syscall does not take a `flags` argument. Another syscall, `map_shadow_stack`, was also added for x86_64.

Add fchmodat2 bits to os/linux.zig

eb3a9f8

Linux: Add fchmodat fallback when flags is nonzero

05f338e

The check for determining whether to use the fallback code has been moved into an inline function as per Andrew's comments in #17954.

Rework fchmodat tests

138131a

Based on the Linux kernel fchmodat2 test

andrewrk pushed a commit that referenced this pull request Jan 14, 2024

Linux: Add fchmodat fallback when flags is nonzero

bc69d62

The check for determining whether to use the fallback code has been moved into an inline function as per Andrew's comments in #17954.

andrewrk mentioned this pull request Jan 14, 2024

Add fchmodat fallback on Linux when flags is nonzero. #18547

Merged

andrewrk closed this Jan 14, 2024

The-King-of-Toasters deleted the fchmod-fixes branch January 15, 2024 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `fchmodat` fallback on Linux when `flags` is nonzero. #17954

Add `fchmodat` fallback on Linux when `flags` is nonzero. #17954

The-King-of-Toasters commented Nov 10, 2023

The-King-of-Toasters commented Nov 11, 2023

The-King-of-Toasters commented Nov 15, 2023 •

edited

Loading

rootbeer left a comment

rootbeer Nov 15, 2023

rootbeer Nov 15, 2023

The-King-of-Toasters commented Dec 20, 2023

The-King-of-Toasters commented Jan 5, 2024

andrewrk left a comment

The-King-of-Toasters commented Jan 11, 2024 •

edited

Loading

andrewrk commented Jan 11, 2024

The-King-of-Toasters commented Jan 11, 2024

andrewrk commented Jan 11, 2024

The-King-of-Toasters commented Jan 11, 2024

andrewrk commented Jan 12, 2024

The-King-of-Toasters commented Jan 13, 2024 •

edited

Loading

andrewrk commented Jan 14, 2024

Add fchmodat fallback on Linux when flags is nonzero. #17954

Add fchmodat fallback on Linux when flags is nonzero. #17954

Conversation

The-King-of-Toasters commented Nov 10, 2023

The-King-of-Toasters commented Nov 11, 2023

The-King-of-Toasters commented Nov 15, 2023 • edited Loading

rootbeer left a comment

Choose a reason for hiding this comment

rootbeer Nov 15, 2023

Choose a reason for hiding this comment

rootbeer Nov 15, 2023

Choose a reason for hiding this comment

The-King-of-Toasters commented Dec 20, 2023

The-King-of-Toasters commented Jan 5, 2024

andrewrk left a comment

Choose a reason for hiding this comment

The-King-of-Toasters commented Jan 11, 2024 • edited Loading

andrewrk commented Jan 11, 2024

The-King-of-Toasters commented Jan 11, 2024

andrewrk commented Jan 11, 2024

The-King-of-Toasters commented Jan 11, 2024

andrewrk commented Jan 12, 2024

The-King-of-Toasters commented Jan 13, 2024 • edited Loading

andrewrk commented Jan 14, 2024

Add `fchmodat` fallback on Linux when `flags` is nonzero. #17954

Add `fchmodat` fallback on Linux when `flags` is nonzero. #17954

The-King-of-Toasters commented Nov 15, 2023 •

edited

Loading

The-King-of-Toasters commented Jan 11, 2024 •

edited

Loading

The-King-of-Toasters commented Jan 13, 2024 •

edited

Loading