fuzz: test HTTPRequest state machine in http

frankomosh commented at 4:35 AM on September 1, 2026: contributor

Follow-up to #35735, adding a test to the state machine added there. As it is, the target parses the whole input in one pass, so the paths that resume a partly-read request are unreachable from it.

This target now parses each input twice, once whole and once in random slices with a parse attempt after each, and requires both runs to produce the same request. HTTP framing is defined by the byte stream, so how it was split must not change the result.

Also raises the input cap from 4096, which sat below MAX_HEADERS_SIZE and kept the header size limit out of reach.

fuzz: extract http_request parse and framing check into helpers

Pure refactor with no behaviour change. Moving the single-shot parse into a
named helper lets later commits add checks after it without its early returns
skipping them, and lets the framing invariant from #35759 apply to requests
parsed by other means.

56af5e072a

fuzz: allow http_request inputs to reach MAX_HEADERS_SIZE

The input was capped at 4096 bytes, which is below the 8192-byte
MAX_HEADERS_SIZE enforced in HTTPHeaders::Read(), so no input this target
generates could reach the headers size limit. The cap dates back to the
original libevent harness and predates the parser the target now exercises.

d23b53e0cf

DrahtBot added the label Fuzzing on Sep 1, 2026

DrahtBot commented at 4:35 AM on September 1, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36135.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	hodlinator, jeanpablojp

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36124 (http: Make HTTPRequest update state internally by hodlinator)
#35964 (fuzz: add coverage for GetQueryParameterFromUri by laxmanacharya8)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label CI failed on Sep 1, 2026

DrahtBot commented at 6:09 AM on September 1, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task fuzzer,address,undefined,integer: https://github.com/bitcoin/bitcoin/actions/runs/33470407667/job/99738839067</sub> <sub>LLM reason (✨ experimental): CI failed because the http_request libFuzzer run crashed due to a UBSan error (implicit-integer-sign-change) in src/util/sock.cpp line 49.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

frankomosh force-pushed on Sep 1, 2026

frankomosh marked this as a draft on Sep 1, 2026

frankomosh commented at 8:10 AM on September 1, 2026: contributor

Drafting to check CI issues

fanquake requested review from hodlinator on Sep 1, 2026

fanquake requested review from pinheadmz on Sep 1, 2026

fanquake commented at 11:12 AM on September 1, 2026: member

In file included from /home/runner/work/_temp/src/httpserver.h:21,
                 from /home/runner/work/_temp/src/test/fuzz/http_request.cpp:5:
/home/runner/work/_temp/src/util/sock.h:67:19: error: 'virtual Sock& Sock::operator=(Sock&&)' was hidden [-Werror=overloaded-virtual=]
   67 |     virtual Sock& operator=(Sock&& other);
      |                   ^~~~~~~~
/home/runner/work/_temp/src/test/fuzz/http_request.cpp:78:7: note:   by '{anonymous}::NoopSock& {anonymous}::NoopSock::operator=(const {anonymous}::NoopSock&)'
   78 | class NoopSock : public Sock
      |       ^~~~~~~~

frankomosh force-pushed on Sep 1, 2026

DrahtBot removed the label CI failed on Sep 1, 2026

frankomosh commented at 5:22 PM on September 1, 2026: contributor

In file included from /home/runner/work/_temp/src/httpserver.h:21,
                 from /home/runner/work/_temp/src/test/fuzz/http_request.cpp:5:
/home/runner/work/_temp/src/util/sock.h:67:19: error: 'virtual Sock& Sock::operator=(Sock&&)' was hidden [-Werror=overloaded-virtual=]
   67 |     virtual Sock& operator=(Sock&& other);
      |                   ^~~~~~~~
/home/runner/work/_temp/src/test/fuzz/http_request.cpp:78:7: note:   by '{anonymous}::NoopSock& {anonymous}::NoopSock::operator=(const {anonymous}::NoopSock&)'
   78 | class NoopSock : public Sock
      |       ^~~~~~~~

Thanks. Switched to ZeroSock (from test/util/net.h) instead of trying to use a custom mock. It already handles the operator GCC flagged here.

frankomosh marked this as ready for review on Sep 1, 2026

jeanpablojp commented at 10:08 PM on September 1, 2026: contributor

Concept ACK

Built with ASan and UBSan, replayed the qa-assets corpus and fuzzed on top of it without a failure.

