Fix dangling uintptr bugs#139
Merged
safchain merged 2 commits intoJun 11, 2026
Merged
Conversation
Ethtool.getNames can fail when calling the ETHTOOL_GSSET_INFO ioctl
if the morestack prologue of Ethtool.ioctl grows the stack. This is
because the ssetInfo variable is stored on the stack, and uintptr is
opaque and not updated when the stack is moved.
Add a test `TestFeatureNamesStackDepth` to assert the correctness at varying
stack depths. It is expected to fail at this commit and will be fixed in
the next.
```
--- FAIL: TestFeatureNamesStackDepth (0.00s)
ethtool_test.go:224: FeatureNames("lo") incorrect at stack depth=10: got 0 features, want 60.
```
A uintptr is opaque to the runtime and so any time an unsafe.Pointer is converted into a uintptr we must ensure that the underlying pointer cannot be invalidated. The documentation for [unsafe.Pointer](https://pkg.go.dev/unsafe#Pointer) lists the safe ways to use uintptr. The relevant subsection for this patch is 2), a uintptr has no pointer semantics and it is unsafe to treat it as a live reference except in a limited set of cases. Subsection 4) describes the exception we rely on, that converting unsafe.Pointer to uintptr in an argument is live until the call completes. We'll also store unsafe.Pointer instead of uintptr in the ifreq struct. Both types have the same underlying representation and so this is equivalent to the kernel.
Owner
|
Thanks for the PR. |
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
2 participants
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.
Ethtool.getNames can fail when calling the ETHTOOL_GSSET_INFO ioctl if the morestack prologue of Ethtool.ioctl grows the stack. This is because the ssetInfo variable is stored on the stack, and uintptr is opaque and not updated when the stack is moved.
Add a test
TestFeatureNamesStackDepthto assert the correctness at varying stack depths. It is expected to fail at this commit and will be fixed in the next.A uintptr is opaque to the runtime and so any time an unsafe.Pointer is converted into a uintptr we must ensure that the underlying pointer cannot be invalidated.
The documentation for unsafe.Pointer lists the safe ways to use uintptr. The relevant subsection for this patch is 2), a uintptr has no pointer semantics and it is unsafe to treat it as a live reference except in a limited set of cases. Subsection 4) describes the exception we rely on, that converting unsafe.Pointer to uintptr in an argument is live until the call completes. We'll also store unsafe.Pointer instead of uintptr in the ifreq struct. Both types have the same underlying representation and so this is equivalent to the kernel.
I chose to introduce a failing test in the first commit so it can be checked out independently and tested. If you'd prefer each commit to be passing, let me know and I can squash or reorder :)