Repository navigation
Conversation
AlxcBTW
left a comment
There was a problem hiding this comment.
Thanks a lot for this, @fogydes. Fuzzing what the service does with the messages (not just the parsers) is exactly the gap in docs/gaps.md, and the structure is good: the service.rs change is test-only (no runtime behaviour touched), the workflow diff is fine, and the target uses the real run over duplex streams. Three things need to change before it can land; I have not waited for the fuzz job to prove them, they follow from reading Service::command.
1. The "one answer per id" assertion will fire on valid input (false positive). The reader panics on any repeated answer.id. But the fuzzer controls the ids, and the service legitimately answers each line: for example {"id":1,"do":"nope"}\n sent twice gives two unknown_command answers for id 1 (Input::BadCommand(Some(id))), and two hellos give a bad_state answer each. The invariant only holds for unique ids. Please assert it relative to the input: track which ids the wire contained (or parse the wire lines with parse_command/the id field) and check that the number of answers for an id never exceeds the number of lines that carried it. Alternatively only assert on ids that occur once.
2. The target is not hermetic once hello is accepted. hello takes any http(s) URL and builds a real cloud client; after it, commands such as browser_sign_in or the e-mail code request make real HTTP requests to whatever host the fuzzer wrote (and browser::start may try to open a browser). Fuzz inputs would therefore reach the network and become non-deterministic. Please keep the fuzzer away from that: simplest is to drop (or rewrite to a fixed http://127.0.0.1:9-style address) hello commands in the harness, or to run only the pre-hello surface plus a fixed hello you control and filter the other lines that need a cloud. A short comment on which surface is covered would help too. (bind: 127.0.0.1:0, the bridge listener and Traffic::start bind loopback sockets per iteration; that is acceptable, but please mention it in the doc comment and keep an eye on exec/s.)
3. The hardening test is weaker than its name and the PR body. join_code_revoke is sent before hello, so the single answer is only the "Say hello first" bad_state failure. Please assert that explicitly (ans.id == 10 and that it is a failure with Failure::BAD_STATE), and also add a line that checks the oversized line really is skipped and the following line is still read (the id 10 answer already shows that, so just say so in a comment). Bonus, optional: send one {"id":1,"do":"hello",...}-free unknown command like {"id":5,"do":"nope"} to cover UNKNOWN_COMMAND.
Small things: the doc comment of the target lists "output ends with \n, valid UTF-8" as checked, but the reader works on lines(), so that is not actually asserted; either drop the claim or check it. fuzz/README.md says "lines over 1 MiB are discarded without unbounded memory growth", which the target cannot observe either; I would phrase it as "an oversized line does not stop the loop".
Follow-up from our side after merge: update the "Fuzzing beyond the parsers" entry in docs/gaps.md, no need for you to do it. Happy to re-run CI as soon as you push.
|
Thanks @AlxcBTW for the thorough and helpful review! Addressed all points in
|
Resolves #120.
Overview
As agreed in #120 and noted in
docs/gaps.mdunder Fuzzing beyond the parsers, this adds theservice_controlfuzz target and companion hardening test to drive the net service's control dispatcher against in-memory duplex streams.Invariants Tested & Documented (
fuzz/README.md)control::MAX_LINE(1 MiB) are discarded without unbounded memory growth.MAX_LINE, ends with\n, is valid UTF-8 and valid JSON, and parses as a controlOutput(AnswerorEvent).Verification
cargo fmt --manifest-path fuzz/Cargo.toml -- --checkcargo fmt --manifest-path Cargo.toml --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo clippy --manifest-path fuzz/Cargo.toml --all-targets -- -D warningscargo test -p anyplay-net(all 154 unit tests and 48 integration tests pass)Need help on this PR? Tag
@codesmith-botwith what you need.