Fix possible race condition between tcp and udp servers when stopping or crashing

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

.github/workflows/ci.yml

@@ -28,10 +28,6 @@ on:
       - go.mod
       - go.sum
 
-concurrency:
-  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
-  cancel-in-progress: true
-
 jobs:
   verify:
     runs-on: ubuntu-latest
@@ -41,7 +37,7 @@ jobs:
     env:
       DOCKER_BUILDKIT: "1"
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@v6
 
       - uses: reviewdog/action-misspell@v1
         with:
@@ -79,7 +75,7 @@ jobs:
       actions: read
       contents: read
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@v6
 
       - uses: actions/setup-go@v6
         with:
@@ -105,7 +101,7 @@ jobs:
     runs-on: ubuntu-latest
     environment: secrets
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@v6
 
       - run: docker build -t qmcgaw/gluetun .
 
@@ -157,7 +153,7 @@ jobs:
       contents: read
       security-events: write
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@v6
       - uses: actions/setup-go@v6
         with:
           go-version-file: go.mod
@@ -183,7 +179,7 @@ jobs:
     runs-on: ubuntu-latest
     environment: secrets
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@v6
 
       # extract metadata (tags, labels) for Docker
       # https://github.com/docker/metadata-action

.github/workflows/labels.yml

@@ -11,7 +11,7 @@ jobs:
       issues: write
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@v6
       - uses: crazy-max/ghaction-github-labeler@v6
         with:
           yaml-file: .github/labels.yml

.github/workflows/markdown.yml

@@ -11,10 +11,6 @@ on:
       - "**.md"
       - .github/workflows/markdown.yml
 
-concurrency:
-  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
-  cancel-in-progress: true
-
 jobs:
   markdown:
     runs-on: ubuntu-latest
@@ -23,7 +19,7 @@ jobs:
       contents: read
     environment: secrets
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@v6
 
       - uses: DavidAnson/markdownlint-cli2-action@v22
         with:

.golangci.yml

@@ -12,10 +12,6 @@ formatters:
       - builtin$
       - examples$
 
-run:
-  build-tags:
-    - integration
-
 linters:
   settings:
     misspell:

internal/command/start.go

@@ -9,9 +9,8 @@ import (
 )
 
 // Start launches a command and streams stdout and stderr to channels.
