Fix padding calculation, closes #45

osc/osc.go

@@ -707,7 +707,7 @@ func readArguments(msg *Message, reader *bufio.Reader, start *int) error {
 
 	// If the typetag doesn't start with ',', it's not valid
 	if typetags[0] != ',' {
-		return errors.New("unsupported type tag string")
+		return fmt.Errorf("unsupported type tag string %s", typetags)
 	}
 
 	// Remove ',' from the type tag
@@ -954,12 +954,8 @@ func readPaddedString(reader *bufio.Reader) (string, int, error) {
 	}
 	n := len(str)
 
-	// Remove the string delimiter, in order to calculate the right amount
-	// of padding bytes
-	str = str[:len(str)-1]
-
-	// Remove the padding bytes
-	padLen := padBytesNeeded(len(str)) - 1
+	// Remove the padding bytes (leaving the null delimiter)
+	padLen := padBytesNeeded(len(str))
 	if padLen > 0 {
 		n += padLen
 		padBytes := make([]byte, padLen)
@@ -968,7 +964,8 @@ func readPaddedString(reader *bufio.Reader) (string, int, error) {
 		}
 	}
 
-	return str, n, nil
+	// Strip off the string delimiter
+	return str[:len(str)-1], n, nil
 }
 
 // writePaddedString writes a string with padding bytes to the a buffer.
@@ -980,8 +977,14 @@ func writePaddedString(str string, buf *bytes.Buffer) (int, error) {
 		return 0, err
 	}
 
+	// Add a null terminator if not already present
+	if str[:len(str)-1] != 0 {
+		buf.WriteByte(0)
+		n += 1
+	}
+
 	// Calculate the padding bytes needed and create a buffer for the padding bytes
