From 38f9f795e4d1793cc03fe834a7d40e9ecc72503c Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Wed, 13 Nov 2024 15:31:05 -0600 Subject: [PATCH 1/4] wtwire: s/harness/wireMsgHarness This slightly more descriptive name obviates the repetitive comments at every call site. --- watchtower/wtwire/fuzz_test.go | 40 ++++++++++------------------------ 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/watchtower/wtwire/fuzz_test.go b/watchtower/wtwire/fuzz_test.go index 5a03b84e3..23643d4c4 100644 --- a/watchtower/wtwire/fuzz_test.go +++ b/watchtower/wtwire/fuzz_test.go @@ -18,12 +18,12 @@ func prefixWithMsgType(data []byte, prefix MessageType) []byte { return data } -// harness performs the actual fuzz testing of the appropriate wire message. -// This function will check that the passed-in message passes wire length -// checks, is a valid message once deserialized, and passes a sequence of +// wireMsgHarness performs the actual fuzz testing of the appropriate wire +// message. This function will check that the passed-in message passes wire +// length checks, is a valid message once deserialized, and passes a sequence of // serialization and deserialization checks. Returns an int that determines // whether the input is unique or not. -func harness(t *testing.T, data []byte, emptyMsg Message) { +func wireMsgHarness(t *testing.T, data []byte, emptyMsg Message) { t.Helper() // Create a reader with the byte array. @@ -64,9 +64,7 @@ func FuzzCreateSessionReply(f *testing.F) { // check if the max payload constraint is violated. emptyMsg := CreateSessionReply{} - // Pass the message into our general fuzz harness for wire - // messages! - harness(t, data, &emptyMsg) + wireMsgHarness(t, data, &emptyMsg) }) } @@ -79,9 +77,7 @@ func FuzzCreateSession(f *testing.F) { // check if the max payload constraint is violated. emptyMsg := CreateSession{} - // Pass the message into our general fuzz harness for wire - // messages! - harness(t, data, &emptyMsg) + wireMsgHarness(t, data, &emptyMsg) }) } @@ -94,9 +90,7 @@ func FuzzDeleteSessionReply(f *testing.F) { // check if the max payload constraint is violated. emptyMsg := DeleteSessionReply{} - // Pass the message into our general fuzz harness for wire - // messages! - harness(t, data, &emptyMsg) + wireMsgHarness(t, data, &emptyMsg) }) } @@ -109,9 +103,7 @@ func FuzzDeleteSession(f *testing.F) { // check if the max payload constraint is violated. emptyMsg := DeleteSession{} - // Pass the message into our general fuzz harness for wire - // messages! - harness(t, data, &emptyMsg) + wireMsgHarness(t, data, &emptyMsg) }) } @@ -124,9 +116,7 @@ func FuzzError(f *testing.F) { // check if the max payload constraint is violated. emptyMsg := Error{} - // Pass the message into our general fuzz harness for wire - // messages! - harness(t, data, &emptyMsg) + wireMsgHarness(t, data, &emptyMsg) }) } @@ -139,9 +129,7 @@ func FuzzInit(f *testing.F) { // check if the max payload constraint is violated. emptyMsg := Init{} - // Pass the message into our general fuzz harness for wire - // messages! - harness(t, data, &emptyMsg) + wireMsgHarness(t, data, &emptyMsg) }) } @@ -154,9 +142,7 @@ func FuzzStateUpdateReply(f *testing.F) { // check if the max payload constraint is violated. emptyMsg := StateUpdateReply{} - // Pass the message into our general fuzz harness for wire - // messages! - harness(t, data, &emptyMsg) + wireMsgHarness(t, data, &emptyMsg) }) } @@ -169,8 +155,6 @@ func FuzzStateUpdate(f *testing.F) { // check if the max payload constraint is violated. emptyMsg := StateUpdate{} - // Pass the message into our general fuzz harness for wire - // messages! - harness(t, data, &emptyMsg) + wireMsgHarness(t, data, &emptyMsg) }) } From e2f7eb963f5057d4f3d31b04a20229112d1936dd Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Wed, 13 Nov 2024 15:28:06 -0600 Subject: [PATCH 2/4] wtwire: move message prefixing to the harness The prefixing is done every time the harness is used, so it may as well reside in the harness itself. Since we already pass an empty message of the correct type, we can use that message to get the required prefix bytes. --- watchtower/wtwire/fuzz_test.go | 38 +++++++--------------------------- 1 file changed, 8 insertions(+), 30 deletions(-) diff --git a/watchtower/wtwire/fuzz_test.go b/watchtower/wtwire/fuzz_test.go index 23643d4c4..4ed69487c 100644 --- a/watchtower/wtwire/fuzz_test.go +++ b/watchtower/wtwire/fuzz_test.go @@ -26,17 +26,19 @@ func prefixWithMsgType(data []byte, prefix MessageType) []byte { func wireMsgHarness(t *testing.T, data []byte, emptyMsg Message) { t.Helper() - // Create a reader with the byte array. - r := bytes.NewReader(data) - - // Make sure byte array length (excluding 2 bytes for message type) is - // less than max payload size for the wire message. - payloadLen := uint32(len(data)) - 2 + // Make sure byte array length is less than max payload size for the + // wire message. + payloadLen := uint32(len(data)) if payloadLen > emptyMsg.MaxPayloadLength(0) { // Ignore this input - max payload constraint violated. return } + data = prefixWithMsgType(data, emptyMsg.MsgType()) + + // Create a reader with the byte array. + r := bytes.NewReader(data) + msg, err := ReadMessage(r, 0) if err != nil { return @@ -57,9 +59,6 @@ func wireMsgHarness(t *testing.T, data []byte, emptyMsg Message) { func FuzzCreateSessionReply(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Prefix with MsgCreateSessionReply. - data = prefixWithMsgType(data, MsgCreateSessionReply) - // Create an empty message so that the FuzzHarness func can // check if the max payload constraint is violated. emptyMsg := CreateSessionReply{} @@ -70,9 +69,6 @@ func FuzzCreateSessionReply(f *testing.F) { func FuzzCreateSession(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Prefix with MsgCreateSession. - data = prefixWithMsgType(data, MsgCreateSession) - // Create an empty message so that the FuzzHarness func can // check if the max payload constraint is violated. emptyMsg := CreateSession{} @@ -83,9 +79,6 @@ func FuzzCreateSession(f *testing.F) { func FuzzDeleteSessionReply(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Prefix with MsgDeleteSessionReply. - data = prefixWithMsgType(data, MsgDeleteSessionReply) - // Create an empty message so that the FuzzHarness func can // check if the max payload constraint is violated. emptyMsg := DeleteSessionReply{} @@ -96,9 +89,6 @@ func FuzzDeleteSessionReply(f *testing.F) { func FuzzDeleteSession(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Prefix with MsgDeleteSession. - data = prefixWithMsgType(data, MsgDeleteSession) - // Create an empty message so that the FuzzHarness func can // check if the max payload constraint is violated. emptyMsg := DeleteSession{} @@ -109,9 +99,6 @@ func FuzzDeleteSession(f *testing.F) { func FuzzError(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Prefix with MsgError. - data = prefixWithMsgType(data, MsgError) - // Create an empty message so that the FuzzHarness func can // check if the max payload constraint is violated. emptyMsg := Error{} @@ -122,9 +109,6 @@ func FuzzError(f *testing.F) { func FuzzInit(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Prefix with MsgInit. - data = prefixWithMsgType(data, MsgInit) - // Create an empty message so that the FuzzHarness func can // check if the max payload constraint is violated. emptyMsg := Init{} @@ -135,9 +119,6 @@ func FuzzInit(f *testing.F) { func FuzzStateUpdateReply(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Prefix with MsgStateUpdateReply. - data = prefixWithMsgType(data, MsgStateUpdateReply) - // Create an empty message so that the FuzzHarness func can // check if the max payload constraint is violated. emptyMsg := StateUpdateReply{} @@ -148,9 +129,6 @@ func FuzzStateUpdateReply(f *testing.F) { func FuzzStateUpdate(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Prefix with MsgStateUpdate. - data = prefixWithMsgType(data, MsgStateUpdate) - // Create an empty message so that the FuzzHarness func can // check if the max payload constraint is violated. emptyMsg := StateUpdate{} From 567ff5d7510500e34aec1377c6fca5d328159f2a Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Wed, 13 Nov 2024 15:41:26 -0600 Subject: [PATCH 3/4] wtwire: remove incorrect function comment The harness doesn't return anything, so don't say that it does. --- watchtower/wtwire/fuzz_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/watchtower/wtwire/fuzz_test.go b/watchtower/wtwire/fuzz_test.go index 4ed69487c..0691b956e 100644 --- a/watchtower/wtwire/fuzz_test.go +++ b/watchtower/wtwire/fuzz_test.go @@ -21,8 +21,7 @@ func prefixWithMsgType(data []byte, prefix MessageType) []byte { // wireMsgHarness performs the actual fuzz testing of the appropriate wire // message. This function will check that the passed-in message passes wire // length checks, is a valid message once deserialized, and passes a sequence of -// serialization and deserialization checks. Returns an int that determines -// whether the input is unique or not. +// serialization and deserialization checks. func wireMsgHarness(t *testing.T, data []byte, emptyMsg Message) { t.Helper() From 9dd921243d83a7fb062453872b44ea316e13e32e Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Wed, 13 Nov 2024 15:50:11 -0600 Subject: [PATCH 4/4] wtwire: explain emptyMsg parameter in function comment This makes it unnecessary to explain the argument at every call site, so we can simplify each fuzz target. --- watchtower/wtwire/fuzz_test.go | 52 +++++++--------------------------- 1 file changed, 11 insertions(+), 41 deletions(-) diff --git a/watchtower/wtwire/fuzz_test.go b/watchtower/wtwire/fuzz_test.go index 0691b956e..385e5a58b 100644 --- a/watchtower/wtwire/fuzz_test.go +++ b/watchtower/wtwire/fuzz_test.go @@ -21,7 +21,9 @@ func prefixWithMsgType(data []byte, prefix MessageType) []byte { // wireMsgHarness performs the actual fuzz testing of the appropriate wire // message. This function will check that the passed-in message passes wire // length checks, is a valid message once deserialized, and passes a sequence of -// serialization and deserialization checks. +// serialization and deserialization checks. emptyMsg must be an empty Message +// of the type to be fuzzed, as it is used to determine the appropriate prefix +// bytes and max payload length for decoding. func wireMsgHarness(t *testing.T, data []byte, emptyMsg Message) { t.Helper() @@ -58,80 +60,48 @@ func wireMsgHarness(t *testing.T, data []byte, emptyMsg Message) { func FuzzCreateSessionReply(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Create an empty message so that the FuzzHarness func can - // check if the max payload constraint is violated. - emptyMsg := CreateSessionReply{} - - wireMsgHarness(t, data, &emptyMsg) + wireMsgHarness(t, data, &CreateSessionReply{}) }) } func FuzzCreateSession(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Create an empty message so that the FuzzHarness func can - // check if the max payload constraint is violated. - emptyMsg := CreateSession{} - - wireMsgHarness(t, data, &emptyMsg) + wireMsgHarness(t, data, &CreateSession{}) }) } func FuzzDeleteSessionReply(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Create an empty message so that the FuzzHarness func can - // check if the max payload constraint is violated. - emptyMsg := DeleteSessionReply{} - - wireMsgHarness(t, data, &emptyMsg) + wireMsgHarness(t, data, &DeleteSessionReply{}) }) } func FuzzDeleteSession(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Create an empty message so that the FuzzHarness func can - // check if the max payload constraint is violated. - emptyMsg := DeleteSession{} - - wireMsgHarness(t, data, &emptyMsg) + wireMsgHarness(t, data, &DeleteSession{}) }) } func FuzzError(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Create an empty message so that the FuzzHarness func can - // check if the max payload constraint is violated. - emptyMsg := Error{} - - wireMsgHarness(t, data, &emptyMsg) + wireMsgHarness(t, data, &Error{}) }) } func FuzzInit(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Create an empty message so that the FuzzHarness func can - // check if the max payload constraint is violated. - emptyMsg := Init{} - - wireMsgHarness(t, data, &emptyMsg) + wireMsgHarness(t, data, &Init{}) }) } func FuzzStateUpdateReply(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Create an empty message so that the FuzzHarness func can - // check if the max payload constraint is violated. - emptyMsg := StateUpdateReply{} - - wireMsgHarness(t, data, &emptyMsg) + wireMsgHarness(t, data, &StateUpdateReply{}) }) } func FuzzStateUpdate(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { - // Create an empty message so that the FuzzHarness func can - // check if the max payload constraint is violated. - emptyMsg := StateUpdate{} - - wireMsgHarness(t, data, &emptyMsg) + wireMsgHarness(t, data, &StateUpdate{}) }) }