From d37d83385dc92a6c4f14fd71624981018f264b00 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Wed, 25 Nov 2020 20:04:57 +0000 Subject: [PATCH 01/29] Fix padding calculation, hopefully fixes #45 Untested, but maths checked externally --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index e829ec4..bbe1508 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -998,7 +998,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 -elementLen%4 } //// From 73358d09af5610de6195eb8128261e6514fd30aa Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Fri, 19 Mar 2021 00:47:32 +0000 Subject: [PATCH 02/29] Add some more byte padding tests (and fix up the broken ones) --- osc/osc_test.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index 9e5e358..a19f640 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -351,8 +351,8 @@ func TestWritePaddedString(t *testing.T) { 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) @@ -360,19 +360,24 @@ func TestPadBytesNeeded(t *testing.T) { 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(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) From 6b33958cdcc427259558ac43862c3ebc6f569bab Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Fri, 19 Mar 2021 01:08:09 +0000 Subject: [PATCH 03/29] Fix the incorrect padding on the example packet --- osc/osc_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index a19f640..1b9be1f 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -316,7 +316,7 @@ func TestReadPaddedString(t *testing.T) { s string // resulting string }{ {[]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'}, 4, "test"}, } { buf := bytes.NewBuffer(tt.buf) s, n, err := readPaddedString(bufio.NewReader(buf)) From c642f32523eadd1317b12e4ee52614d7d7caf81d Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Fri, 19 Mar 2021 01:22:13 +0000 Subject: [PATCH 04/29] Guess at how to fix the out of range error It's either this or put the -1 within the brackets... --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index bbe1508..ffa69be 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -959,7 +959,7 @@ func readPaddedString(reader *bufio.Reader) (string, int, error) { str = str[:len(str)-1] // Remove the padding bytes - padLen := padBytesNeeded(len(str)) - 1 + padLen := padBytesNeeded(len(str)) if padLen > 0 { n += padLen padBytes := make([]byte, padLen) From 6ff26a0920d40dc91aaac7d44e816f3eb187c682 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Fri, 19 Mar 2021 01:35:29 +0000 Subject: [PATCH 05/29] Try the other way to fix the calculation --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index ffa69be..e066c4d 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -959,7 +959,7 @@ func readPaddedString(reader *bufio.Reader) (string, int, error) { str = str[:len(str)-1] // Remove the padding bytes - padLen := padBytesNeeded(len(str)) + padLen := padBytesNeeded(len(str) - 1) if padLen > 0 { n += padLen padBytes := make([]byte, padLen) From a6d96e2859348fd971a216f9c1fa86bd4befebe2 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Fri, 19 Mar 2021 01:48:26 +0000 Subject: [PATCH 06/29] Fix a lint issue --- osc/osc_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index 1b9be1f..e68c146 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -364,7 +364,7 @@ func TestPadBytesNeeded(t *testing.T) { 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) From 4921c4549a9e8ce740a1f3972048ac18f315a93e Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Fri, 19 Mar 2021 01:53:39 +0000 Subject: [PATCH 07/29] Actually use the maths I tested which works in Go --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index e066c4d..9f60792 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -998,7 +998,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 -elementLen%4 + return -(-elementLen%4) } //// From ee0d744b4ddc35fc5f800748ff0a18827d076a63 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Fri, 19 Mar 2021 02:07:34 +0000 Subject: [PATCH 08/29] Further correction to the formula --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index 9f60792..61b8a11 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -998,7 +998,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 -(-elementLen%4) + return ((4 - elementLen) % 4) } //// From 54b9f885b2fee87529d7a16093f5c96fec2ea1e0 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Fri, 19 Mar 2021 02:10:48 +0000 Subject: [PATCH 09/29] Try not removing stuff twice again --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index 61b8a11..3e85239 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -959,7 +959,7 @@ func readPaddedString(reader *bufio.Reader) (string, int, error) { str = str[:len(str)-1] // Remove the padding bytes - padLen := padBytesNeeded(len(str) - 1) + padLen := padBytesNeeded(len(str)) if padLen > 0 { n += padLen padBytes := make([]byte, padLen) From 3a933786b7b1e5d262cdc3e0ec1b5e9d0379e8b7 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 13:55:44 +0000 Subject: [PATCH 10/29] Add some more tests --- osc/osc_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/osc/osc_test.go b/osc/osc_test.go index e68c146..5911fd2 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -316,6 +316,8 @@ func TestReadPaddedString(t *testing.T) { s string // resulting string }{ {[]byte{'t', 'e', 's', 't', 's', 't', 'r', 'i', 'n', 'g', 0, 0}, 12, "teststring"}, + {[]byte{'t', 'e', 's', 't', 'e', 'r', 's', 0}, 8, "testers"}, + {[]byte{'t', 'e', 's', 't', 's', 0, 0, 0}, 8, "tests"}, {[]byte{'t', 'e', 's', 't'}, 4, "test"}, } { buf := bytes.NewBuffer(tt.buf) @@ -375,6 +377,16 @@ func TestPadBytesNeeded(t *testing.T) { 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 != 0 { t.Errorf("Number of pad bytes should be 0 and is: %d", n) From 699e7b69dcf53f4cb6a7df7fe6784b380b6595e2 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 14:02:19 +0000 Subject: [PATCH 11/29] Correct the padding bytes needed calculation to actually work --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index 3e85239..e44a10a 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -998,7 +998,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) + return ((4 - (elementLen % 4)) % 4) } //// From 31e0e07d4c5e63016cb16d3d24ad83ae1fb2fd21 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 14:19:56 +0000 Subject: [PATCH 12/29] Add some more debug --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index e44a10a..a1819a6 100644 --- a/osc/osc.go +++ b/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 From 0199891ec4a82b1a1c900e2213c4e840de0881a9 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 15:35:30 +0000 Subject: [PATCH 13/29] Don't remove the string delimiter --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index a1819a6..86c0ef9 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -956,7 +956,7 @@ func readPaddedString(reader *bufio.Reader) (string, int, error) { // Remove the string delimiter, in order to calculate the right amount // of padding bytes - str = str[:len(str)-1] + // str = str[:len(str)-1] // Remove the padding bytes padLen := padBytesNeeded(len(str)) From 5d752b639f961f9649367d5cbf732ccea3561e5f Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 15:37:00 +0000 Subject: [PATCH 14/29] Add more debugging --- osc/osc_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index 5911fd2..a474789 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -192,7 +192,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 { From 8fea80906f60476309d9b495dbb1669b3f77005b Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 15:55:51 +0000 Subject: [PATCH 15/29] We do actually need to remove the string delimiter --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index 86c0ef9..a1819a6 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -956,7 +956,7 @@ func readPaddedString(reader *bufio.Reader) (string, int, error) { // Remove the string delimiter, in order to calculate the right amount // of padding bytes - // str = str[:len(str)-1] + str = str[:len(str)-1] // Remove the padding bytes padLen := padBytesNeeded(len(str)) From 4d26d1980350938f39b5161e02d2b6cab5b8c16f Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 16:03:45 +0000 Subject: [PATCH 16/29] Address @chabad360 comments --- osc/osc.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/osc/osc.go b/osc/osc.go index a1819a6..23472ff 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -954,11 +954,7 @@ 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 + // Remove the padding bytes (leaving the null delimiter) padLen := padBytesNeeded(len(str)) if padLen > 0 { n += 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. From 7075a586e8f4649999dbf217c363ded6aa190410 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 16:05:48 +0000 Subject: [PATCH 17/29] Add @chabad360 's extra tests, but fix one I think is incorrect --- osc/osc_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index a474789..b3314dc 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -318,7 +318,9 @@ func TestReadPaddedString(t *testing.T) { {[]byte{'t', 'e', 's', 't', 's', 't', 'r', 'i', 'n', 'g', 0, 0}, 12, "teststring"}, {[]byte{'t', 'e', 's', 't', 'e', 'r', 's', 0}, 8, "testers"}, {[]byte{'t', 'e', 's', 't', 's', 0, 0, 0}, 8, "tests"}, - {[]byte{'t', 'e', 's', 't'}, 4, "test"}, + {[]byte{'t', 'e', 's', 't', 0, 0, 0, 0}, 8, "test"}, + {[]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)) From 952f77bb2ad85fdd8b671751abcba5a6400927b7 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 16:19:08 +0000 Subject: [PATCH 18/29] Add support for expected errors in TestReadPaddedString --- osc/osc_test.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index b3314dc..2d80d04 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -3,6 +3,7 @@ package osc import ( "bufio" "bytes" + "io" "net" "reflect" "sync" @@ -314,18 +315,19 @@ 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', 'e', 'r', 's', 0}, 8, "testers"}, - {[]byte{'t', 'e', 's', 't', 's', 0, 0, 0}, 8, "tests"}, - {[]byte{'t', 'e', 's', 't', 0, 0, 0, 0}, 8, "test"}, - {[]byte{'t', 'e', 's', 0}, 4, "tes", nil}, // OSC uses null terminated strings + {[]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{'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) From 1b4ed372a99e47d4f5dfc64d61983e6b020a9b91 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 19:25:53 +0000 Subject: [PATCH 19/29] Improve TestWritePaddedString with more test cases --- osc/osc_test.go | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index 2d80d04..c862445 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -317,10 +317,11 @@ func TestReadPaddedString(t *testing.T) { s string // resulting string e error // expected error }{ - {[]byte{'t', 'e', 's', 't', 's', 't', 'r', 'i', 'n', 'g', 0, 0}, 12, "teststring", nil}, + {[]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, "", nil}, {[]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. } { @@ -339,18 +340,31 @@ 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}, + {"", []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; Equal(got, want) { + t.Errorf("%s: Buffers don't match; got = %s, want = %s", tt.s, got.String(), want.String()) + } } } From d9159af2f21041c82c24658158788e142a7542f0 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 19:44:07 +0000 Subject: [PATCH 20/29] Fix the test code --- osc/osc_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index c862445..de57961 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -362,8 +362,8 @@ func TestWritePaddedString(t *testing.T) { 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; Equal(got, want) { - t.Errorf("%s: Buffers don't match; got = %s, want = %s", tt.s, got.String(), want.String()) + if got, want := bytesBuffer, tt.buf; bytes.Equal(got, want) { + t.Errorf("%s: Buffers don't match; got = %s, want = %s", tt.s, got.Bytes(), want) } } } From c5b97671f53a101a23e3fad0e116c8ad531a7698 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 19:56:22 +0000 Subject: [PATCH 21/29] Fix the type of an argument to equal --- osc/osc_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index de57961..49baa5b 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -362,7 +362,7 @@ func TestWritePaddedString(t *testing.T) { 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, 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) } } From ba7f2d6d965a82974c2e67a6b20cac5dec6f8c20 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 20:10:08 +0000 Subject: [PATCH 22/29] Fix a test and add another one --- osc/osc_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index 49baa5b..cd753c8 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -321,7 +321,7 @@ func TestReadPaddedString(t *testing.T) { {[]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, "", 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. } { @@ -350,6 +350,7 @@ func TestWritePaddedString(t *testing.T) { {"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{} From f2ddec4e1d97a3cd646271f25a0631121df72484 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 20:21:25 +0000 Subject: [PATCH 23/29] Add the null byte to the end of strings --- osc/osc.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index 23472ff..18a2762 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -977,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 From 11cef410f83098f55a4017a890b480fe6e0f01fa Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 20:45:19 +0000 Subject: [PATCH 24/29] Fix the check for pre-existing nulls --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index 18a2762..d2338de 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -978,7 +978,7 @@ func writePaddedString(str string, buf *bytes.Buffer) (int, error) { } // Add a null terminator if not already present - if str[:len(str)-1] != 0 { + if str[len(str)-1] != 0 { buf.WriteByte(0) n += 1 } From 495f85c14c3c69066b58e826aaf7ed08622da9a1 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 20:53:16 +0000 Subject: [PATCH 25/29] Add even more tests around nulls --- osc/osc_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index cd753c8..e00cbff 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -322,8 +322,10 @@ func TestReadPaddedString(t *testing.T) { {[]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. + {[]byte{'t', 'e', 's', 0}, 4, "tes", nil}, // OSC uses null terminated strings + {[]byte{'t', 'e', 's', 0, 0, 0, 0, 0}, 4, "tes", nil}, // Additional nulls should be ignored + {[]byte{'t', 'e', 's', 0, 0, 0}, 4, "tes", nil}, // Whether or not the nulls fall on a 4 byte padding boundary + {[]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)) @@ -350,8 +352,10 @@ func TestWritePaddedString(t *testing.T) { {"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 + {"tes\x00", []byte{'t', 'e', 's', 0}, 4}, // Don't add a second null terminator if one is already present + {"tes\x00\x00\x00\x00\x00", []byte{'t', 'e', 's', 0}, 4}, // Skip extra nulls + {"tes\x00\x00\x00", []byte{'t', 'e', 's', 0}, 4}, // Even if they don't fall on a 4 byte padding boundary + {"", []byte{0, 0, 0, 0}, 4}, // OSC uses null terminated strings, padded to the 4 byte boundary } { buf := []byte{} bytesBuffer := bytes.NewBuffer(buf) From 7893ee4475a2700924bc1b3a5d83da3d23028920 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 21:07:13 +0000 Subject: [PATCH 26/29] Use full escaped strings for more helpful troubleshooting --- osc/osc_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index e00cbff..d7d5ed3 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -330,13 +330,13 @@ func TestReadPaddedString(t *testing.T) { buf := bytes.NewBuffer(tt.buf) s, n, err := readPaddedString(bufio.NewReader(buf)) if got, want := err, tt.e; got != want { - t.Errorf("%s: Unexpected error reading padded string; got = %s, want = %s", tt.s, got, want) + t.Errorf("%q: Unexpected error reading padded string; got = %q, want = %q", 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) + t.Errorf("%q: Bytes needed don't match; got = %d, want = %d", tt.s, got, want) } if got, want := s, tt.s; got != want { - t.Errorf("%s: Strings don't match; got = %s, want = %s", tt.s, got, want) + t.Errorf("%q: Strings don't match; got = %q, want = %q", tt.s, got, want) } } } @@ -365,10 +365,10 @@ func TestWritePaddedString(t *testing.T) { 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) + t.Errorf("%q: 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) + t.Errorf("%q: Buffers don't match; got = %q, want = %q", tt.s, got.Bytes(), want) } } } From 62ff70956c0afd8f8b7da40133e6e20b70462ffc Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 21:23:30 +0000 Subject: [PATCH 27/29] We want to know if they're NOT equal! --- osc/osc_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc_test.go b/osc/osc_test.go index d7d5ed3..b22fd69 100644 --- a/osc/osc_test.go +++ b/osc/osc_test.go @@ -367,7 +367,7 @@ func TestWritePaddedString(t *testing.T) { if got, want := n, tt.n; got != want { t.Errorf("%q: 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) { + if got, want := bytesBuffer, tt.buf; !bytes.Equal(got.Bytes(), want) { t.Errorf("%q: Buffers don't match; got = %q, want = %q", tt.s, got.Bytes(), want) } } From 7a0dc97086ed31505ad7b475d94d321e441cd2e0 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 21:27:10 +0000 Subject: [PATCH 28/29] Ensure we correctly handle multiple nulls in the incoming string --- osc/osc.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/osc/osc.go b/osc/osc.go index d2338de..53473a8 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -971,17 +971,20 @@ func readPaddedString(reader *bufio.Reader) (string, int, error) { // writePaddedString writes a string with padding bytes to the a buffer. // Returns, the number of written bytes and an error if any. func writePaddedString(str string, buf *bytes.Buffer) (int, error) { + // Truncate at the first null, just in case there is more than one present + nullIndex := strings.Index(str, "\x00") + if (nullIndex > 0) { + str = str[:nullIndex] + } // Write the string to the buffer n, err := buf.WriteString(str) if err != nil { return 0, err } - // Add a null terminator if not already present - if str[len(str)-1] != 0 { - buf.WriteByte(0) - n += 1 - } + // Always write a null terminator, as we stripped it earlier if it existed + buf.WriteByte(0) + n += 1 // Calculate the padding bytes needed and create a buffer for the padding bytes numPadBytes := padBytesNeeded(n) From 41e1b810194ded7de464299266376d6eb0e99596 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Mon, 22 Mar 2021 22:48:48 +0000 Subject: [PATCH 29/29] Fix the lint issue --- osc/osc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/osc.go b/osc/osc.go index 53473a8..d4b89ca 100644 --- a/osc/osc.go +++ b/osc/osc.go @@ -973,7 +973,7 @@ func readPaddedString(reader *bufio.Reader) (string, int, error) { func writePaddedString(str string, buf *bytes.Buffer) (int, error) { // Truncate at the first null, just in case there is more than one present nullIndex := strings.Index(str, "\x00") - if (nullIndex > 0) { + if nullIndex > 0 { str = str[:nullIndex] } // Write the string to the buffer