-// stdoutLines and stderrLines channels will be closed when there is no more
-// output to read, in order for the caller to catch all lines even after the
-// command has finished. The waitError channel returned will never be closed.
+// All the channels returned are ready only and won't be closed
+// if the command fails later.
 func (c *Cmder) Start(cmd *exec.Cmd) (
 	stdoutLines, stderrLines <-chan string,
 	waitError <-chan error, startErr error,
@@ -39,7 +38,6 @@ func start(cmd execCmd) (stdoutLines, stderrLines <-chan string,
 	if err != nil {
 		_ = stdout.Close()
 		<-stdoutDone
-		close(stdoutLinesCh)
 		return nil, nil, nil, err
 	}
 	go streamToChannel(stderrReady, stderrDone, stderr, stderrLinesCh)
@@ -47,11 +45,9 @@ func start(cmd execCmd) (stdoutLines, stderrLines <-chan string,
 	err = cmd.Start()
 	if err != nil {
 		_ = stdout.Close()
-		<-stdoutDone
-		close(stdoutLinesCh)
 		_ = stderr.Close()
+		<-stdoutDone
 		<-stderrDone
-		close(stderrLinesCh)
 		return nil, nil, nil, err
 	}
 
@@ -59,10 +55,8 @@ func start(cmd execCmd) (stdoutLines, stderrLines <-chan string,
 	go func() {
 		err := cmd.Wait()
 		<-stdoutDone
-		close(stdoutLinesCh)
-		_ = stdout.Close()
 		<-stderrDone
-		close(stderrLinesCh)
+		_ = stdout.Close()
 		_ = stderr.Close()
 		waitErrorCh <- err
 	}()

internal/command/start_test.go

@@ -89,48 +89,30 @@ func Test_start(t *testing.T) {
 
 			require.NoError(t, err)
 
-			collectAndCheckChannels(t, stdoutLines, stderrLines, waitError,
-				testCase.stdout, testCase.stderr, testCase.waitErr)
+			var stdoutIndex, stderrIndex int
+
+			done := false
+			for !done {
+				select {
+				case line := <-stdoutLines:
+					assert.Equal(t, testCase.stdout[stdoutIndex], line)
+					stdoutIndex++
+				case line := <-stderrLines:
+					assert.Equal(t, testCase.stderr[stderrIndex], line)
+					stderrIndex++
+				case err := <-waitError:
+					if testCase.waitErr != nil {
+						require.Error(t, err)
+						assert.Equal(t, testCase.waitErr.Error(), err.Error())
+					} else {
+						assert.NoError(t, err)
+					}
+					done = true
+				}
+			}
+
+			assert.Equal(t, len(testCase.stdout), stdoutIndex)
+			assert.Equal(t, len(testCase.stderr), stderrIndex)
 		})
 	}
 }
-
-func collectAndCheckChannels(t *testing.T, stdoutLines, stderrLines <-chan string,
-	waitError <-chan error, expectedStdout, expectedStderr []string, expectedWaitErr error,
-) {
-	t.Helper()
-
-	stdoutIndex := 0
-	stderrIndex := 0
-
-	done := false
-	for !done {
-		select {
-		case line, ok := <-stdoutLines:
-			if !ok {
-				stdoutLines = nil
-				continue
-			}
-			assert.Equal(t, expectedStdout[stdoutIndex], line)
-			stdoutIndex++
-		case line, ok := <-stderrLines:
-			if !ok {
-				stderrLines = nil
-				continue
-			}
-			assert.Equal(t, expectedStderr[stderrIndex], line)
-			stderrIndex++
-		case err := <-waitError:
-			if expectedWaitErr != nil {
-				require.Error(t, err)
-				assert.Equal(t, expectedWaitErr.Error(), err.Error())
-			} else {
-				assert.NoError(t, err)
-			}
-			done = true
-		}
-	}
-
-	assert.Equal(t, len(expectedStdout), stdoutIndex)
-	assert.Equal(t, len(expectedStderr), stderrIndex)
-}

internal/command/startnlog.go

@@ -18,39 +18,31 @@ func (c *Cmder) RunAndLog(ctx context.Context, command string, logger Logger) (e
 		return err
 	}
 
+	streamCtx, streamCancel := context.WithCancel(context.Background())
 	streamDone := make(chan struct{})
-	go streamLines(streamDone, logger, stdout, stderr)
+	go streamLines(streamCtx, streamDone, logger, stdout, stderr)
 
 	err = <-waitError
+	streamCancel()
 	<-streamDone
 	return err
 }
 
