fuzz: add test for the gossipd message processing#9115
fuzz: add test for the gossipd message processing#9115NishantBansal2003 wants to merge 3 commits into
Conversation
|
|
||
| #define GOSSIP_STORE_TEMP_FILENAME "gossip_store.tmp" | ||
| #define GOSSIP_STORE_TEMP_FILENAME tal_strcat(tmpctx, GOSSIP_STORE_FILENAME, ".tmp") | ||
| #define GOSSIP_STORE_CORRUPT_FILENAME tal_strcat(tmpctx, GOSSIP_STORE_FILENAME, ".corrupt") |
There was a problem hiding this comment.
The correct C way to do this is:
#define GOSSIP_STORE_TEMP_FILENAME GOSSIP_STORE_FILENAME ".tmp"There was a problem hiding this comment.
Yes, but that would fail in our fuzzing setup, where we need to define a different GOSSIP_STORE_FILENAME for each worker to avoid corruption from multiple fuzz workers accessing the same file. String concatenation only works with string literals, but here we need GOSSIP_STORE_FILENAME to be a variable so that it can vary across workers
There was a problem hiding this comment.
In general caps macros (particularly without parens) imply something constant. Replacing it with something dynamic, particularly one dependent on tmpctx can easily throw someone off in the future causing hard-to-discover bugs and issues down the line.
What's the issue that's causing multiple fuzz workers to access the same file?
There was a problem hiding this comment.
What's the issue that's causing multiple fuzz workers to access the same file?
In libFuzzer, multiple fuzz workers run in the same CWD. If there is a file with the same name, one worker may rename it to .tmp, .compact, or something while another worker is still accessing the original file. In that case, the second worker may panic when attempting to access the file, causing a false-positive fuzz failure to be reported
In general caps macros (particularly without parens) imply something constant. Replacing it with something dynamic, particularly one dependent on tmpctx can easily throw someone off in the future causing hard-to-discover bugs and issues down the line.
Got your concern. I've updated the approach so the production paths stay as literal constants (no behaviour change), and are only overridden when GOSSIP_STORE_FUZZ_OVERRIDE is defined
NishantBansal2003
force-pushed
the
fuzz-gossipd
branch
from
14ee0ba to
91eee52
Compare
The .tmp, .corrupt and .compact paths are derived from GOSSIP_STORE_FILENAME, but were hardcoded as literal constants. Wrap their definitions in #ifndef GOSSIP_STORE_FUZZ_OVERRIDE so a fuzz target can supply its own paths before including these files. In production nothing defines GOSSIP_STORE_FUZZ_OVERRIDE, so the literals are used unchanged: no behaviour change. This is done for fuzz-gossipd: it already overrides GOSSIP_STORE_FILENAME with a per-process path, and this lets the derived .tmp/.corrupt/.compact paths follow, so parallel libFuzzer workers don't race on the same files in CWD. Changelog-None Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Changelog-None Co-authored-by: Chandra Pratap <Chand-ra@users.noreply.github.com> Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
NishantBansal2003
force-pushed
the
fuzz-gossipd
branch
from
91eee52 to
a76b3de
Compare
3 participants
Add state machine fuzz tests for gossipd message processing. Currently, all message states handled by gossipd are covered here, but other gossip messages like
gossip_timestamp_filter,query_short_channel_id, andquery_channel_rangeare handled by connectd, so those states need to be explicitly tested in separate fuzz tests.Most of the work here is based on #8423, so I've added the PR author as a co-author. I also rebased the changes and fixed the entropy issue afterward. Additionally, some new states have been added (e.g. UTXO lookup, update blockheight, seeker state machine), and a few existing states have been updated to support e2e message processing (e.g. UTXO lookup in channel announcements).
Also, in the ref PR there was a discussion about issues with multi worker fuzzing. I’ve addressed that in 26f7b52. I think with this we can run regression tests on some previously observed edge-cases
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:
tools/lightning-downgrade