Fix SCTPListener.Close race with concurrent Accept

sctp.go

@@ -18,6 +18,7 @@ package sctp
 import (
 	"bytes"
 	"encoding/binary"
+	"errors"
 	"fmt"
 	"net"
 	"strconv"
@@ -29,6 +30,11 @@ import (
 	"unsafe"
 )
 
+// ErrListenerClosed is returned by SCTPListener.Accept and AcceptSCTP when the
+// listener has been closed, either before the call or while it was blocked in
+// the kernel. Compare with == (errors.Is is not available on Go 1.12).
+var ErrListenerClosed = errors.New("sctp: use of closed listener")
+
 const (
 	SOL_SCTP = 132
 
@@ -736,13 +742,23 @@ func (c *SCTPConn) SetWriteDeadline(t time.Time) error {
 }
 
 type SCTPListener struct {
-	fd                  int
+	_fd                 int32 // -1 once Close has run; read/written via sync/atomic
 	m                   sync.Mutex
 	notificationHandler NotificationHandler
 }
 
+// fd returns the current listener file descriptor or -1 if the listener has
+// been closed. Reads are atomic so callers can detect concurrent Close.
+func (ln *SCTPListener) fd() int {
+	return int(atomic.LoadInt32(&ln._fd))
+}
+
 func (ln *SCTPListener) Addr() net.Addr {
-	laddr, err := sctpGetAddrs(ln.fd, 0, SCTP_GET_LOCAL_ADDRS)
+	fd := ln.fd()
+	if fd < 0 {
+		return nil
+	}
+	laddr, err := sctpGetAddrs(fd, 0, SCTP_GET_LOCAL_ADDRS)
 	if err != nil {
 		return nil
 	}

sctp_linux.go

@@ -248,7 +248,7 @@ func listenSCTPExtConfig(network string, laddr *SCTPAddr, options InitMsg, contr
 		return nil, err
 	}
 	return &SCTPListener{
-			fd:                  sock,
+			_fd:                 int32(sock),
 			notificationHandler: notificationHandler,
 		},
 		nil
@@ -271,33 +271,60 @@ func FileListener(file *os.File) (*SCTPListener, error) {
 	}
 
 	return &SCTPListener{
-		fd:                  int(r1),
+		_fd:                 int32(r1),
 		notificationHandler: nil,
 	}, nil
 }
 
 // AcceptSCTP waits for and returns the next SCTP connection to the listener.
+//
+// If the listener has been closed (either before AcceptSCTP was called or
+// concurrently while it was blocked in accept4), AcceptSCTP returns
+// (nil, ErrListenerClosed) instead of leaking the bare syscall.EBADF / EINVAL
+// returned by the kernel on a torn-down fd.
 func (ln *SCTPListener) AcceptSCTP() (*SCTPConn, error) {
-	fd, _, err := syscall.Accept4(ln.fd, 0)
-	return NewSCTPConn(fd, ln.notificationHandler), err
+	fd := atomic.LoadInt32(&ln._fd)
+	if fd < 0 {
+		return nil, ErrListenerClosed
+	}
+	nfd, _, err := syscall.Accept4(int(fd), 0)
+	if err != nil {
+		// If Close ran concurrently, surface a clean ErrListenerClosed.
+		if atomic.LoadInt32(&ln._fd) < 0 {
+			return nil, ErrListenerClosed
+		}
+		return nil, err
+	}
+	return NewSCTPConn(nfd, ln.notificationHandler), nil
 }
 
 // Accept waits for and returns the next connection connection to the listener.
 func (ln *SCTPListener) Accept() (net.Conn, error) {
-	return ln.AcceptSCTP()
+	c, err := ln.AcceptSCTP()
+	if err != nil {
+		return nil, err
+	}
+	return c, nil
 }
 
+// Close releases the listener fd. The first call returns the result of the
+// underlying syscall.Close; subsequent calls return syscall.EBADF without
+// touching the fd, so a reused fd number cannot be closed by mistake.
 func (ln *SCTPListener) Close() error {
-	syscall.Shutdown(ln.fd, syscall.SHUT_RDWR)
-	return syscall.Close(ln.fd)
+	fd := atomic.SwapInt32(&ln._fd, -1)
+	if fd < 0 {
+		return syscall.EBADF
+	}
+	syscall.Shutdown(int(fd), syscall.SHUT_RDWR)
+	return syscall.Close(int(fd))
 }
 
 func (ln *SCTPListener) SyscallConn() (syscall.RawConn, error) {
-	fd := ln.fd
+	fd := ln.fd()
 	if fd < 0 {
 		return nil, syscall.EINVAL
 	}
-	return &rawConn{sockfd: int(fd)}, nil
+	return &rawConn{sockfd: fd}, nil
 }
 
 // DialSCTP - bind socket to laddr (if given) and connect to raddr

sctp_listener_close_test.go

@@ -0,0 +1,165 @@
+//go:build linux && !386
+// +build linux,!386
+
+// Copyright 2024 Wataru Ishida. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package sctp
+
+import (
+	"net"
+	"sync"
+	"syscall"
+	"testing"
+	"time"
+)
+
+// newLoopbackListener returns a fresh SCTPListener on an ephemeral 127.0.0.1 port.
+func newLoopbackListener(t *testing.T) *SCTPListener {
+	t.Helper()
+	addr := &SCTPAddr{IPAddrs: []net.IPAddr{{IP: net.IPv4(127, 0, 0, 1)}}}
+	ln, err := ListenSCTP("sctp", addr)
+	if err != nil {
+		t.Fatalf("ListenSCTP failed: %v", err)
+	}
+	return ln
+}
+
+// TestListenerCloseUnblocksAccept verifies that closing a listener while
+// AcceptSCTP is blocked unblocks Accept and returns ErrListenerClosed (not a
+// bare syscall.EBADF / syscall.EINVAL leaked from accept4).
+func TestListenerCloseUnblocksAccept(t *testing.T) {
+	ln := newLoopbackListener(t)
+
+	type acceptResult struct {
+		conn *SCTPConn
+		err  error
+	}
+	resCh := make(chan acceptResult, 1)
+	go func() {
+		c, err := ln.AcceptSCTP()
+		resCh <- acceptResult{c, err}
+	}()
+
+	// Give the goroutine a moment to block inside accept4.
+	time.Sleep(50 * time.Millisecond)
+
+	if err := ln.Close(); err != nil {
+		t.Fatalf("first Close returned %v, want nil", err)
+	}
+
+	select {
+	case res := <-resCh:
+		if res.err != ErrListenerClosed {
+			t.Errorf("AcceptSCTP after Close returned err=%v, want ErrListenerClosed", res.err)
+		}
+		if res.conn != nil {
+			t.Errorf("AcceptSCTP after Close returned non-nil conn=%+v, want nil", res.conn)
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatal("AcceptSCTP did not return within 2s after Close")
+	}
+}
+
+// TestAcceptAfterCloseReturnsErrClosed verifies that calling AcceptSCTP on an
+// already-closed listener returns ErrListenerClosed with a nil conn, instead
+// of invoking accept4 on a stale (possibly reused) fd.
+func TestAcceptAfterCloseReturnsErrClosed(t *testing.T) {
+	ln := newLoopbackListener(t)
+	if err := ln.Close(); err != nil {
+		t.Fatalf("Close returned %v, want nil", err)
+	}
+
+	conn, err := ln.AcceptSCTP()
+	if err != ErrListenerClosed {
+		t.Errorf("AcceptSCTP on closed listener err=%v, want ErrListenerClosed", err)
+	}
+	if conn != nil {
+		t.Errorf("AcceptSCTP on closed listener conn=%+v, want nil", conn)
+	}
+}
+
+// TestDoubleCloseSafe verifies that calling Close twice does not attempt to
+// close the underlying fd twice (which could close a different, reused fd).
+// The first call returns nil; the second returns syscall.EBADF.
+func TestDoubleCloseSafe(t *testing.T) {
+	ln := newLoopbackListener(t)
+
+	if err := ln.Close(); err != nil {
+		t.Fatalf("first Close returned %v, want nil", err)
+	}
+
+	if err := ln.Close(); err != syscall.EBADF {
+		t.Errorf("second Close returned %v, want syscall.EBADF", err)
+	}
+}
+
+// TestConcurrentCloseSingleCloser verifies that many concurrent Close calls
+// result in exactly one successful close (returns nil) and the rest return
+// syscall.EBADF — guarding against the OS reassigning the fd between calls.
+func TestConcurrentCloseSingleCloser(t *testing.T) {
+	ln := newLoopbackListener(t)
+
+	const n = 16
+	var (
+		wg        sync.WaitGroup
+		mu        sync.Mutex
+		successes int
+		others    []error
+	)
+	wg.Add(n)
+	for i := 0; i < n; i++ {
+		go func() {
+			defer wg.Done()
+			err := ln.Close()
+			mu.Lock()
+			defer mu.Unlock()
+			if err == nil {
+				successes++
+			} else {
+				others = append(others, err)
+			}
+		}()
+	}
+	wg.Wait()
+
+	if successes != 1 {
+		t.Errorf("got %d successful Close calls, want exactly 1 (others=%v)", successes, others)
+	}
+	for _, err := range others {
+		if err != syscall.EBADF {
+			t.Errorf("concurrent Close returned %v, want syscall.EBADF", err)
+		}
+	}
+}
+
+// TestAddrAfterCloseReturnsNil verifies that Addr() on a closed listener does
+// not call sctpGetAddrs on a stale fd number — it should return nil because
+// our fd accessor reports -1.
+func TestAddrAfterCloseReturnsNil(t *testing.T) {
+	ln := newLoopbackListener(t)
+
+	if a := ln.Addr(); a == nil {
+		t.Fatalf("Addr() before Close returned nil, want a real address")
+	}
+
+	if err := ln.Close(); err != nil {
+		t.Fatalf("Close returned %v, want nil", err)
+	}
+
+	if a := ln.Addr(); a != nil {
+		t.Errorf("Addr() after Close returned %v, want nil", a)
+	}
+}