-func streamLines(done chan<- struct{}, logger Logger,
-	stdout, stderr <-chan string,
+func streamLines(ctx context.Context, done chan<- struct{},
+	logger Logger, stdout, stderr <-chan string,
 ) {
 	defer close(done)
 
+	var line string
+
 	for {
 		select {
-		case line, ok := <-stdout:
-			if ok {
-				logger.Info(line)
-				break
-			}
-			if stderr == nil {
-				return
-			}
-			stdout = nil
-		case line, ok := <-stderr:
-			if ok {
-				logger.Error(line)
-				break
-			}
-			if stdout == nil {
-				return
-			}
-			stderr = nil
+		case <-ctx.Done():
+			return
+		case line = <-stdout:
+			logger.Info(line)
+		case line = <-stderr:
+			logger.Error(line)
 		}
 	}
 }

internal/openvpn/run.go

@@ -29,16 +29,19 @@ func (r *Runner) Run(ctx context.Context, errCh chan<- error, ready chan<- struc
 		return
 	}
 
+	streamCtx, streamCancel := context.WithCancel(context.Background())
 	streamDone := make(chan struct{})
-	go streamLines(streamDone, r.logger,
+	go streamLines(streamCtx, streamDone, r.logger,
 		stdoutLines, stderrLines, ready)
 
 	select {
 	case <-ctx.Done():
 		<-waitError
+		streamCancel()
 		<-streamDone
 		errCh <- ctx.Err()
 	case err := <-waitError:
+		streamCancel()
 		<-streamDone
 		errCh <- err
 	}

internal/openvpn/stream.go

@@ -1,37 +1,26 @@
 package openvpn
 
 import (
+	"context"
 	"strings"
 )
 
-func streamLines(done chan<- struct{},
+func streamLines(ctx context.Context, done chan<- struct{},
 	logger Logger, stdout, stderr <-chan string,
 	tunnelReady chan<- struct{},
 ) {
 	defer close(done)
 
+	var line string
+
 	for {
-		var line string
-		var ok bool
 		errLine := false
 		select {
-		case line, ok = <-stdout:
-			if ok {
-				break
-			}
-			if stderr == nil {
-				return
-			}
-			stdout = nil
-		case line, ok = <-stderr:
-			if ok {
-				errLine = true
-				break
-			}
-			if stdout == nil {
-				return
-			}
-			stderr = nil
+		case <-ctx.Done():
+			return
+		case line = <-stdout:
+		case line = <-stderr:
+			errLine = true
 		}
 		line, level := processLogLine(line)
 		if line == "" {

internal/socks5/server.go

@@ -97,7 +97,6 @@ func (s *server) runServer(ready chan<- struct{},
 				return
 			}
 			wg.Go(func() {
-				connection := connection // capture loop variable
 				socksConn := &socksConn{
 					dialer:     dialer,
 					username:   s.username,
@@ -116,7 +115,6 @@ func (s *server) runServer(ready chan<- struct{},
 
 	select {
 	case <-stop:
-		s.listening.Store(false)
 		var errs []error
 		err := s.tcpListener.Close()
 		if err != nil {

internal/socks5/server_integration_test.go

@@ -1,112 +0,0 @@
-//go:build integration
-
-package socks5
-
-import (
-	"math/rand/v2"
-	"net"
-	"testing"
-	"time"
-
-	"github.com/miekg/dns"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-func Test_Server_UDPResolution(t *testing.T) {
-	t.Parallel()
-	ctx := t.Context()
-
-	server := newServer(Settings{
-		Address: "127.0.0.1:0",
-		Logger:  noopLogger{},
-	})
-	runErr, err := server.Start(ctx)
-	require.NoError(t, err, "starting SOCKS5 server")
-
-	const timeout = 3 * time.Second
-
-	// Connect to the SOCKS5 server via TCP to negotiate UDP associate
-	dialer := &net.Dialer{Timeout: timeout}
-	tcpConn, err := dialer.DialContext(ctx, "tcp", server.listeningAddress().String())
-	require.NoError(t, err, "tcp connecting to SOCKS5 server")
-	t.Cleanup(func() { tcpConn.Close() })
-
-	negotiateSOCKS5(t, tcpConn, "", "")
-
-	// UDP Associate Command: [VERSION (5), CMD (3 = UDP ASSOC), RSV (0), ATYP (1 = IPv4), ADDR (0.0.0.0), PORT (0)]
-	_, err = tcpConn.Write([]byte{5, 3, 0, 1, 0, 0, 0, 0, 0, 0})
-	require.NoError(t, err, "sending UDP ASSOC request")
-
-	relayAddressString, err := readSOCKS5ResponseAddress(t, tcpConn)
-	require.NoError(t, err, "reading UDP ASSOC reply")
-	relayAddress, err := net.ResolveUDPAddr("udp", relayAddressString)
-	require.NoError(t, err, "resolving udp relay address")
-
-	// Dial the relay using IPv4 so source IP family matches the control connection.
-	udpConn, err := net.DialUDP("udp4", nil, relayAddress)
-	require.NoError(t, err, "dialing UDP relay")
-	t.Cleanup(func() { _ = udpConn.Close() })
-
-	queryID := uint16(rand.Uint32()) //nolint:gosec
-	dnsRequest := &dns.Msg{
-		MsgHdr: dns.MsgHdr{
-			Id:               queryID,
-			RecursionDesired: true,
-		},
-		Question: []dns.Question{{
-			Name:   dns.Fqdn("github.com"),
-			Qtype:  dns.TypeA,
-			Qclass: dns.ClassINET,
-		}},
-	}
-	dnsQuery, err := dnsRequest.Pack()
-	require.NoError(t, err)
-
-	// Encapsulate DNS payload into SOCKS5 UDP Request Header
-	// [RSV (0,0), FRAG (0), ATYP (1 = IPv4), DST.ADDR (1.1.1.1), DST.PORT (53)]
-	packet := append([]byte{0, 0, 0, 1, 1, 1, 1, 1, 0, 53}, dnsQuery...)
-
-	// Send encapsulated packet to the proxy's UDP relay address
-	_, err = udpConn.Write(packet)
-	require.NoError(t, err, "sending UDP packet to relay")
-
-	// Read response from the proxy relay
-	err = udpConn.SetReadDeadline(time.Now().Add(timeout))
-	require.NoError(t, err, "setting read deadline on UDP connection")
-	buffer := make([]byte, 2048)
-	n, err := udpConn.Read(buffer)
-	require.NoError(t, err, "receiving UDP response from relay")
-	const minimumHeaderSize = 10
-	require.GreaterOrEqual(t, n, minimumHeaderSize, "received UDP packet too short to contain valid SOCKS5 header")
-
-	// Verify header layout and slice out the raw DNS response
-	// Header format: RSV(2) FRAG(1) ATYP(1) DST.ADDR(variable) DST.PORT(2)
-	atyp := buffer[3]
-	var headerSize int
-	switch atyp {
-	case 1: // IPv4
-		headerSize = 10
-	case 3: // Domain name
-		headerSize = 4 + 1 + int(buffer[4]) + 2
-	case 4: // IPv6
-		headerSize = 22
-	default:
-		t.Fatalf("Unknown ATYP in SOCKS5 UDP header: %d", atyp)
-	}
-
-	dnsResponse := new(dns.Msg)
-	err = dnsResponse.Unpack(buffer[headerSize:n])
-	require.NoError(t, err, "unpacking DNS response from SOCKS5 UDP packet")
-
-	assert.Equal(t, queryID, dnsResponse.Id, "DNS response ID should match query ID")
-
-	select {
-	case err := <-runErr:
-		require.NoError(t, err, "SOCKS5 server run error")
-	default:
-	}
-
-	err = server.Stop()
-	require.NoError(t, err, "stopping SOCKS5 server")
-}

internal/socks5/socks5.go

@@ -256,10 +256,10 @@ func udpAssociateExpectedClientEndpoint(request request) (expectedAddrPort netip
 		}
 		return netip.AddrPortFrom(netip.Addr{}, request.port), nil
 	case domainName:
-		// For UDP associate, client endpoint matching is based on observed UDP source
-		// address/port. A hostname is not directly matchable at this stage, so we
-		// ignore the domain name request destination entirely.
-		return netip.AddrPortFrom(netip.Addr{}, request.port), nil
+		if request.destination != "" || request.port != 0 {
+			return netip.AddrPort{}, fmt.Errorf("domain name is not supported for UDP associate destination")
+		}
+		return netip.AddrPort{}, nil
 	default:
 		return netip.AddrPort{}, fmt.Errorf("address type %d is not supported", request.addressType)
 	}

internal/socks5/socks5_test.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"io"
 	"net"
-	"net/netip"
 	"strconv"
 	"strings"
 	"testing"
@@ -704,70 +703,6 @@ func Test_decodeRequest(t *testing.T) {
 	}
 }
 
-func Test_udpAssociateExpectedClientEndpoint(t *testing.T) {
-	t.Parallel()
-
-	testCases := map[string]struct {
-		request     request
-		expected    netip.AddrPort
-		expectedErr string
-	}{
-		"ipv4_endpoint": {
-			request: request{
-				addressType: ipv4,
-				destination: "192.0.2.10",
-				port:        5555,
-			},
-			expected: netip.MustParseAddrPort("192.0.2.10:5555"),
-		},
-		"ipv4_unspecified_address": {
-			request: request{
-				addressType: ipv4,
-				destination: "0.0.0.0",
-				port:        6000,
-			},
-			expected: netip.AddrPortFrom(netip.Addr{}, 6000),
-		},
-		"domain_name_with_port": {
-			request: request{
-				addressType: domainName,
-				destination: "client.example",
-				port:        7000,
-			},
-			expected: netip.AddrPortFrom(netip.Addr{}, 7000),
-		},
-		"domain_name_without_port": {
-			request: request{
-				addressType: domainName,
-				destination: "client.example",
-			},
-			expected: netip.AddrPort{},
-		},
-		"unsupported_address_type": {
-			request: request{
-				addressType: 255,
-			},
-			expectedErr: "address type 255 is not supported",
-		},
-	}
-
-	for name, testCase := range testCases {
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-
-			result, err := udpAssociateExpectedClientEndpoint(testCase.request)
-
-			if testCase.expectedErr != "" {
-				assert.ErrorContains(t, err, testCase.expectedErr)
-				return
-			}
-
-			assert.NoError(t, err)
-			assert.Equal(t, testCase.expected, result)
-		})
-	}
-}
-
 func Test_verifyFirstNegotiation(t *testing.T) {
 	t.Parallel()
 	testCases := map[string]struct {

internal/socks5/udp_router.go

@@ -76,7 +76,7 @@ func (r *udpRouter) registerAssociation(controlConn net.Conn, expectedAddrPort n
 	r.mutex.Lock()
 	defer r.mutex.Unlock()
 
-	const udpPacketChannelBuffer = 64
+	const udpPacketChannelBuffer = 2
 	associationID := r.nextAssociationID
 	r.nextAssociationID++
 

internal/socks5/udp_router_integration_test.go

@@ -19,8 +19,6 @@ import (
 )
 
 func Test_udpRouter_ResolveGithubFromCloudflareDNS(t *testing.T) {
-	t.Parallel()
-
 	ctx := t.Context()
 	var cancel context.CancelFunc
 	deadline, hasDeadline := ctx.Deadline()
@@ -109,7 +107,7 @@ func Test_udpRouter_ResolveGithubFromCloudflareDNS(t *testing.T) {
 		assert.NoError(t, err, "closing client UDP connection")
 	})
 
-	queryID := uint16(rand.Uint32()) //nolint:gosec
+	queryID := uint16(rand.Uint())
 	dnsRequest := &dns.Msg{
 		MsgHdr: dns.MsgHdr{
 			Id:               queryID,
@@ -158,7 +156,7 @@ func Test_udpRouter_ResolveGithubFromCloudflareDNS(t *testing.T) {
 	assert.Equal(t, dns.RcodeSuccess, dnsResponse.Rcode)
 	require.NotEmpty(t, dnsResponse.Question)
 	assert.Equal(t, dns.Fqdn("github.com"), dnsResponse.Question[0].Name)
-	assert.Equal(t, dns.TypeA, dnsResponse.Question[0].Qtype)
+	assert.Equal(t, uint16(dns.TypeA), dnsResponse.Question[0].Qtype)
 	assert.NotEmpty(t, dnsResponse.Answer)
 	require.NoError(t, err)
 }