Repository navigation
build(libkernel): compile NativeStat and Rtld on macOS - #11240
iamthehimansh wants to merge 2 commits into
Conversation
NativeStat.cpp reads the Linux st_atim/st_mtim/st_ctim fields, which Darwin names st_atimespec/st_mtimespec/st_ctimespec, and Rtld.cpp includes <link.h>, which macOS does not have. Both now take a macOS branch: - NativeStat: copy the Darwin timespec fields, and stat through a directory descriptor when a host path reaches PATH_MAX (1024 on Darwin, the same as the guest limit, so a guest path near it no longer fits once the host root is prepended). - Rtld: find the image with dladdr and take the extra reference with dlopen(RTLD_NOLOAD), skipping the main executable as the Linux path does. Linux and Windows compile the same code as before.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The rest of the macOS changes are scoped to the platform, and the reported compile checks are useful. I found one permission regression in the long-path fallback; see the inline note. git diff --check and the current-main conventions check pass. Hosted eligibility, conventions, intake, labels and report pass; all product build jobs are skipped. I did not run the macOS behavior locally.
| return -1; | ||
| } | ||
| const std::string prefix = split == 0 ? std::string("/") : rest.substr(0, split); | ||
| const int next = ::openat(directory, prefix.c_str(), O_RDONLY | O_DIRECTORY | O_CLOEXEC); |
There was a problem hiding this comment.
For a long path, this opens each intermediate directory with O_RDONLY, which requires read permission on that directory. stat/fstatat only require search permission, so a caller that can traverse an execute-only directory and stat a child can now get EACCES once the path crosses PATH_MAX. macOS provides O_SEARCH for directory descriptors used for *at lookup; please use a search-only descriptor (or another approach preserving stat's permission semantics) and add a long-path regression with a non-readable but searchable intermediate directory.
There was a problem hiding this comment.
Good catch, thanks. Fixed in 078315299: the prefix walk now opens each directory with O_SEARCH | O_CLOEXEC. On Darwin O_SEARCH is O_EXEC | O_DIRECTORY, so it needs only search permission and still rejects a non-directory.
I checked it with the branch's own StatAt compiled for x86_64, on a 1,472-byte path through seven 200-byte levels set to 0111:
| open flags | result |
|---|---|
O_RDONLY | O_DIRECTORY (before) |
EACCES |
O_SEARCH (now) |
ok, size 5 |
Linux and Windows views are unchanged, and the file still compiles on macOS.
AI-assisted: yes (Cana)
The long-path fallback in NativeStat opened each intermediate directory with O_RDONLY | O_DIRECTORY, which needs read permission on it. stat and fstatat only need search permission, so a path through an execute-only directory failed with EACCES once it crossed PATH_MAX. O_SEARCH (O_EXEC | O_DIRECTORY on Darwin) needs only search permission and still rejects a non-directory.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Rechecked exact head 0783152995bab8ecac72893e9a8e116e4a0a80f9 against current main b90d009d9f73354cbea239a74a8f7e2dcd138772. Replacing O_RDONLY | O_DIRECTORY with Darwin O_SEARCH addresses the execute-only intermediate-directory regression. The author reports a focused x86_64 run on a 1,472-byte path through seven 0111 directories: the old flags returned EACCES, and O_SEARCH succeeded. I did not run that Darwin-only scenario locally, and no automated regression was added; the existing macOS runtime suite remains blocked on the other in-progress port PRs. Exact-worktree git diff --check and conventions pass; hosted eligibility, labels and conventions pass, the progress report is still running, and product build/test jobs are skipped by path policy. No other blocking issue found.
|
Update to my review note: the progress-report check has now completed successfully (run 40241871214). The product build/test jobs remain skipped by path policy. |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
Closing this in favour of @krishalivaibhav's #10552. It is older and already carries the same macOS code for this file, as @MarcZX10 pointed out. I should have checked open PRs for the same implementation, not only for the same files, before opening this one. The review fix from this PR is now offered to #10552's branch as krishalivaibhav#3, with the same code and the same check as here. Thanks for the careful review. AI-assisted: yes (Cana) |
What
Next part of splitting #8658: two
libkernelfiles that do not compile on macOS. Each change is macOS-only; Linux and Windows compile the same code as before.File/src/NativeStat.cpp: copy the Darwinst_atimespec/st_mtimespec/st_ctimespecfields, since Darwin'sstruct stathas nost_atim/st_mtim/st_ctim. Also stat through a directory descriptor when the host path reachesPATH_MAX. That is 1024 on Darwin, the same as the guest limit, so a guest path near the limit no longer fits once the host root is prepended.System/src/Rtld.cpp: macOS has no<link.h>. Find the image withdladdrand take the extra reference withdlopen(RTLD_NOLOAD), skipping the main executable as the Linux path does.@marion909's #7201 also changes these two files, inside a 100-file port; this PR carries only these two so they can land separately.
DirectMemory.cpp,SharedMemory.cpp,Stdio.cpp,DynamicLoader.cpp,ModuleInfo.cpp,Thread.cpp,Exception.cpp,Process.cppand libc'sGuestAllocations.cppstill fail on macOS; they follow in separate PRs.Tested
macOS 26 on an Apple M2 Pro, x86_64 under Rosetta 2, Release with Ninja, on
mainatb90d009d9: both objects compile, wheremainfails them. Thelibkerneltests cannot run on macOS until the files above build, so this is a compile check only. Both files passed the stat and module tests on #8658's branch. For Linux and Windows, I checked each file's view underunifdef -U__APPLE__, with-D__linux__and with-D_WIN32: it equalsmain.python3 tools/check_conventions.py --base origin/mainpasses.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)