Repository navigation
fix(libkernel): build the file, module and rtld calls on macOS hosts - #10552
krishalivaibhav wants to merge 12 commits into
Conversation
|
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.
Exports introduced here that are already present in
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.
|
|
Linux check: Ubuntu 24.04, g++ 13.3.0, x86-64 Release (x86-64 chroot under Rosetta in an Apple Silicon VM), on Tests: |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The macOS AppendingWritev fallback has a correctness hole and overlaps directly with the helper in open PR #10543. Here it treats any descriptor with O_APPEND as an appendable regular file and calls writev even when the saved lseek position is negative. For example, an O_APPEND pipe/non-regular descriptor can be written successfully here although pwritev should fail with ESPIPE; if saving the offset fails, the code also performs the write without being able to restore the shared descriptor position. #10543’s WritevAtEnd checks fstat/S_ISREG, refuses the fallback when the offset cannot be saved, and covers the same pwrite/pwritev behavior. Please coordinate this shared path with #10543 (or make this PR depend on/reuse its helper) and preserve the native failure behavior for non-regular or non-seekable descriptors. A regression for an O_APPEND pipe and a regular-file offset-preservation case would cover the boundary.
|
@krishalivaibhav #10627 touches the same AI-assisted: yes |
|
@maximilianfv yes, fine with #10627 landing first. I'll drop the |
imellag
left a comment
imellag
left a comment
There was a problem hiding this comment.
Tested head 972a2c6525 on Apple M4 Pro, macOS 27.0.1, Apple clang, x86-64 Release under Rosetta 2. Two points for the rebase.
Conflict in ModuleInfo.cpp. On main 3fcb5e4d76, NativeStat.cpp, SharedMemory.cpp, Stdio.cpp, DynamicLoader.cpp and Rtld.cpp compile with this PR, but ModuleInfo.cpp conflicts. #10230 rewrote sceKernelGetModuleInfoFromAddr:
- it accepts flags 1 and 2;
- it leaves
st_size0; - it builds a
GuestImageon every host for theflags == 2SystemImagecheck.
The macOS branch needs the same. Locally I used this, taking the image header from dladdr, which is what the Apple SystemImage reads:
#elif defined(__APPLE__)
Dl_info symbol{};
const bool found = Fill(static_cast<std::uintptr_t>(address), result) &&
::dladdr(reinterpret_cast<const void*>(address), &symbol) && symbol.dli_fbase != nullptr;
const GuestImage image{nullptr, found ? reinterpret_cast<std::uintptr_t>(symbol.dli_fbase) : 0};Test results. With that and the other open macOS build PRs (the same stack as in my review of #7631), guest_dynamic_loader, guest_module_args, guest_libc_threads, guest_statvfs and guest_file_timestamps pass.
guest_module_info aborts at GuestModuleInfo.cpp:68 (info.eh_frame_addr != 0). Apple ld gives the fixture only compact unwind information: otool -l libguest_module_fixture.dylib lists __text and __unwind_info, with no __eh_frame. So the Mach-O branch of Fill leaves the eh_frame fields 0. The eh_frame_hdr check on the next lines can't hold for Mach-O either.
AI-assisted: yes
972a2c6 to
e60d9df
Compare
|
@MarcZX10 thanks. I removed the macOS Since the push I also added a second commit: Tested on M2, x86_64 Release,
|
|
@imellag thanks for testing. The rebased head About AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e60d9dfa3a911ebdc0847f229eebae1878e4e487. The earlier O_APPEND fallback concern is resolved by removing that path, and the F_GETPATH addition matches the macOS descriptor-path need. I cannot approve this head yet for two concrete reasons:
- GitHub currently reports the branch as
DIRTYagainstmain; please rebase and rerun checks. - The existing cross-platform
guest_module_infotest still requires nonzeroeh_frameandeh_frame_hdrfor every non-Windows host. The reported macOS fixture has only__unwind_info, and the new Mach-OFillpath leaves both fields zero. The author’s own test note identifies this mismatch. Please add an explicit macOS expectation/fixture for these fields (or otherwise make the test pass with documented semantics) and runguest_module_infoon macOS.
The hosted product jobs are skipped by path policy, so they do not cover this macOS behavior.
|
Thanks for confirming the macOS fixture has no |
e60d9df to
3206fbd
Compare
|
@MarcZX10 thanks. I rebased onto I added a third commit for
Tested on M2, x86_64 Release,
AI-assisted: yes (Claude Code) |
github-actions
Bot
added
build
and removed
needs rebase
|
macOS test report on head I looked for the smallest set of open PRs that boots titles on macOS from current This PR is needed: with it removed from that set, the libraries don't build: One heads-up: this head now conflicts with the latest |
NativeStat, SharedMemory, Stdio, DynamicLoader, ModuleInfo and Rtld used Linux-only APIs (st_atim, memfd_create, sys/vfs.h, link.h and dl_iterate_phdr), so libkernel did not compile on macOS. Add macOS paths behind __APPLE__: st_*timespec and a long-path fstatat walk, an unlinked temp file for shared memory, statfs, dlopen/dladdr and Mach-O segment walks for module and rtld queries. The Linux and Windows paths are unchanged. Extracted from feat/macos-port (boykopovar#8658). Co-authored-by: Himansh Raj <iamthehimansh@gmail.com> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…on macOS Co-authored-by: Himansh Raj <iamthehimansh@gmail.com> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… on macOS Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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.
The macOS sceKernelGetdirentries wrote *basep and the entry records into buf without entering GuestArena::HostWrite, so the write-watch host-write path never saw those writes. It now opens HostWrite guards for buf over nbytes and for basep before it moves the descriptor offset, and holds them through every write, as the Linux path does. It also rejects a non-directory with EINVAL before the guards, matching Linux.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ed on macOS Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rite on macOS Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tory Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ea0213b to
6a68918
Compare
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
maximilianfv
left a comment
maximilianfv
left a comment
There was a problem hiding this comment.
Re-review of 6a689187b. The new commits fix all four items and both smaller points from my review of 8d601f9ec, and the A/B below shows no new failures. I am still requesting changes for three defects, items 1 to 3 below. None of them comes from the new commits. Items 1 and 2 were already there at 8d601f9ec, and item 3 comes from the rebase onto main.
I tested ea0213b6a, and you rebased while my runs were going. Between the two heads only the F_GETPATH commit changed: it is now the HostDescriptorTarget helper, which covers both of main's /proc/self/fd reads. Every other file this PR changes is the same on this tree at both heads (git merge-file of each head onto the tree's main gives identical files). I did not rebuild at 6a689187b, because the new DirectoryDescriptor.cpp needs main's archived-directory changes, which this tree does not have yet.
Earlier items
- Zero-length
readv/writev/preadv/pwritev: fixed (f0bb0ae73). I called them through lldb on the "with" build:- fd -1,
writevon a read-only descriptor andreadvon a write-only one returnEBADF(0x80020009). preadvandpwritevon a pipe returnESPIPE(0x8002001d).- Valid descriptors return 0.
- fd -1,
anyps5-shm-outside the temporary directory: fixed (88b344f1c). The check uses the sametemp_directory_path()asCreateAnonymousFile, andcanonicalresolves symbolic links the same wayF_GETPATHdoes.- Named FIFOs: fixed (
f8bd9c3ae).NativeFstatfsbehaves as before. The change a title can see isfchflagson a named FIFO, which now throws, as on Linux. sceKernelGetdirentriesdebt entry: added (6a689187b), with theRTLD_GLOBALquestion and both trace names.
Tested on Apple M1 Pro, macOS 27.0.1 (x86_64 build under Rosetta 2)
Applied at ea0213b6a to mugurc's macOS series (27e968974) merged with main at 2742004ed2, with other open macOS fixes, then toggled only this PR. As before, the head does not git apply there, so "with" is the file set from my last round plus git diff 8d601f9ec ea0213b6a. The series turns guest_module_info off on Apple, so I turned it back on in both states. Both states build with 0 errors, and the PR's lines add no warnings.
I ran the same 44 tests as last time, three times in each state, one run after another:
guest_filesystem |
guest_kernel_sys_module_internal |
guest_module_info |
guest_shm_open (the series' copy) |
44 tests | |
|---|---|---|---|---|---|
| Without this PR | Fails | Fails | module info check failed at line 34 |
Passed | 36, 36, 36 pass |
| With this PR | Passed | Passed | Passed | Passed | 39, 39, 39 pass |
The other five failures stop at the same point in both states:
guest_aioat:437(needs #10543)guest_dynamic_loaderat:57(needs #10627)guest_append_writesat:79guest_host_standard_errorguest_kernel_app_archive_mmap(mmap on a file read from app0.zar ... not implemented)
guest_refused_connect passed all six runs this time. No log has a Validation Error line.
Findings
-
core/libs/prx/libkernel/Module/src/DynamicLoader.cpp:423:dlcloseunregisters a relinked image under its Mach-O header, butmprotectregistered it under its guest load bias.mprotectregisters the image undersearch.base = image->dlpi_addr(DirectMemory/DirectMemory.cpp:1285and:1369).- The macOS
dl_iterate_phdrsetsdlpi_addrto the__APS5DATA,__metaload bias (libc/src/specifics/macos/GuestImages.cpp:33). - The relinker writes that load bias as
imageBase + rva(first PT_LOAD) - p_vaddr(relinker/elfpatcher/src/macos/MacOsImage.cpp:511). __TEXTtakes the firstLoadRva(0x10000) bytes, andGuestAllocations.cpp:336requires the image's first loaded page to be its base. The key is therefore never the header.ImageBase(:113) returns_dyld_get_image_header(index).UnregisterImage(GuestAllocations.cpp:343) finds no entry under that key and returns without doing anything.- Effect: when a title
mprotects inside a.guest.prxand then unloads it, the image's pages stay in the allocation registry. - The
guest_flexible_memory_budgetfixture cannot catch this, because it writesdli_fbaseas its load bias. - Fix: pass the
dlpi_addrthatdl_iterate_phdrreports for this image, as the macOS series'ImageBasedoes. Keep the header for theDefaultScopeSymbolcomparison withdli_fbase. - I found this by reading the code and did not run it: no test here unloads a relinked image.
-
DynamicLoader.cpp:134-145:DefaultScopeSymbolreturns null when the firstdlsym(RTLD_DEFAULT)hit is in a local module, even if a global module exports the same name.- Example: module A is loaded with flags 0 and exports
foo, then module B is loaded with 0x100 and also exportsfoo. dyld returns A'sfoofirst, the filter drops it, and the lookup returns null. - On Linux A is
RTLD_LOCAL, so the same lookup returns B'sfoo. - Fix: when the first hit is hidden, try
dlsymon each global module's handle in load order. Otherwise, state this limit in the new debt entry (docs/dev/debt/libkernel.md:145).
- Example: module A is loaded with flags 0 and exports
-
core/libs/prx/libkernel/File/src/Stdio.cpp:1267: the macOSsceKernelGetdirentriesdoes not have main's directory-descriptor branch.0a64d23bd(feat(libkernel): enumerate archived app directories) addedif (File::IsDirectoryDescriptor(fd)) return File::ReadDirectoryDescriptor(fd, buf, nbytes, basep);to the non-Windows copy. After the rebase, that branch is only in the Linux copy (:1354).- On POSIX an archived app0 directory is opened as
/dev/null(DirectoryDescriptor.cpp:246). On macOS,getdirentrieson it therefore reachesfstat, sees a file that is not a directory, and returnsEINVALinstead of the archive's entries. - Fix: add the same branch to the macOS copy, after the
HostWritechecks. - I found this by reading the code: this tree does not have main's archived directories yet.
Smaller points:
SharedMemory.cpp:133-135callstemp_directory_pathandcanonical(realpath) on every check of an unlinkedanyps5-shm-file. A function-localstatic constwould do this once.- Body,
f0bb0ae73: "as host Linux does" is not quite right. Linuxpreadvchecks that the descriptor is seekable first, so a write-only pipe end givesESPIPEon Linux andEBADFhere. The code's order (EBADFfirst) is the FreeBSD 11.4 order, so only the wording needs to change. - Question:
ImageBasethrowsstd::runtime_error(DynamicLoader.cpp:131) when noRTLD_NOLOADprobe returns the module's handle.dlclose_nid_postfixdoes not catch it, so the exception would leave anextern "C"export. I have not found an input that reaches the throw. If the fix for item 1 stores the base when the module is opened,ImageBaseand its throw are no longer needed.
Not checked here: Linux, Windows, a build at 6a689187b, and this PR together with #10543 or #10627.
AI-assisted: yes (Claude Code applied the PR, ran the tests, read the code and wrote this review).
What
Builds
libkernelon macOS hosts. Six files used Linux-only APIs and did not compile there; each now has a macOS path behind__APPLE__, and the Linux and Windows paths are unchanged:File/src/NativeStat.cpp:st_atimespec/st_mtimespec/st_ctimespec, and afstatatwalk for guest paths that no longer fitPATH_MAXonce the host root is prepended.File/src/SharedMemory.cpp: an unlinked temp file instead ofmemfd_create; descriptors are recognised throughF_GETPATH.File/src/Stdio.cpp:statfsfromsys/mount.hinstead ofsys/vfs.h, plus the directory-read and vector-IO differences of Darwin. The O_APPEND positioned-write path is left to fix(libkernel): append positioned writes on an O_APPEND descriptor on macOS #10543.Module/src/DynamicLoader.cpp,Module/src/ModuleInfo.cpp,System/src/Rtld.cpp:dlopen/dladdrhandles and Mach-O segment walks instead oflink.h/dl_iterate_phdr.A second commit makes
File/src/DirectoryDescriptor.cppresolve a descriptor's path throughfcntl(F_GETPATH)on macOS, where/proc/self/fddoes not exist (fdopendirreturned ENOTDIR).A third commit (rebased
f023a5e691) givesguest_module_infoa macOS expectation:eh_framemust match the image's__eh_framesection fromgetsectiondata,eh_frame_hdris 0 (Mach-O has none), and the fixture skips the ELF-only--no-as-neededoption andnid_patcher_dynamic_stringstest on Apple.Extracted from iamthehimansh's
feat/macos-port(#8658); Co-authored-by on both commits. The 22 explanatory//comments of the branch version were dropped to follow the no-comments rule.Further commits:
e7b3e68721(was997f6cfd4, iamthehimansh): long-path stat prefixes are opened withO_SEARCHon macOS, so a directory without read permission can still be walked.703e5dcf6a(was458b30a05):sceKernelGetdirentriesandHostWriteguard their guest outputs on macOS, with theS_ISDIRcheck the Linux path has.49c24af9a8:Rtld.cppimageContainingreturnsnullptron macOS for an address outside every image, as main does on Linux and Windows after fix(libkernel): answer the thread atexit references as measured on a PS5 #10243, instead of throwing.105e043a7c: namedshm_openand namedshm_unlinkcallNotImplemented_nid_no_patchon macOS, asdocs/dev/debt/libkernel.mdsays. The host name/anyps5-shm-plus 40 hex digits is 52 bytes against macOSPSHMNAMLEN31, and/dev/shmdoes not exist there. Anonymous shared memory is unchanged.8d601f9ec4: a debt entry for the macOS module info fields left at 0 (tls_*,init_proc_addr,fini_proc_addr,eh_frame_hdr_*) and for the non-empty Mach-O__eh_framecase theguest_module_infofixture does not cover.7e9b2f7a68: the macOSNativeIsPipematches only FIFOs whosefstatfsfails, aspipe()descriptors do, andNativeFstatfsuses it, so a named FIFO is no longer a pipe.57ec3a52ef: a zero-lengthreadv,writev,preadvorpwritevon macOS checks the descriptor, its access mode and, for the positioned calls, that it is seekable before returning 0, as host Linux does.3439513045: an unlinkedanyps5-shm-file counts as shared memory only in the temporary directoryCreateAnonymousFileuses; the not-implemented traces nameshm_openandshm_unlink.ea0213b6ac: debt entries for the macOSsceKernelGetdirentries(full re-read, O(n²) paging, index offsets) and for macOS opening every module withRTLD_GLOBAL.Tested
Rebased onto
main14c99b5 (no conflicts), macOS x86_64 Release on Apple Silicon M2:DirectoryDescriptor,NativeStat,SharedMemory,Stdio,DynamicLoader,ModuleInfo,Rtld) compile with 0 errors;Stdio.cpp,SharedMemory.cppandDynamicLoader.cppcompile with 0 errors atea0213b6ac; a standalone x86_64 program with the new empty-vector, shared-memory-directory and pipe checks gives the expected result in 12 cases (EBADF,ESPIPE, 0; temporary directory matches, another directory does not;pipe()is a pipe, a named FIFO is not);Rtld.cppandSharedMemory.cppobjects (other symbols stubbed) checks that_sceKernelRtldThreadAtexitIncrementreturns -1 andDecrementreturns 0 for&localandnullptr, and that namedshm_openandshm_unlinkon a valid name reachNotImplemented_nid_no_patch. It printsok.Not run on the rebased head: the
guest_*test programs. On currentmainthey do not link on macOS (libcGuestAllocations.cppincludes<link.h>), soguest_module_info,guest_filesystem,guest_kernel_sysand the other tests below were not re-run; anonymous shared memory was not exercised.Earlier results on the pre-rebase head (macOS: local integration of
main(e7e3e9c) + #10419 + #10366 + #10369 + #7631 + #7804 + #4630 + this PR + #10853):guest_module_info,guest_module_args,guest_libc_threads,guest_statvfs,guest_file_timestamps,guest_pthread_stack,guest_raise_exception,guest_fdopendir,guest_kernel_sys,guest_filesystempassed;guest_aioneeds #10543 andguest_dynamic_loaderneeds #10627.Linux: Ubuntu 24.04, g++ 13.3.0, x86-64 Release, on
main60465af + the first commit: libkernel builds with no warnings from the six files. The later commits are__APPLE__-only.Checklist
main; no other open PR implements the same functions (the six files are also in Macos port #7201, which is being split)NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repositoryguest_aioon macOS needs fix(libkernel): append positioned writes on an O_APPEND descriptor on macOS #10543;guest_dynamic_loaderon macOS needs fix(libkernel): keep dlerror messages whole #10627🤖 Generated with Claude Code