in src/test/fuzz/http_request.cpp:97 in 1a004e4ceb outdated

 113 | +    case HTTPRequest::State::Error: return 4;
 114 |      }
 115 | +    assert(false);
 116 | +}
 117 | +
 118 | +struct ParsedRequest {

jeanpablojp commented at 10:08 PM on September 1, 2026:

No headers in here, so a header parsed differently by the one-shot and sliced runs is invisible unless it also changes something the comparison does look at. The m_consumed resume path in HTTPHeaders::Read is new ground for this target, and its output is what gets left out. Worth adding headers? I tried one extra field populated by GetHeader("Host"), and today's qa-assets corpus then catches a mutation that changes a header only on the resumed path.

frankomosh commented at 4:26 PM on September 3, 2026:

Added the Host. Thanks for catching it

in src/test/fuzz/http_request.cpp:213 in 1a004e4ceb outdated

 229 | +FUZZ_TARGET(http_request)
 230 | +{
 231 | +    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
 232 | +    // MAX_HEADERS_SIZE is 8192: leave room for a headers section that can
 233 | +    // reach the limit, plus a body.
 234 | +    const std::string http_buffer{fuzzed_data_provider.ConsumeRandomLengthString(2 * MAX_HEADERS_SIZE)};

jeanpablojp commented at 10:08 PM on September 1, 2026:

nit: nothing in test_runner.py passes -max_len, so libFuzzer caps generated inputs at 4128 bytes for this target, the largest file in the corpus today. That leaves MAX_HEADERS_SIZE out of reach. A bigger seed fixes it. I tried an 8387-byte one, which survives the corpus merge and adds coverage.

frankomosh commented at 4:52 PM on September 3, 2026:

Agreed, and it matches what I also measured. no -max_len means libFuzzer derives its limit from the corpus max (4128 today), so corpus is bounded and thus generation. Raising the cap removes one half of that problem; the other half needs a larger seed in qa-assets. I am not completely sure(or aware of precedent) if adding seeds manually is recommendable but if yes then that would much better be done separately from this PR.

jeanpablojp commented at 9:11 PM on September 3, 2026:

Contributing corpus to qa-assets is routine and you have done it yourself. I could not find a case that was clearly a hand-written seed though, so that part I cannot settle. What I can say is that the small chunked input I wrote by hand survives the same set_cover_merge the corpus is maintained with, and the merge reported far more new coverage from it than from a large generated input.

in src/test/fuzz/http_request.cpp:136 in 1a004e4ceb outdated

 152 | +                assert(cur->GetState() != HTTPRequest::State::Complete);
 153 | +                assert(StateRank(cur->GetState()) >= StateRank(out.last_state));
 154 | +                out.last_state = cur->GetState();
 155 | +                if (cur->GetState() == HTTPRequest::State::Error) out.errored = true;
 156 | +                if (const auto chunk_size{cur->GetChunkSize()}) {
 157 | +                    assert(cur->GetChunkProgress() <= *chunk_size);

jeanpablojp commented at 10:08 PM on September 1, 2026:

No qa-assets input reaches this assertion. I probed the three resume points separately. Header resumption shows up in every qa-assets corpus I tried, chunk resumption in none, and body resumption in one of the 146 inputs, but that request never completes. A mutation that drops a byte on body resume goes unnoticed by the current corpus. Worth sending two seeds, one chunked and one Content-Length request with a complete body?

frankomosh commented at 4:57 PM on September 3, 2026:

This also matches my coverage runs: the chunked branch of LoadBody() gets zero hits from the qa-assets corpus, and none of the inputs I generated contains Transfer-Encoding either. A manual seed injection kills the mutant, as with the case above. I'd be interested if there's a way to reach it without hand-writing one. Trying to see around if thats possible.

jeanpablojp commented at 9:11 PM on September 3, 2026:

I tried. Two ten-minute value-profile runs never produced even the header name, which fits CaseInsensitiveEqual not going through memcmp, so libFuzzer never learns it. Nothing I ran reached this assertion without a hand-written input, so a seed still looks like the way here. Short runs on one box, so treat the negative half as evidence rather than proof.

frankomosh commented at 7:28 AM on September 4, 2026:

Aah..Just seen that there is already a dictionary for this harness in qa-assets (https://github.com/bitcoin-core/qa-assets/blob/main/fuzz_dicts/http_request.dict). So I think the best course of action is that we could do a follow up there to add "chunked" and "Transfer-Encoding: chunked".....

jeanpablojp commented at 12:31 PM on September 4, 2026:

Since that exchange I went at the corpus side and opened bitcoin-core/qa-assets#293 with it. The dictionary I left untouched, so that follow-up is yours. Happy to open it if you'd rather not. Either way it has to be a separate pull request, since the qa-assets lint rejects one that mixes fuzz_corpora with files outside it.

One number from those runs, in case it saves you time: "Transfer-Encoding" is already an entry there, and adding "chunked" beside it stayed out of the branch over 1.79M executions. A small test dictionary that included the whole "Transfer-Encoding: chunked" did get in, and that is the entry I have not tried against the real one.

in src/test/fuzz/http_request.cpp:82 in 1a004e4ceb

  98 | +//! Expose HTTPRemoteClient with the receive buffer.
  99 | +class FuzzClient : public HTTPRemoteClient
 100 | +{
 101 | +public:
 102 | +    FuzzClient() : HTTPRemoteClient{/*id=*/0, /*addr=*/CService(), /*socket=*/CreateSock(0, 0, 0)} {}
 103 | +    void receive(std::string_view s) { MutateRecvBuffer().append(s); }

hodlinator commented at 1:16 PM on September 3, 2026:

Please don't replicate lowercase methods:

    void Receive(std::string_view s) { MutateRecvBuffer().append(s); }

Attempt at making the unit test conform to developer-notes.md: e69331c (#36160)

hodlinator commented at 4:09 PM on September 3, 2026:

Hm.. although @janb84 pointed out the the base class already has a Receive() method.. so a different name might be preferable.. then again I think the name is fine here and the base class method could rather be called something more specific like ReceiveFromSocket(). :\

janb84 commented at 4:29 PM on September 3, 2026:

Agree that renaming the base class is better option ( suggestion, rename the base class to ReceiveBytesFromSocket() to maximizes symmetry with MaybeSendBytesFromBuffer() )

hodlinator commented at 7:09 PM on September 3, 2026:

re #36135 (review):

"bytes" in MaybeSendBytesFromBuffer() seems redundant... itching for another rename to MaybeSendFromBuffer() or MaybeSendToSocket() or MaybeSendBufferToSocket(). :) Also itching to rename GetRecvBuffer() to PeekRecvBuffer() and GetRequest() to PeekRequest(), especially to disambiguate the latter one further from TryReadRequest()...

But with all HTTP PRs in flight let's defer those renames for now - except for the horrible case of MaybeDisconnect() which really really doesn't do what it sounds like (#36159).

Will try not to derail too much for now, grateful the PR author applied my suggestion at the start of the thread.

in src/test/fuzz/http_request.cpp:171 in 1a004e4ceb

 187 | +
 188 | +    // fuzz.cpp points CreateSock at std::terminate(), and a client needs a socket
 189 | +    // even though nothing here connects to anything. Keep it deterministic: a
 190 | +    // FuzzedSock would consume fuzz data and desynchronise the two runs below.
 191 | +    auto CreateSockOrig{CreateSock};
 192 | +    CreateSock = [](int, int, int) { return std::make_unique<ZeroSock>(); };

hodlinator commented at 1:29 PM on September 3, 2026:

Seems like we can skip this dance through:

--- a/src/test/fuzz/http_request.cpp
+++ b/src/test/fuzz/http_request.cpp
@@ -78,7 +78,7 @@ void SingleShotParse(const std::string& http_buffer, FuzzedDataProvider& provide
 class FuzzClient : public HTTPRemoteClient
 {
 public:
-    FuzzClient() : HTTPRemoteClient{/*id=*/0, /*addr=*/CService(), /*socket=*/CreateSock(0, 0, 0)} {}
+    FuzzClient() : HTTPRemoteClient{/*id=*/0, /*addr=*/CService(), /*socket=*/std::make_unique<ZeroSock>()} {}
     void receive(std::string_view s) { MutateRecvBuffer().append(s); }
 };
 
@@ -164,12 +164,6 @@ void CheckSegmentationIndependence(const std::string& input, FuzzedDataProvider&
     FakeNodeClock clock{1610000000s};
     FakeSteadyClock steady_clock;
 
-    // fuzz.cpp points CreateSock at std::terminate(), and a client needs a socket
-    // even though nothing here connects to anything. Keep it deterministic: a
-    // FuzzedSock would consume fuzz data and desynchronise the two runs below.
-    auto CreateSockOrig{CreateSock};
-    CreateSock = [](int, int, int) { return std::make_unique<ZeroSock>(); };
-
     // The whole stream arrives in one I/O cycle.
     RunResult one_shot;
     {
@@ -194,8 +188,6 @@ void CheckSegmentationIndependence(const std::string& input, FuzzedDataProvider&
         sliced.remainder = client->GetRecvBuffer();
     }
 
-    CreateSock = CreateSockOrig;
-
     assert(one_shot.requests == sliced.requests);
     assert(one_shot.errored == sliced.errored);
     // The whole receive buffer is discarded on a parse error, so how much is

frankomosh commented at 4:30 PM on September 3, 2026:

Taken. thanks

in src/test/fuzz/http_request.cpp:202 in 1a004e4ceb

 218 | +    CreateSock = CreateSockOrig;
 219 | +
 220 | +    assert(one_shot.requests == sliced.requests);
 221 | +    assert(one_shot.errored == sliced.errored);
 222 | +    // The whole receive buffer is discarded on a parse error, so how much is
 223 | +    // left over depends on how much had arrived when the error fired.

hodlinator commented at 1:49 PM on September 3, 2026:

    // The whole receive buffer is discarded on a parse error, so in the sliced case
    // what is left over depends on how much had arrived when the error fired.

frankomosh commented at 4:30 PM on September 3, 2026:

Taken. thanks

hodlinator commented at 1:54 PM on September 3, 2026: contributor

Concept ACK 1a004e4cebffac58f767aec7f37ec182c1b3a892

fuzz: exercise the HTTPRequest state machine

The target parses each input in one pass: a single LineReader over the whole
buffer, each Load* called once. A request that arrives over several I/O cycles
takes a different path, which resumes where it left off, and the target could
not reach it.

Feed the same bytes through a client a slice at a time, so parsing has to
resume across cycle boundaries, and check what must hold at each boundary. A
completed request is always handed back, parsing only advances or fails, chunk
progress never exceeds the declared chunk size, a failed request consumes
nothing further, and nothing is parsed or consumed while a request is out with
a worker.

c368de3eec

frankomosh force-pushed on Sep 3, 2026

in src/test/fuzz/http_request.cpp:185 in 1837de9d2d

 201 | +        auto client{std::make_shared<FuzzClient>()};
 202 | +        size_t pos{0};
 203 | +        while (pos < input.size()) {
 204 | +            const size_t n{provider.ConsumeIntegralInRange<size_t>(1, input.size() - pos)};
 205 | +            client->Receive(std::string_view{input}.substr(pos, n));
 206 | +            pos += n;

hodlinator commented at 7:22 PM on September 3, 2026:

nanonit: Have a slight preference for more string_views - it decreases arithmetic but makes L183 loong. Feel free to ignore.

        std::string_view remaining{input};
        while (!remaining.empty()) {
            const std::string_view chunk{remaining.substr(0, provider.ConsumeIntegralInRange<size_t>(1, remaining.size()))};
            client->Receive(chunk);
            remaining = remaining.substr(chunk.size());

frankomosh commented at 7:24 AM on September 4, 2026:

Taken, with a small tweak (slice length pulled into a named variable so the line stays under 100 characters).

hodlinator commented at 11:23 AM on September 4, 2026:

Neat variant!

in src/test/fuzz/http_request.cpp:167 in 1837de9d2d

 183 | +    // WriteReply() stamps a wall-clock Date header and the client stamps a
 184 | +    // steady-clock idle time, both of which the fuzz determinism check rejects.
 185 | +    FakeNodeClock clock{1610000000s};
 186 | +    FakeSteadyClock steady_clock;
 187 | +
 188 | +    // The whole stream arrives in one I/O cycle.

hodlinator commented at 7:55 PM on September 3, 2026:

Could elaborate:

    // The whole stream arrives in one I/O cycle. This is similar to
    // SingleShotParse(), although here we pipe it through the client and also
    // parse multiple requests.

frankomosh commented at 7:23 AM on September 4, 2026:

updated. Thanks

hodlinator commented at 7:57 PM on September 3, 2026: contributor

Reviewed 1837de9d2d8994738d8f4ea8be085db05979d064

fuzz: assert HTTP framing is independent of stream segmentation

Framing is defined by the byte stream, so how TCP happened to split it must
not change what the parser produces. Run the same input twice, once delivered
whole and once in arbitrary slices, and require both runs to agree on the
requests parsed, their order, and whether parsing failed.

The leftover receive buffer is only compared when neither run failed: the
whole buffer is discarded on a parse error, so how much had arrived by then
legitimately differs between the two runs.

fca8fef8f9

frankomosh force-pushed on Sep 4, 2026

frankomosh commented at 7:22 AM on September 4, 2026: contributor

New update mainly on style nits.

hodlinator approved

hodlinator commented at 11:40 AM on September 4, 2026: contributor

ACK fca8fef8f903d841df693d18e4b9078f0867e07f

Should™️ provide more coverage of HTTPRemoteClient as well as adding comparison of the resulting requests when the input is sliced up and when it's not (the slice points being defined by the fuzz input itself).

https://corecheck.dev/bitcoin/bitcoin/pulls/36135 doesn't show any significant difference in coverage, probably it needs an updated fuzz corpus.

DrahtBot requested review from jeanpablojp on Sep 4, 2026

jeanpablojp commented at 1:46 PM on September 4, 2026: contributor

tACK fca8fef8f903d841df693d18e4b9078f0867e07f

Rebuilt with ASan and UBSan, replayed the corpus and fuzzed on top of it without a failure. Mutating the resume paths trips the new assertions.

fuzz: test HTTPRequest state machine in http_request #36135

Code Coverage & Benchmarks

Reviews

Conflicts