[darwin][process]: fix errno handling and library lifetime on darwin#2119
Open
shirou wants to merge 2 commits into
Open
[darwin][process]: fix errno handling and library lifetime on darwin#2119shirou wants to merge 2 commits into
shirou wants to merge 2 commits into
Conversation
Read errno through __error() resolved before the libc call, so that the dlsym and allocation performed by symbol resolution cannot overwrite it first. Apply the same to CwdWithContext, which additionally read errno without checking for failure and could surface a stale EPERM. Share dlopen handles process-wide instead of opening and closing them per call, completing for libSystem what #1832 did for IOKit and CoreFoundation. Stop populating ReadBytes/WriteBytes on darwin, which #2117 filled with the disk figures. They mean all I/O including cache on other platforms, so reusing the disk value there would silently change what the field means depending on the platform.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Darwin process handling by making errno retrieval reliable around libc calls, sharing purego dlopen handles for the lifetime of the process to avoid dlclose-related crashes, and aligning IOCounters semantics with other platforms by not mislabeling disk-only bytes as generic I/O bytes.
Changes:
- Fix errno handling on Darwin by resolving
__error()(errno location) before the libc call and reading it only after failure, while holdingruntime.LockOSThread(). - Make Darwin dynamic library handles shared process-wide (no dlclose), with a thread-safe cached symbol-resolution map.
- Stop populating
ReadBytes/WriteByteson Darwin IOCounters (leave them at 0) and update docs/tests accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| process/process.go | Updates IOCounters field docs to clarify Darwin semantics vs platforms that expose all-I/O byte counts. |
| process/process_darwin.go | Fixes errno read timing/thread affinity for IOCountersWithContext and CwdWithContext, and stops setting generic Read/Write bytes on Darwin. |
| process/process_darwin_test.go | Strengthens struct layout validation and updates IOCounters tests to reflect disk-only semantics on Darwin. |
| internal/common/common_darwin.go | Introduces process-wide dylib handle caching (no dlclose) and thread-safe symbol caching; adds ErrnoLocation() helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
purego maps uintptr to uintptr_t, a plain integer: it neither keeps the pointee alive nor makes it escape. A Go local therefore stays on the goroutine stack, and its address goes stale as soon as the stack grows, leaving the kernel to write into the old stack while the caller reads zeroes. This was latent until the shared library handle removed the per-call Dlopen, whose deep call chain had been growing the stack before the address was taken and hiding the bug. TestMemoryInfo and TestExe caught it; TestNumThread had been asserting only n >= 0 and passing on zero.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/common/common_darwin.go:132
- CFNumberCreate was updated to accept unsafe.Pointer to avoid passing stack addresses as uintptr into purego (which can go stale on stack growth). CFNumberGetValue (and CFNumberGetValueFunc) still take valuePtr as uintptr even though it is an output buffer written by CoreFoundation, so it can suffer the same stale-address/zeroed-results issue (e.g., callers pass pointers into local structs). Consider switching CFNumberGetValue to unsafe.Pointer and updating call sites accordingly.
func (c *CoreFoundationLib) CFNumberCreate(allocator uintptr, theType int64, valuePtr unsafe.Pointer) unsafe.Pointer {
fn := getFunc[CFNumberCreateFunc](c.library, "CFNumberCreate")
return fn(allocator, theType, valuePtr)
}
| func TestIOCountersDarwinNonExistentProcess(t *testing.T) { | ||
| p := &Process{Pid: 999999} | ||
| func TestIOCounters_NonExistent(t *testing.T) { | ||
| p := &Process{Pid: 99999} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a follow-up PR on #2117.
Read errno through __error() resolved before the libc call, so that the dlsym and allocation performed by symbol resolution cannot overwrite it first. Apply the same to CwdWithContext, which additionally read errno without checking for failure and could surface a stale EPERM.
Share dlopen handles process-wide instead of opening and closing them per call, completing for libSystem what #1832 did for IOKit and CoreFoundation.
Stop populating ReadBytes/WriteBytes on darwin, which #2117 filled with the disk figures. They mean all I/O including cache on other platforms, so reusing the disk value there would silently change what the field means depending on the platform.