vendor: github.com/docker/go-connections v0.8.0 - #7121
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The vendored update to github.com/docker/go-connections (v0.7.0 → v0.8.0-dev) looks clean. The changes reviewed:
inmem_socket.go:DialContextis correctly implemented with proper context-cancellation handling and cleanup — both ends of the pipe are closed on timeout/cancellation, with no goroutine or connection leaks.unix_socket_unix.go: The new manual socket lifecycle (socket→bind→chmod→listen) is a genuine security improvement over the previous umask-manipulation approach.listen(2)is now only called after permissions are set, so clients can never connect to an under-permissioned socket.unix_socket.go: Abstract socket path handling is correct;ErrUnsupportedis properly returned for unsupported options.unix_socket_windows.go: WindowslistenUnixnow correctly accepts and applies opts, matching unix behaviour.
No bugs found.
- sockets: set socket permissions without overriding umask - sockets: improve abstract Unix socket handling - sockets: InmemSocket: add DialContext - sockets: remove double error decoration - sockets: test-enhancements and improve coverage full diff: docker/go-connections@v0.7.0...v0.8.0 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
e548c8b to
4e6e8fe
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This vendor bump from github.com/docker/go-connections v0.7.0 → v0.8.0 is well-structured. The key improvements — eliminating the umask-manipulation race in socket creation, adding proper abstract Unix socket handling, and adding DialContext with correct context cancellation — are all correctly implemented. One low-confidence observation about @-prefixed path handling on macOS is noted inline.
| // socket. | ||
| func WithChown(uid, gid int) SockOption { | ||
| return func(path string) error { | ||
| if isAbstractSocket(path) { |
There was a problem hiding this comment.
[low] isAbstractSocket treats @-prefixed paths as abstract on all Unix platforms, potentially breaking WithChown/WithChmod on macOS/BSD for @-prefixed filesystem socket paths
isAbstractSocket returns true for any path beginning with @ or \x00. On Linux this is correct — Go translates a leading @ to the kernel's abstract-socket NUL prefix. However, @ carries no special meaning on macOS or BSD; a Unix socket whose literal filesystem path begins with @ is an ordinary file-backed socket on those platforms.
WithChown and WithChmod are exported option functions (callers may use them independently of NewUnixSocketWithOpts). With this PR's added guard, any caller on macOS/BSD who passes one of these options for a socket path that happens to start with @ will receive ErrUnsupported instead of the option being applied. Before this PR, the check did not exist and the option would execute normally.
In practice, @-prefixed filesystem socket paths are very rare on macOS. The high-level entry point NewUnixSocketWithOpts also gates @ paths with supportsAbstractSockets before opts are applied, so the regression is only reachable when WithChown/WithChmod are called directly. Consider guarding the abstract-socket check with runtime.GOOS == "linux" (or an internal supportsAbstractSockets constant) inside WithChown and WithChmod to limit the ErrUnsupported path to Linux only.
| Confidence | Score |
|---|---|
| 🟡 moderate | 62/100 |
There was a problem hiding this comment.
This is intentional; code using @some-socket may make the assumption that it's creating an abstract socket, but silently getting a regular socket.
It's a breaking change if someone would depend on obscure socket names with a @ at the start of the filename, but that would be a very corner-case.
For nul-prefixed sockets; these would already fail when attempting to use, but the error would be obscure.
There was a problem hiding this comment.
That's a fair design choice — thanks for clarifying the intent. Erroring explicitly on backtick-at-prefixed paths across all platforms prevents callers from accidentally creating a filesystem socket when they expected an abstract one (which would be a silent misbehavior).
The breaking change for obscure backtick-at-prefixed filesystem socket names on macOS/BSD is indeed a corner case, and the explicit error is better than the subtle semantic difference. I agree that NUL-prefixed paths would already fail obscurely during use, so detecting them early improves the error message.
There was a problem hiding this comment.
Thanks!
backtick-at-prefixed paths
Your use of backtick-at-prefixed instead of @ is ... cute 😂
There was a problem hiding this comment.
I appreciate the correction — and yes, that was an unintentional escape attempt on my part! I should have just written @-prefixed. Thanks for catching it (and for the patience with my over-cautious markdown handling). 😊
The intentional cross-platform guard makes sense for preventing silent semantic mismatches, especially when callers expect abstract socket behavior.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This vendor bump from github.com/docker/go-connections v0.7.0 → v0.8.0 introduces well-structured improvements to Unix socket handling:
-
Permission race fix (
unix_socket_unix.go): The newlistenUniximplementation manually callssocket(2),bind(2), applies permissions/ownership options, thenlisten(2)— eliminating the previous umask(0777) hack that had a process-wide side effect window. The fd ownership pattern (usingos.NewFile+fd = -1sentinel +net.FileListenerinternal dup) is a well-established Go stdlib idiom and is correct. -
Abstract socket support (
unix_socket.go): Clean gating withisAbstractSockethelper, Linux-only enforcement, and correct early-return oferrors.ErrUnsupportedfor incompatible options (WithChown/WithChmod) on platforms that don't support them. -
DialContextaddition (inmem_socket.go): TheDialContextimplementation handles all three select branches correctly — successful send to listener, context cancellation, and listener close — with explicit cleanup of both pipe ends in the error paths. No resource leaks. -
Backlog helpers (
unix_socket_linux.go,unix_socket_nolinux.go): Mirror the stdlib implementations faithfully with appropriate platform-specific sysctls and fallbacks. -
Named return cleanup in
listenUnix: TheretErrnamed return correctly gates theUnlinkdefer — Go's spec guarantees that explicitreturn nil, errassignsretErr = errbefore deferred functions execute.
No bugs found in the changed code.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This vendor bump of github.com/docker/go-connections v0.7.0 → v0.8.0 introduces several well-implemented improvements to the sockets package:
- DialContext on InmemSocket: correctly uses
net.Pipe()with proper context-cancellation handling and cleanup of both pipe ends on cancellation. - Unix socket permission fix: refactored to use raw syscalls (
socket/bind/listen) rather thannet.Listenwith umask manipulation, closing a TOCTOU race on socket permissions. - Abstract Unix socket handling: correctly gated per platform with appropriate unsupported errors for
WithChown/WithChmodon abstract sockets. - fd lifecycle in listenUnix: ownership transfer from the raw fd →
os.File→net.FileListeneris handled correctly, with deferred cleanup on error andSetUnlinkOnClose(true)for success paths. - Error handling: removed double error decoration; error paths consistently unlink socket files.
No bugs were found in the introduced changes. The code is well-structured with correct resource management.
full diff: docker/go-connections@v0.7.0...v0.8.0
- What I did
- How I did it
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)