Skip to content

Fix faccessat and add faccessat2#3578

Merged
sporksmith merged 3 commits intoshadow:mainfrom
sporksmith:faccessat
May 3, 2025
Merged

Fix faccessat and add faccessat2#3578
sporksmith merged 3 commits intoshadow:mainfrom
sporksmith:faccessat

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

Our faccessat handler incorrectly took an extra flags parameter, which was garbage data. This led to a heisenbug in chutney shell scripts, since sometimes the corresponding register would happen to have 0 and work, but sometimes it would have garbage data and cause the syscall to return an error.

This renames the old handler to faccessat2, which does take the flags parameter, and adds faccessat, which doesn't.

@sporksmith sporksmith added this to the Support missing syscalls milestone May 2, 2025
@sporksmith sporksmith self-assigned this May 2, 2025
@github-actions github-actions bot added Component: Testing Unit and integration tests and frameworks Component: Main Composing the core Shadow executable labels May 2, 2025
@sporksmith sporksmith force-pushed the faccessat branch 2 times, most recently from 6725fff to 2cd6e3d Compare May 3, 2025 01:13
sporksmith added 3 commits May 2, 2025 20:52
Doesn't pass yet under shadow
The implementation actually matches the faccessat2 syscall, which has a
flags parameter. faccessat doesn't (though the libc wrapper does).
This is just like faccessat2, except doesn't take a flags parameter.
@github-actions github-actions bot added the Component: Build Build/install tools and dependencies label May 3, 2025
@sporksmith sporksmith requested a review from a team May 3, 2025 01:59
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

This looks similar to #3384. I wonder if we should audit all of these to make sure we don't have other instances of these 2/not-2 syscall handlers.

Maybe double check if you haven't that we don't need to make any changes to the libc wrappers in the shim? For example do we need to add faccessat2 somewhere in src/lib/preload-libc/gen_syscall_wrappers_c.py?

@sporksmith
Copy link
Copy Markdown
Contributor Author

Maybe double check if you haven't that we don't need to make any changes to the libc wrappers in the shim? For example do we need to add faccessat2 somewhere in src/lib/preload-libc/gen_syscall_wrappers_c.py?

Yup, those are already excluded.

@sporksmith sporksmith merged commit 925d62d into shadow:main May 3, 2025
25 checks passed
sporksmith added a commit that referenced this pull request May 4, 2025
sporksmith added a commit that referenced this pull request May 4, 2025
sporksmith added a commit that referenced this pull request May 4, 2025
zydou pushed a commit to zydou/arti that referenced this pull request May 6, 2025
Mostly to get the `faccessat` fix in
<shadow/shadow#3578>. I don't *think* it's
currently affecting these tests, but is a very confusing bug when it
does bite.
hacklschorsch pushed a commit to LeastAuthority/chutney that referenced this pull request May 21, 2025
In particular, shadow/shadow#3578 fixes
`faccessat` behavior that `test-network.sh` depends on (via shell tests
like `-x`).

get faccessat fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Build Build/install tools and dependencies Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants