Repository: pion/turn (go)

Issue:
WriteTo() on relay conn (net.PacketConn) returns incorrect number of bytes sent

### Your environment.
  - Version: v2.0.8
  - Browser: *include version*
  - Other Information - *stacktraces, related issues, suggestions how to fix, links for us to have context*

### What did you do?
Sent data from TURN client to a peer.

### What did you expect?
Passing buffer with size N to relayConn.WriteTo() should return N on success.

### What happened?
Returned number is slightly longer than what the given buffer size was.

```
    turn_server_test.go:330: relayConn0: sent 1024 (1028) bytes to 127.0.0.1:52277
    turn_server_test.go:337: relayConn1: sent 1024 (1028) bytes to 127.0.0.1:54472
```


Pull request:
Fix incorrect written size returned by relayConn.WriteTo()
Fixes #257 
See #257 for details.

Patch:
diff --git a/.github/hooks/pre-commit.sh b/.github/hooks/pre-commit.sh
index cc318d7..d5a1ce5 100755
--- a/.github/hooks/pre-commit.sh
+++ b/.github/hooks/pre-commit.sh
@@ -10,3 +10,4 @@
 exec 1>&2
 
 .github/lint-disallowed-functions-in-library.sh
+.github/lint-no-trailing-newline-in-log-messages.sh
diff --git a/.github/lint-no-trailing-newline-in-log-messages.sh b/.github/lint-no-trailing-newline-in-log-messages.sh
new file mode 100755
index 0000000..29cd4a2
--- /dev/null
+++ b/.github/lint-no-trailing-newline-in-log-messages.sh
@@ -0,0 +1,41 @@
+#!/usr/bin/env bash
+
+#
+# DO NOT EDIT THIS FILE
+#
+# It is automatically copied from https://github.com/pion/.goassets repository.
+#
+# If you want to update the shared CI config, send a PR to
+# https://github.com/pion/.goassets instead of this repository.
+#
+
+set -e
+
+# Disallow usages of functions that cause the program to exit in the library code
+SCRIPT_PATH=$( cd "$(dirname "${BASH_SOURCE[0]}")" ; pwd -P )
+if [ -f ${SCRIPT_PATH}/.ci.conf ]
+then
+  . ${SCRIPT_PATH}/.ci.conf
+fi
+
+files=$(
+  find "$SCRIPT_PATH/.." -name "*.go" \
+    | while read file
+    do
+      excluded=false
+      for ex in $EXCLUDE_DIRECTORIES
+      do
+        if [[ $file == */$ex/* ]]
+        then
+          excluded=true
+          break
+        fi
+      done
+      $excluded || echo "$file"
+    done
+)
+
+if grep -E '\.(Trace|Debug|Info|Warn|Error)f?\("[^"]*\\n"\)?' $files | grep -v -e 'nolint'; then
+	echo "Log format strings should have trailing new-line"
+	exit 1
+fi
\ No newline at end of file
diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml
index 18641ca..69bef30 100644
--- a/.github/workflows/codeql-analysis.yml
+++ b/.github/workflows/codeql-analysis.yml
@@ -18,6 +18,14 @@ jobs:
     - name: Checkout repo
       uses: actions/checkout@v3
 
+    # The code in examples/ might intentionally do things like log credentials
+    # in order to show how the library is used, aid in debugging etc. We
+    # should ignore those for CodeQL scanning, and only focus on the package
+    # itself.
+    - name: Remove example code
+      run: |
+        rm -rf examples/
+
     - name: Initialize CodeQL
       uses: github/codeql-action/init@v2
       with:
diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml
index d284b19..3858008 100644
--- a/.github/workflows/lint.yaml
+++ b/.github/workflows/lint.yaml
@@ -36,6 +36,9 @@ jobs:
         - name: Functions
           run: .github/lint-disallowed-functions-in-library.sh
 
+        - name: Logging messages should not have trailing newlines
+          run: .github/lint-no-trailing-newline-in-log-messages.sh
+
   lint-go:
     name: Go
     runs-on: ubuntu-latest
diff --git a/internal/client/conn.go b/internal/client/conn.go
index 6d53f38..8aeb742 100644
--- a/internal/client/conn.go
+++ b/internal/client/conn.go
@@ -153,6 +153,21 @@ func (c *UDPConn) ReadFrom(p []byte) (n int, addr net.Addr, err error) {
 	}
 }
 
+func (c *UDPConn) createPermission(perm *permission, addr net.Addr) error {
+	perm.mutex.Lock()
+	defer perm.mutex.Unlock()
+
+	if perm.state() == permStateIdle {
+		// punch a hole! (this would block a bit..)
+		if err := c.CreatePermissions(addr); err != nil {
+			c.permMap.delete(addr)
+			return err
+		}
+		perm.setState(permStatePermitted)
+	}
+	return nil
+}
+
 // WriteTo writes a packet with payload p to addr.
 // WriteTo can be made to time out and return
 // an Error with Timeout() == true after a fixed time limit;
@@ -172,30 +187,15 @@ func (c *UDPConn) WriteTo(p []byte, addr net.Addr) (int, error) { //nolint: goco
 		c.permMap.insert(addr, perm)
 	}
 
-	// This func-block would block, per destination IP (, or perm), until
-	// the perm state becomes "requested". Purpose of this is to guarantee
-	// the order of packets (within the same perm).
-	// Note that CreatePermission transaction may not be complete before
-	// all the data transmission. This is done assuming that the request
-	// will be mostly likely successful and we can tolerate some loss of
-	// UDP packet (or reorder), inorder to minimize the latency in most cases.
-	createPermission := func() error {
-		perm.mutex.Lock()
-		defer perm.mutex.Unlock()
-
-		if perm.state() == permStateIdle {
-			// punch a hole! (this would block a bit..)
-			if err = c.CreatePermissions(addr); err != nil {
-				c.permMap.delete(addr)
-				return err
-			}
-			perm.setState(permStatePermitted)
-		}
-		return nil
-	}
-
 	for i := 0; i < maxRetryAttempts; i++ {
-		if err = createPermission(); !errors.Is(err, errTryAgain) {
+		// c.createPermission() would block, per destination IP (, or perm),
+		// until the perm state becomes "requested". Purpose of this is to
+		// guarantee the order of packets (within the same perm).
+		// Note that CreatePermission transaction may not be complete before
+		// all the data transmission. This is done assuming that the request
+		// will be most likely successful and we can tolerate some loss of
+		// UDP packet (or reorder), inorder to minimize the latency in most cases.
+		if err = c.createPermission(perm, addr); !errors.Is(err, errTryAgain) {
 			break
 		}
 	}
@@ -277,7 +277,11 @@ func (c *UDPConn) WriteTo(p []byte, addr net.Addr) (int, error) { //nolint: goco
 	}()
 
 	// send via ChannelData
-	return c.sendChannelData(p, b.number)
+	_, err = c.sendChannelData(p, b.number)
+	if err != nil {
+		return 0, err
+	}
+	return len(p), nil
 }
 
 // Close closes the connection.
@@ -551,7 +555,11 @@ func (c *UDPConn) sendChannelData(data []byte, chNum uint16) (int, error) {
 		Number: proto.ChannelNumber(chNum),
 	}
 	chData.Encode()
-	return c.obs.WriteTo(chData.Raw, c.obs.TURNServerAddr())
+	_, err := c.obs.WriteTo(chData.Raw, c.obs.TURNServerAddr())
+	if err != nil {
+		return 0, err
+	}
+	return len(data), nil
 }
 
 func (c *UDPConn) onRefreshTimers(id int) {
diff --git a/lt_cred.go b/lt_cred.go
index f40f40c..d4e9ab5 100644
--- a/lt_cred.go
+++ b/lt_cred.go
@@ -36,7 +36,7 @@ func NewLongTermAuthHandler(sharedSecret string, l logging.LeveledLogger) AuthHa
 		l = logging.NewDefaultLoggerFactory().NewLogger("turn")
 	}
 	return func(username, realm string, srcAddr net.Addr) (key []byte, ok bool) {
-		l.Tracef("Authentication username=%q realm=%q srcAddr=%v\n", username, realm, srcAddr)
+		l.Tracef("Authentication username=%q realm=%q srcAddr=%v", username, realm, srcAddr)
 		t, err := strconv.Atoi(username)
 		if err != nil {
 			l.Errorf("Invalid time-windowed username %q", username)


Test patch:
diff --git a/internal/client/conn_test.go b/internal/client/conn_test.go
index 4effccd..c4ce6c3 100644
--- a/internal/client/conn_test.go
+++ b/internal/client/conn_test.go
@@ -73,4 +73,40 @@ func TestUDPConn(t *testing.T) {
 		assert.Equal(t, 0, len(bm.chanMap), "should be 0")
 		assert.Equal(t, 0, len(bm.addrMap), "should be 0")
 	})
+
+	t.Run("WriteTo()", func(t *testing.T) {
+		obs := &dummyUDPConnObserver{
+			_performTransaction: func(msg *stun.Message, to net.Addr, dontWait bool) (TransactionResult, error) {
+				return TransactionResult{}, errFake
+			},
+			_writeTo: func(data []byte, to net.Addr) (int, error) {
+				return len(data), nil
+			},
+		}
+
+		addr := &net.UDPAddr{
+			IP:   net.ParseIP("127.0.0.1"),
+			Port: 1234,
+		}
+
+		pm := newPermissionMap()
+		assert.True(t, pm.insert(addr, &permission{
+			st: permStatePermitted,
+		}))
+
+		bm := newBindingManager()
+		binding := bm.create(addr)
+		binding.setState(bindingStateReady)
+
+		conn := UDPConn{
+			obs:        obs,
+			permMap:    pm,
+			bindingMgr: bm,
+		}
+
+		buf := []byte("Hello")
+		n, err := conn.WriteTo(buf, addr)
+		assert.NoError(t, err, "should fail")
+		assert.Equal(t, len(buf), n)
+	})
 }
diff --git a/internal/client/permission_test.go b/internal/client/permission_test.go
new file mode 100644
index 0000000..cbbf087
--- /dev/null
+++ b/internal/client/permission_test.go
@@ -0,0 +1,75 @@
+package client
+
+import (
+	"net"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestPermission(t *testing.T) {
+	t.Run("Getter and setter", func(t *testing.T) {
+		perm := &permission{}
+
+		assert.Equal(t, permStateIdle, perm.state())
+		perm.setState(permStatePermitted)
+		assert.Equal(t, permStatePermitted, perm.state())
+	})
+}
+
+func TestPermissionMap(t *testing.T) {
+	t.Run("Basic operations", func(t *testing.T) {
+		pm := newPermissionMap()
+		assert.NotNil(t, pm)
+		assert.NotNil(t, pm.permMap)
+
+		perm1 := &permission{st: permStateIdle}
+		perm2 := &permission{st: permStatePermitted}
+		udpAddr1, _ := net.ResolveUDPAddr("udp", "1.2.3.4:5000")
+		udpAddr2, _ := net.ResolveUDPAddr("udp", "5.6.7.8:8888")
+		tcpAddr, _ := net.ResolveTCPAddr("tcp", "1.2.3.4:5000")
+		assert.True(t, pm.insert(udpAddr1, perm1))
+		assert.Equal(t, 1, len(pm.permMap))
+		assert.True(t, pm.insert(udpAddr2, perm2))
+		assert.Equal(t, 2, len(pm.permMap))
+		assert.False(t, pm.insert(tcpAddr, perm1))
+		assert.Equal(t, 2, len(pm.permMap))
+
+		p, ok := pm.find(udpAddr1)
+		assert.True(t, ok)
+		assert.Equal(t, perm1, p)
+		assert.Equal(t, permStateIdle, p.st)
+
+		p, ok = pm.find(udpAddr2)
+		assert.True(t, ok)
+		assert.Equal(t, perm2, p)
+		assert.Equal(t, permStatePermitted, p.st)
+
+		_, ok = pm.find(tcpAddr)
+		assert.False(t, ok)
+
+		addrs := pm.addrs()
+		ips := []net.IP{}
+		for _, addr := range addrs {
+			udpAddr, err := net.ResolveUDPAddr(addr.Network(), addr.String())
+			assert.NoError(t, err)
+			assert.Equal(t, 0, udpAddr.Port)
+			ips = append(ips, udpAddr.IP)
+		}
+
+		assert.Equal(t, 2, len(ips))
+		if ips[0].Equal(udpAddr1.IP) {
+			assert.True(t, ips[1].Equal(udpAddr2.IP))
+		} else {
+			assert.True(t, ips[0].Equal(udpAddr2.IP))
+			assert.True(t, ips[1].Equal(udpAddr1.IP))
+		}
+
+		pm.delete(tcpAddr)
+		assert.Equal(t, 2, len(pm.permMap))
+		pm.delete(udpAddr1)
+		assert.Equal(t, 1, len(pm.permMap))
+		pm.delete(udpAddr2)
+		assert.Equal(t, 0, len(pm.permMap))
+	})
+}