[client] Fix the ICEBind races that wedge interface creation (#7377)

* [client] Add tests for the ICEBind open and close races

Running many embedded clients in one process intermittently wedges interface
creation. A goroutine dump taken from 50 clients shows ten of them parked for
seven minutes in Device.IpcSet, in closeBindLocked waiting on
device.net.stopping.Wait, holding device.net while every other device
goroutine queues behind it on Device.Up.

Open writes s.closed and Close reads it with no synchronisation, and Close
also closes s.closedChan without the mutex that Open swaps it under. Two
Closes can both pass the check and close the same channel, and a Close racing
an Open can mark the bind closed while a live channel and live receive
functions remain, after which every later Close takes its early return and
runs neither close(closedChan) nor StdNetBind.Close. The receive functions
never stop, so stopping.Wait never returns.

These tests do not fix that. The first pins the contract closeBindLocked
depends on and passes today. The other two fail under -race, reporting the
races at the three sites above, and pass again once closed and closedChan are
guarded consistently.

* [client] Release parked receivers so reopening a bind cannot stall

receiveRelayed held closedChanMu for the whole of its blocking select, so a
parked receiver kept the read lock indefinitely and Open could never take the
write lock it needs to install a fresh closedChan. wireguard-go reaches Open
from Device.IpcSet and Device.Up with device.net held, so the stall took the
device lock with it: interface creation never finished, every other device
goroutine queued behind Device.Up, and Engine.Start never returned.

Callers now copy the channel under a short read lock and select on the copy.
Copying alone would stand a new trap in the same place, because an Open that
follows an Open leaves the previous generation parked on a channel no later
Close can reach, so Open now closes the outgoing channel before swapping it.

closed and closedChan are also updated together under that mutex. Read and
written apart, Close could see a stale closed and skip both close(closedChan)
and StdNetBind.Close, leaving every receive function running and wedging
closeBindLocked on device.net.stopping.Wait, or two Close calls could pass the
check together and close the same channel twice.

TestICEBindOpenDoesNotBlockOnParkedReceiver fails without this change, without
needing the race detector. The other three cover the surrounding contract and
report the state races under -race.

* [client] Make the bind lifecycle transition atomic and tighten its tests

Review caught that the previous commit moved the torn transition rather than
removing it. Open published the new generation before calling StdNetBind.Open,
so an Open rejected because the bind was already open had already signalled the
outgoing generation, and a Close arriving in that window could mark the bind
closed while the same call went on to install live sockets. Every later Close
then returned early and never shut them down.

Open now calls StdNetBind.Open first, so a failure leaves the current
generation untouched, and both Open and Close hold the lock across the whole
transition. Ordering is safe: StdNetBind.Open reaches muUDPMux through
createReceiverFn, and no path takes muUDPMux before closedChanMu.

The tests were also weaker than they read. The stress test claimed to cover a
stale channel but only ever raced two Closes, and the concurrency test left
overlap to goroutine start order. Both now gate their goroutines on a common
start, the stress test races an Open against the Closes, and both assert the
surviving generation channel is actually closed. Waiting on receive functions
to be entered replaces part of the sleep in the reopen probe, and teardown
bounds its Close so a regression fails the assertion instead of hanging.

Two of the four now fail without the fix and no race detector, the stress test
by reproducing close of a closed channel at the Close early return.

* [client] Fail the reopen probe when its teardown does not complete

closeBounded swallowed its timeout and the cleanup discarded what
receiversStopped returned, so the bounds added in the previous commit only
stopped teardown hanging. A wedged Close or a parked receiver would have left
the test green with a leaked goroutine, which is the failure this test exists
to catch.

closeBounded now reports whether Close returned, and cleanup fails the test on
either bound.

client/iface/bind/ice_bind.go

@@ -57,10 +57,13 @@ type ICEBind struct {
 	endpoints   map[netip.Addr]net.Conn
 	endpointsMu sync.Mutex
 	recvChan    chan recvMessage
-	// every time when Close() is called (i.e. BindUpdate()) we need to close exit from the receiveRelayed and create a
-	// new closed channel. With the closedChanMu we can safely close the channel and create a new one
+	// Close() (i.e. BindUpdate()) closes closedChan to release receiveRelayed,
+	// and the following Open() installs a fresh one. closedChanMu guards both
+	// closedChan and closed: readers only ever hold it long enough to copy the
+	// channel, never across a blocking receive, so Open cannot be starved by a
+	// parked receiver.
 	closedChan       chan struct{}
-	closedChanMu     sync.RWMutex // protect the closeChan recreation from reading from it.
+	closedChanMu     sync.RWMutex
 	closed           bool
 	activityRecorder *ActivityRecorder
 
@@ -92,24 +95,41 @@ func NewICEBind(transportNet transport.Net, address wgaddr.Address, mtu uint16)
 }
 
 func (s *ICEBind) Open(uport uint16) ([]wgConn.ReceiveFunc, uint16, error) {
-	s.closed = false
 	s.closedChanMu.Lock()
-	s.closedChan = make(chan struct{})
-	s.closedChanMu.Unlock()
+	defer s.closedChanMu.Unlock()
+
+	// Open the underlying bind before touching any state, so a failure leaves
+	// the current generation exactly as it was. Publishing the new generation
+	// first would strand it: StdNetBind rejects an Open while it is already
+	// open, and a Close arriving in that window would mark the bind closed
+	// while this call went on to install live sockets, after which every later
+	// Close returns early and never shuts them down.
 	fns, port, err := s.StdNetBind.Open(uport)
 	if err != nil {
 		return nil, 0, err
 	}
+
+	// Release whoever is parked on the outgoing generation before replacing it.
+	// An Open that follows an Open rather than a Close would otherwise leave
+	// them waiting on a channel no later Close can reach.
+	if !s.closed {
+		close(s.closedChan)
+	}
+	s.closed = false
+	s.closedChan = make(chan struct{})
+
 	fns = append(fns, s.receiveRelayed)
 	return fns, port, nil
 }
 
 func (s *ICEBind) Close() error {
+	s.closedChanMu.Lock()
+	defer s.closedChanMu.Unlock()
+
 	if s.closed {
 		return nil
 	}
 	s.closed = true
-
 	close(s.closedChan)
 
 	s.muUDPMux.Lock()
@@ -121,6 +141,15 @@ func (s *ICEBind) Close() error {
 	return s.StdNetBind.Close()
 }
 
+// currentClosedChan copies the channel that signals the current Open
+// generation is closing. Callers select on the copy so the lock is never held
+// across a blocking receive, which would otherwise stall the next Open.
+func (s *ICEBind) currentClosedChan() chan struct{} {
+	s.closedChanMu.RLock()
+	defer s.closedChanMu.RUnlock()
+	return s.closedChan
+}
+
 func (s *ICEBind) ActivityRecorder() *ActivityRecorder {
 	return s.activityRecorder
 }
@@ -150,8 +179,10 @@ func (b *ICEBind) RemoveEndpoint(fakeIP netip.Addr) {
 }
 
 func (b *ICEBind) ReceiveFromEndpoint(ctx context.Context, ep *Endpoint, buf []byte) {
+	closedChan := b.currentClosedChan()
+
 	select {
-	case <-b.closedChan:
+	case <-closedChan:
 		return
 	case <-ctx.Done():
 		return
@@ -333,11 +364,10 @@ func (s *ICEBind) parseSTUNMessage(raw []byte) (*stun.Message, error) {
 // receiveRelayed is a receive function that is used to receive packets from the relayed connection and forward to the
 // WireGuard. Critical part is do not block if the Closed() has been called.
 func (c *ICEBind) receiveRelayed(buffs [][]byte, sizes []int, eps []wgConn.Endpoint) (int, error) {
-	c.closedChanMu.RLock()
-	defer c.closedChanMu.RUnlock()
+	closedChan := c.currentClosedChan()
 
 	select {
-	case <-c.closedChan:
+	case <-closedChan:
 		return 0, net.ErrClosed
 	case msg, ok := <-c.recvChan:
 		if !ok {

client/iface/bind/ice_bind_close_test.go

@@ -0,0 +1,261 @@
+package bind
+
+import (
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	wgConn "golang.zx2c4.com/wireguard/conn"
+)
+
+// startReceivers runs every receive function the way wireguard-go's device
+// does: one goroutine per function, all tracked by a single WaitGroup. After
+// calling Bind.Close, closeBindLocked waits on exactly that WaitGroup while
+// holding device.net, so a receive function that never returns wedges the
+// device and every goroutine that needs the same lock.
+func startReceivers(fns []wgConn.ReceiveFunc) *sync.WaitGroup {
+	wg, _ := startReceiversEntered(fns)
+	return wg
+}
+
+// startReceiversEntered also returns a channel closed once every receive
+// function has been called at least once. A receive function that has been
+// entered is either inside its blocking receive or about to be, which is a
+// stronger signal to synchronise on than a bare sleep.
+func startReceiversEntered(fns []wgConn.ReceiveFunc) (*sync.WaitGroup, <-chan struct{}) {
+	var wg sync.WaitGroup
+	var entered sync.WaitGroup
+	wg.Add(len(fns))
+	entered.Add(len(fns))
+
+	for i := range fns {
+		go func(fn wgConn.ReceiveFunc) {
+			defer wg.Done()
+			buffs := [][]byte{make([]byte, 1500)}
+			sizes := make([]int, 1)
+			eps := make([]wgConn.Endpoint, 1)
+			first := true
+			for {
+				if first {
+					entered.Done()
+					first = false
+				}
+				if _, err := fn(buffs, sizes, eps); err != nil {
+					return
+				}
+			}
+		}(fns[i])
+	}
+
+	allEntered := make(chan struct{})
+	go func() {
+		entered.Wait()
+		close(allEntered)
+	}()
+	return &wg, allEntered
+}
+
+// closeBounded runs Close off the caller's goroutine so a regression that
+// wedges it fails the test instead of hanging teardown, and reports whether it
+// returned in time.
+func closeBounded(iceBind *ICEBind, timeout time.Duration) bool {
+	done := make(chan struct{})
+	go func() {
+		_ = iceBind.Close()
+		close(done)
+	}()
+	select {
+	case <-done:
+		return true
+	case <-time.After(timeout):
+		return false
+	}
+}
+
+// isClosed reports whether the bind's current generation channel is closed.
+func isClosed(iceBind *ICEBind) bool {
+	iceBind.closedChanMu.RLock()
+	ch := iceBind.closedChan
+	iceBind.closedChanMu.RUnlock()
+	select {
+	case <-ch:
+		return true
+	default:
+		return false
+	}
+}
+
+// receiversStopped reports whether every receive function returned before the
+// timeout, mirroring device.net.stopping.Wait() inside closeBindLocked.
+func receiversStopped(wg *sync.WaitGroup, timeout time.Duration) bool {
+	done := make(chan struct{})
+	go func() {
+		wg.Wait()
+		close(done)
+	}()
+	select {
+	case <-done:
+		return true
+	case <-time.After(timeout):
+		return false
+	}
+}
+
+// TestICEBindCloseReleasesReceivers covers the contract closeBindLocked relies
+// on: once Close returns, every receive function handed out by Open must stop.
+func TestICEBindCloseReleasesReceivers(t *testing.T) {
+	iceBind := setupICEBind(t)
+
+	fns, _, err := iceBind.Open(0)
+	require.NoError(t, err, "opening the bind must succeed")
+
+	wg := startReceivers(fns)
+	require.NoError(t, iceBind.Close())
+
+	require.True(t, receiversStopped(wg, 5*time.Second),
+		"every receive function must return once Close returns")
+}
+
+// TestICEBindOpenDoesNotBlockOnParkedReceiver reproduces the deadlock that
+// wedges interface creation.
+//
+// receiveRelayed used to hold closedChanMu for the whole of its blocking
+// select, so a parked receiver kept the read lock indefinitely. Open takes the
+// same mutex for writing to install a fresh closedChan, so it could never
+// acquire it while a receiver was parked. wireguard-go reaches Open from
+// Device.IpcSet and Device.Up with device.net held, so the stall takes the
+// device's lock with it and every other device goroutine queues behind it.
+//
+// Failing here means Open never returned.
+func TestICEBindOpenDoesNotBlockOnParkedReceiver(t *testing.T) {
+	iceBind := setupICEBind(t)
+
+	fns, _, err := iceBind.Open(0)
+	require.NoError(t, err, "the first Open must succeed")
+
+	wg, entered := startReceiversEntered(fns)
+	t.Cleanup(func() {
+		// Both bounded: a regression that wedges Close must surface as the
+		// assertion below, not as a hung teardown.
+		if !closeBounded(iceBind, 5*time.Second) {
+			t.Error("Close did not return during teardown; the bind lifecycle is wedged even though the assertion above passed")
+		}
+		if !receiversStopped(wg, 5*time.Second) {
+			t.Error("receive functions were still running after teardown Close, which is what closeBindLocked blocks on")
+		}
+	})
+
+	select {
+	case <-entered:
+	case <-time.After(5 * time.Second):
+		t.Fatal("receive functions never started")
+	}
+	// Entered is not yet parked, so still allow the blocking receive to be
+	// reached. Parking takes microseconds; this margin is six orders larger.
+	time.Sleep(500 * time.Millisecond)
+
+	reopened := make(chan struct{})
+	go func() {
+		// The error is irrelevant; StdNetBind rejects a second Open. What
+		// matters is that the call returns at all.
+		_, _, _ = iceBind.Open(0)
+		close(reopened)
+	}()
+
+	select {
+	case <-reopened:
+	case <-time.After(10 * time.Second):
+		t.Fatal("Open blocked while a receive function was parked; wireguard-go makes this call with device.net held, which is what stalls interface creation")
+	}
+}
+
+// TestICEBindConcurrentOpenClose exercises Open and Close from separate
+// goroutines, the way Device.IpcSet and Device.Up reach the bind, and is meant
+// to be run under -race.
+//
+// closed and closedChan must be updated together. When they were not, Close
+// could observe a stale closed and either skip close(closedChan) and
+// StdNetBind.Close entirely, leaving the receive functions running, or race a
+// second Close and close the same channel twice.
+//
+// Receive functions are deliberately not started here: this test is about the
+// shared state, and parking them would turn a race report into a hang.
+func TestICEBindConcurrentOpenClose(t *testing.T) {
+	iceBind := setupICEBind(t)
+
+	var wg sync.WaitGroup
+	wg.Add(2)
+
+	// Release both loops together so the calls genuinely interleave rather
+	// than depending on goroutine start order.
+	start := make(chan struct{})
+
+	go func() {
+		defer wg.Done()
+		<-start
+		for i := 0; i < 200; i++ {
+			_, _, _ = iceBind.Open(0)
+		}
+	}()
+	go func() {
+		defer wg.Done()
+		<-start
+		for i := 0; i < 200; i++ {
+			_ = iceBind.Close()
+		}
+	}()
+
+	close(start)
+	wg.Wait()
+	require.NoError(t, iceBind.Close())
+	require.True(t, isClosed(iceBind), "the final Close must leave the current generation channel closed")
+}
+
+// TestICEBindCloseReleasesReceiversUnderConcurrentClose runs full Open, receive,
+// Close cycles with a second Close and an Open racing the first Close. Any
+// iteration where the receive functions outlive Close, or where the surviving
+// generation channel is left open, is the state closeBindLocked deadlocks on.
+func TestICEBindCloseReleasesReceiversUnderConcurrentClose(t *testing.T) {
+	if testing.Short() {
+		t.Skip("stress test")
+	}
+
+	for i := 0; i < 200; i++ {
+		iceBind := setupICEBind(t)
+
+		fns, _, err := iceBind.Open(0)
+		require.NoError(t, err, "iteration %d: opening the bind must succeed", i)
+		wg := startReceivers(fns)
+
+		start := make(chan struct{})
+		var racers sync.WaitGroup
+		racers.Add(3)
+		for c := 0; c < 2; c++ {
+			go func() {
+				defer racers.Done()
+				<-start
+				_ = iceBind.Close()
+			}()
+		}
+		// An Open overlapping the Closes is what produces a generation whose
+		// channel outlives the flag saying the bind is closed.
+		go func() {
+			defer racers.Done()
+			<-start
+			_, _, _ = iceBind.Open(0)
+		}()
+		close(start)
+		racers.Wait()
+
+		// Settle on a closed bind whatever order the racers landed in.
+		_ = iceBind.Close()
+
+		if !receiversStopped(wg, 5*time.Second) {
+			t.Fatalf("iteration %d: receive functions still running after Close; closeBindLocked would block here on device.net.stopping.Wait", i)
+		}
+		if !isClosed(iceBind) {
+			t.Fatalf("iteration %d: Close returned with the current generation channel still open, so nothing will ever release its receivers", i)
+		}
+	}
+}