fuzz-tests: Add fuzz target for closing_complete #8216
Conversation
|
Probably the best person to review this code is @morehouse |
Yeah, I've had a conversation with him over mail. He said he'd get around to it soon. |
| size_t upto_closer_scriptpubkey = (uintptr_t)&x->closer_scriptpubkey - (uintptr_t)x; | ||
| if (memcmp(x, y, upto_closer_scriptpubkey) != 0) | ||
| return false; |
There was a problem hiding this comment.
I'm concerned that some architectures may pad/align struct members to 8 bytes, which means there would be 4 bytes of uninitialized padding between locktime and fee_satoshis that could trigger false positives.
If we want to use the memcmp trick, we should probably move locktime to after fee_satoshis and then manually compare fields starting with locktime and thereafter.
There was a problem hiding this comment.
The memcmp dance takes as much lines as individually comparing struct closing_complete's members, so I've replaced the former with the latter in the latest push.
|
Also would be good to add a minimal input set as a seed corpus. |
| assert(!memcmp(&x->channel_id, &y->channel_id, sizeof(struct channel_id))); | ||
| assert(x->locktime == y->locktime); | ||
| assert(!memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(struct amount_sat))); |
There was a problem hiding this comment.
Nit: for better readability can we use assert(memcmp(...) == 0)? I've seen bugs arise because people thought !memcmp(...) meant the buffers differed.
| assert(!memcmp(&x->channel_id, &y->channel_id, sizeof(struct channel_id))); | |
| assert(x->locktime == y->locktime); | |
| assert(!memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(struct amount_sat))); | |
| assert(memcmp(&x->channel_id, &y->channel_id, sizeof(struct channel_id)) == 0); | |
| assert(x->locktime == y->locktime); | |
| assert(memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(struct amount_sat)) == 0); |
| assert(!memcmp(&x->channel_id, &y->channel_id, sizeof(struct channel_id))); | ||
| assert(x->locktime == y->locktime); | ||
| assert(!memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(struct amount_sat))); |
There was a problem hiding this comment.
Nit: common trick to avoid the memcmp breaking if the struct type changes in the future:
| assert(!memcmp(&x->channel_id, &y->channel_id, sizeof(struct channel_id))); | |
| assert(x->locktime == y->locktime); | |
| assert(!memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(struct amount_sat))); | |
| assert(!memcmp(&x->channel_id, &y->channel_id, sizeof(x->channel_id))); | |
| assert(x->locktime == y->locktime); | |
| assert(!memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(x->fee_satoshis))); |
I did. Doesn't seem to do a lot though. Am I using it right:
Right. |
No, you should be making sure that ./tests/fuzz/run.py tests/fuzz/corpora --merge_dir your_closing_complete_corpusSee documentation here. And please also make sure any other PRs you have open with new corpus inputs are properly minimized this way (e.g., #8183). |
Done. |
| assert(tal_arr_eq(x->closee_scriptpubkey, y->closee_scriptpubkey)); | ||
|
|
||
| assert(x->tlvs && y->tlvs); | ||
| return tal_arr_eq(x->tlvs->closer_and_closee_outputs, y->tlvs->closer_and_closee_outputs); |
There was a problem hiding this comment.
Looks like we should also be comparing the other TLV fields:
closer_output_only
closee_output_only
There was a problem hiding this comment.
The closer_and_closee_outputs field is actually an amalgamation of the closer_output and closee_output fields. Is there any merit to comparing both these fields separately as well?
There was a problem hiding this comment.
Pretty sure that's not true. They're all distinct signatures. https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#rationale-21
There was a problem hiding this comment.
Oh okay, I seem to have misinterpreted a comment in wire/peer_wiregen.c:
struct tlv_closing_tlvs {
struct tlv_field *fields;
/* The following explicit fields could just point into the
* tlv_field entries above to save memory. */
secp256k1_ecdsa_signature *closer_output_only;
secp256k1_ecdsa_signature *closee_output_only;
secp256k1_ecdsa_signature *closer_and_closee_outputs;
};
Changelog-None: 'closing_signed' and 'closing_complete' are channel closing negotiation messages defined in BOLT ElementsProject#2. While 'closing_signed' has a wire fuzz test, 'closing_complete' does not. Add a test to perform a round-trip encoding check (towire -> fromwire) similar to the other wire fuzzers.
Add a minimal input set as a seed corpus for the newly introduced test. This leads to discovery of interesting code paths faster.
closing_signedandclosing_completeare channel closing negotiation messages defined in BOLT #2.While
closing_signedhas a wire fuzz test,closing_completedoes not. Add a test to perform a round-trip encoding check (towire -> fromwire) similar to the other wire fuzzers.Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked: