lnwire: fuzz onion failure messages #7669
Conversation
Crypt-iQ
left a comment
Crypt-iQ
left a comment
There was a problem hiding this comment.
will do a more in-depth pass later and run the fuzz harnesses, 2 minor comments
lnwire/fuzz_test.go
Outdated
There was a problem hiding this comment.
I guess we should fail here?
There was a problem hiding this comment.
Not sure, it seems like a precondition for the rest of the test that we have a valid message
There was a problem hiding this comment.
Not every sequence of bytes is decodable as an onion error, so we don't want to report a bug when we encounter such a byte sequence. If the sequence is decodable, we do the further tests below.
Crypt-iQ
left a comment
Crypt-iQ
left a comment
There was a problem hiding this comment.
I think that this should use testify instead of t.Fatal. Otherwise, looks good
|
@morehouse, remember to re-request review from reviewers when ready |
lnwire/fuzz_test.go
Outdated
There was a problem hiding this comment.
Why wait for an encode failure to check this condition? Seems like something you can check immediately.
There was a problem hiding this comment.
Waiting to check this condition avoids some false negatives.
Suppose len(data) == FailureMessageLength and data contains the channel_update type prefix. In this case, encoding will succeed, but we wouldn't even try if this condition was checked earlier.
ProofOfKeags
left a comment
ProofOfKeags
left a comment
There was a problem hiding this comment.
Looks good. I had one comment but it's small and non-blocking. Three cheers for randomized testing!
|
Needs a rebase. |
Fuzz tests for decoding and encoding of onion failure messages, based on the fuzz harness for other lnwire messages. The onion failure messages were uncovered by existing fuzz tests.
The test is identical to other onion failure fuzz tests, except that it
uses a custom equality function to get around the nil != []byte{} issue
with reflect.DeepEqual.
morehouse
force-pushed
the
fuzz_lnwire_onion_errors
branch
from
1ec843f to
66d6a84
Compare
|
Rebased and addressed nits. |
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Looks like linter timed out. Re-running the job now.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to Branch Protection failures
|
Merge queue would've worked, but a new perm added was too heavy handed, have tweaked that now. |
9 participants
Fuzz tests for decoding and encoding of onion failure messages, based on the fuzz harness for other lnwire messages. The onion failure messages were not covered by existing fuzz tests.
Some of these fuzz tests will fail until #7665 is merged, since the decode-encode-decode of messages with
channel_updatescurrently can mutate thechannel_update. See #6461 (comment).No crashes found after 50+ CPU-hours of fuzzing for each target with #7665 patched in.