-	numPadBytes := padBytesNeeded(len(str))
+	numPadBytes := padBytesNeeded(n)
 	if numPadBytes > 0 {
 		padBytes := make([]byte, numPadBytes)
 		// Add the padding bytes to the buffer
@@ -998,7 +1001,7 @@ func writePaddedString(str string, buf *bytes.Buffer) (int, error) {
 // padBytesNeeded determines how many bytes are needed to fill up to the next 4
 // byte length.
 func padBytesNeeded(elementLen int) int {
-	return 4*(elementLen/4+1) - elementLen
+	return ((4 - (elementLen % 4)) % 4)
 }
 
 ////

osc/osc_test.go

@@ -3,6 +3,7 @@ package osc
 import (
 	"bufio"
 	"bytes"
+	"io"
 	"net"
 	"reflect"
 	"sync"
@@ -192,7 +193,7 @@ func TestServerMessageReceiving(t *testing.T) {
 
 		packet, err := server.ReceivePacket(c)
 		if err != nil {
-			t.Error("Server error")
+			t.Errorf("server error: %v", err)
 			return
 		}
 		if packet == nil {
@@ -314,14 +315,20 @@ func TestReadPaddedString(t *testing.T) {
 		buf []byte // buffer
 		n   int    // bytes needed
 		s   string // resulting string
+		e   error  // expected error
 	}{
-		{[]byte{'t', 'e', 's', 't', 's', 't', 'r', 'i', 'n', 'g', 0, 0}, 12, "teststring"},
-		{[]byte{'t', 'e', 's', 't', 0, 0, 0, 0}, 8, "test"},
+		{[]byte{'t', 'e', 's', 't', 'S', 't', 'r', 'i', 'n', 'g', 0, 0}, 12, "testString", nil},
+		{[]byte{'t', 'e', 's', 't', 'e', 'r', 's', 0}, 8, "testers", nil},
+		{[]byte{'t', 'e', 's', 't', 's', 0, 0, 0}, 8, "tests", nil},
+		{[]byte{'t', 'e', 's', 't', 0, 0, 0, 0}, 8, "test", nil},
+		{[]byte{}, 0, "", io.EOF},
+		{[]byte{'t', 'e', 's', 0}, 4, "tes", nil},   // OSC uses null terminated strings
+		{[]byte{'t', 'e', 's', 't'}, 0, "", io.EOF}, // if there is no null byte at the end, it doesn't work.
 	} {
 		buf := bytes.NewBuffer(tt.buf)
 		s, n, err := readPaddedString(bufio.NewReader(buf))
-		if err != nil {
-			t.Errorf("%s: Error reading padded string: %s", s, err)
+		if got, want := err, tt.e; got != want {
+			t.Errorf("%s: Unexpected error reading padded string; got = %s, want = %s", tt.s, got, want)
 		}
 		if got, want := n, tt.n; got != want {
 			t.Errorf("%s: Bytes needed don't match; got = %d, want = %d", tt.s, got, want)
@@ -333,46 +340,75 @@ func TestReadPaddedString(t *testing.T) {
 }
 
 func TestWritePaddedString(t *testing.T) {
-	buf := []byte{}
-	bytesBuffer := bytes.NewBuffer(buf)
-	testString := "testString"
-	expectedNumberOfWrittenBytes := len(testString) + padBytesNeeded(len(testString))
-
-	n, err := writePaddedString(testString, bytesBuffer)
-	if err != nil {
-		t.Errorf(err.Error())
-	}
+	for _, tt := range []struct {
+		s   string // string
+		buf []byte // resulting buffer
+		n   int    // bytes expected
+	}{
+		{"testString", []byte{'t', 'e', 's', 't', 'S', 't', 'r', 'i', 'n', 'g', 0, 0}, 12},
+		{"testers", []byte{'t', 'e', 's', 't', 'e', 'r', 's', 0}, 8},
+		{"tests", []byte{'t', 'e', 's', 't', 's', 0, 0, 0}, 8},
+		{"test", []byte{'t', 'e', 's', 't', 0, 0, 0, 0}, 8},
+		{"tes", []byte{'t', 'e', 's', 0}, 4},
+		{"tes\0", []byte{'t', 'e', 's', 0}, 4}, // Don't add a second null terminator if one is already present
+		{"", []byte{0, 0, 0, 0}, 4}, // OSC uses null terminated strings, padded to the 4 byte boundary
+	} {
+		buf := []byte{}
+		bytesBuffer := bytes.NewBuffer(buf)
 
-	if n != expectedNumberOfWrittenBytes {
-		t.Errorf("Expected number of written bytes should be \"%d\" and is \"%d\"", expectedNumberOfWrittenBytes, n)
+		n, err := writePaddedString(tt.s, bytesBuffer)
+		if err != nil {
+			t.Errorf(err.Error())
+		}
+		if got, want := n, tt.n; got != want {
+			t.Errorf("%s: Count of bytes written don't match; got = %d, want = %d", tt.s, got, want)
+		}
+		if got, want := bytesBuffer, tt.buf; bytes.Equal(got.Bytes(), want) {
+			t.Errorf("%s: Buffers don't match; got = %s, want = %s", tt.s, got.Bytes(), want)
+		}
 	}
 }
 
 func TestPadBytesNeeded(t *testing.T) {
 	var n int
 	n = padBytesNeeded(4)
-	if n != 4 {
-		t.Errorf("Number of pad bytes should be 4 and is: %d", n)
+	if n != 0 {
+		t.Errorf("Number of pad bytes should be 0 and is: %d", n)
 	}
 
 	n = padBytesNeeded(3)
 	if n != 1 {
 		t.Errorf("Number of pad bytes should be 1 and is: %d", n)
 	}
 
+	n = padBytesNeeded(2)
+	if n != 2 {
+		t.Errorf("Number of pad bytes should be 2 and is: %d", n)
+	}
+
 	n = padBytesNeeded(1)
 	if n != 3 {
 		t.Errorf("Number of pad bytes should be 3 and is: %d", n)
 	}
 
 	n = padBytesNeeded(0)
-	if n != 4 {
-		t.Errorf("Number of pad bytes should be 4 and is: %d", n)
+	if n != 0 {
+		t.Errorf("Number of pad bytes should be 0 and is: %d", n)
+	}
+
+	n = padBytesNeeded(5)
+	if n != 3 {
+		t.Errorf("Number of pad bytes should be 3 and is: %d", n)
+	}
+
+	n = padBytesNeeded(7)
+	if n != 1 {
+		t.Errorf("Number of pad bytes should be 1 and is: %d", n)
 	}
 
 	n = padBytesNeeded(32)
-	if n != 4 {
-		t.Errorf("Number of pad bytes should be 4 and is: %d", n)
+	if n != 0 {
+		t.Errorf("Number of pad bytes should be 0 and is: %d", n)
 	}
 
 	n = padBytesNeeded(63)