Fix SCTPListener.Close race with concurrent Accept - #89
Open
igor-lazic wants to merge 1 commit into
Open
Conversation
SCTPListener.Close currently calls syscall.Shutdown and syscall.Close on
ln.fd without any lock or atomic guard. Concurrent AcceptSCTP and Addr
calls read ln.fd directly. This produces three observable bugs:
1. A goroutine blocked inside accept4() returns a bare syscall.EINVAL or
syscall.EBADF when Close runs concurrently, instead of a sentinel
that callers can detect (cf. net.TCPListener which returns
net.ErrClosed since Go 1.16).
2. AcceptSCTP after Close calls accept4 on the now-dead fd integer. If
the OS has reassigned that fd to an unrelated resource between Close
and Accept, accept4 either fails or, worse, returns a fresh
accepted-fd from someone else's socket.
3. AcceptSCTP always returns a non-nil *SCTPConn (wrapping fd=-1 on the
error path), violating the (net.Conn, error) API contract. Callers
that defensively call methods on the returned conn end up with
another EBADF from setsockopt etc.
This change mirrors the pattern already used by SCTPConn.Close:
- Rename SCTPListener.fd to _fd int32, accessed via atomic load/swap.
- Close uses atomic.SwapInt32 so only one caller observes a real fd;
subsequent Close returns syscall.EBADF without re-issuing
syscall.Close on a possibly-reused fd number.
- AcceptSCTP and Addr take an atomic snapshot of _fd and bail to
ErrListenerClosed / nil when the listener has been closed. If
accept4 was already blocked inside the kernel when Close ran, the
post-call atomic re-check normalises the resulting EBADF/EINVAL to
ErrListenerClosed so callers can detect graceful shutdown with a
plain == comparison instead of inspecting raw errnos.
- AcceptSCTP now returns (nil, err) on error, matching the contract
of every other net.Listener.
ErrListenerClosed is a new exported sentinel (defined with errors.New
to stay compatible with the go 1.12 directive — errors.Is and
net.ErrClosed are Go 1.13+ / 1.16+).
Tests: sctp_listener_close_test.go covers Close-during-Accept,
Accept-after-Close, double-Close, concurrent-Close, and Addr-after-Close.
The first two tests fail on master with the exact errnos this patch
fixes (invalid argument / bad file descriptor); all five pass with
this change. Existing tests remain green under -race.
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.
Summary
SCTPListener.Closecurrently callssyscall.Shutdownandsyscall.Closeonln.fdwithout any synchronisation. ConcurrentAcceptSCTPandAddrcalls readln.fddirectly. This produces three observable bugs that are easy to hit in any program that callsClosewhile aServeloop is blocked inAccept:accept4()returnsEINVALorEBADFwhenCloseruns concurrently, instead of a sentinel callers can detect (cf.net.TCPListener, which has returnednet.ErrClosedsince Go 1.16). Real-world failure: downstream callers likego-diameterlog every graceful shutdown as"accept error: invalid argument"or"accept error: bad file descriptor".AcceptSCTPafterClosecallsaccept4on the now-deadfdinteger. If the OS has reassigned that fd betweenCloseandAccept,accept4either fails or, worse, returns an accepted fd from an unrelated socket.AcceptSCTPalways returns a non-nil*SCTPConn(wrappingfd=-1on the error path), violating the(net.Conn, error)API contract. Callers that defensively invoke methods on the returned conn hit anotherEBADFfromsetsockopt.The fix
The change mirrors the pattern this package already uses for
SCTPConn— same field shape, same atomic primitive, same close protocol:SCTPConn(existing)SCTPListener(this PR)_fd int32(sctp.go)_fd int32(sctp.go)c.fd()viaatomic.LoadInt32(&c._fd)ln.fd()viaatomic.LoadInt32(&ln._fd)atomic.SwapInt32(&c._fd, -1)(sctp_linux.go:157)atomic.SwapInt32(&ln._fd, -1)Specifically:
SCTPListener.fd→_fd int32, accessed viaatomic.LoadInt32/atomic.SwapInt32.Closeusesatomic.SwapInt32(-1)so only one caller observes the real fd; subsequent calls returnsyscall.EBADFwithout re-issuingsyscall.Closeon a possibly-reused fd number.AcceptSCTPandAddrtake an atomic snapshot of_fdand bail toErrListenerClosed/nilif the listener has been closed. Whenaccept4was already blocked inside the kernel whenCloseran, the post-call atomic re-check normalises the resultingEBADF/EINVALtoErrListenerClosed.AcceptSCTPnow returns(nil, err)on every error path.ErrListenerClosedis a new exported sentinel, defined witherrors.Newto stay compatible with the existinggo 1.12directive (nonet.ErrClosed/errors.Isusage). Callers compare with==.Test plan
sctp_listener_close_test.go: Close-during-Accept, Accept-after-Close, double-Close, 16-way concurrent Close, Addr-after-Closemaster: the first two tests fail with exactlyinvalid argument(EINVAL) andbad file descriptor(EBADF), with a non-nil*SCTPConn{_fd:-1}returned alongside the error — i.e., the test reproduces the downstreamgo-diameterfailure mode byte-for-bytego test -race -count=1What this PR does not touch
sctp_unsupported.go(the non-Linux stub still returnsErrUnsupported)go.mod(stillgo 1.12)SCTPConn.Close's pre-existingif fd > 0check (mildly off — should be>= 0since stdin is a valid fd — but unrelated to this race fix, happy to send as a separate PR)