test: add LineReader edge cases relevant to HTTP parsing #35135

pull b-l-u-e wants to merge 1 commits into bitcoin:master from b-l-u-e:test/linereader-http-edge-cases changing 1 files +13 −0

b-l-u-e commented at 9:18 PM on April 21, 2026: contributor
This PR adds two small test cases to line_reader_test in src/test/util_string_tests.cpp to lock in util::LineReader behavior that upcoming HTTP request parsing will depend on.

What the new cases check:
- CRLF and trailing bare \r Consecutive CRLFs produce an empty line (this is how HTTP signals end of headers). A lone \r at the end of the buffer is not treated as a line terminator, so the rest can still be read with ReadLength().
- State after a max_line_length exception After the exception, the iterator is reset and both Consumed() and Remaining() reflect that reset. This means callers can safely fall back to ReadLength() without losing position.
LineReader was added in #34242 (commit 1911db8c) with the max_line_length + iterator-reset semantics exercised here. Consumed() was added later in #34905 (commit 81720992) so callers could observe position after an exception.

These utilities are consumed by the in-progress libevent replacement (#32061). Adding these edge cases reduces risk and ambiguity
DrahtBot added the label Tests on Apr 21, 2026
DrahtBot commented at 9:18 PM on April 21, 2026: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

Concept ACK fjahr

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
fanquake commented at 8:10 AM on April 22, 2026: member

cc @fjahr @pinheadmz
in src/test/util_string_tests.cpp:305 in 72a8cfc31e
```
 300 | +    {
 301 | +        // Consumed/Remaining stay in sync after a max_line_length exception
 302 | +        std::string_view input = "toolong\nok\n";
 303 | +        LineReader reader(input, /*max_line_length=*/3);
 304 | +        BOOST_CHECK_EXCEPTION(reader.ReadLine(), std::runtime_error, HasReason{"max_line_length exceeded by LineReader"});
 305 | +        BOOST_CHECK_EQUAL(reader.Consumed(), 0);
```
fjahr commented at 10:01 AM on April 22, 2026:

I think this test is pretty weak since the error occurs on the first line and the progress stays a 0, which could, for example, also mean that the progress is always reset to 0. There is also some redundancy here and I would suggest you instead add this coverage to the existing test starting at L246, while making sure this doesn't just cover the Consumed() == 0 case.

b-l-u-e commented at 6:00 PM on April 22, 2026:

thanks i will fold the consumed() and remaining() rewind check into the existing "baboon" block to be asserted at non-zero positon and remove the redundancy block
in src/test/util_string_tests.cpp:298 in 72a8cfc31e
```
 293 | +        // Empty line between headers and body
 294 | +        BOOST_CHECK_EQUAL(reader.ReadLine(), "");
 295 | +        // Bare \r at end of buffer is not a line terminator
 296 | +        BOOST_CHECK(!reader.ReadLine());
 297 | +        BOOST_CHECK_EQUAL(reader.Remaining(), 5);
 298 | +        BOOST_CHECK_EQUAL(reader.ReadLength(5), "body\r");
```
fjahr commented at 10:03 AM on April 22, 2026:

Could drop these lines since they don't add new coverage and don't contribute to the case above afaict. Could also drop body\r from the input string along with it then.

b-l-u-e commented at 5:57 PM on April 22, 2026:

how about simplifying the input to just "example.com\r\n\r\n\r" drop the Remaining() assertion and keep single ReadLength(1) == "\r" to just document that trailing bare \r that trigggers a rewind so the byte stays drainable maybe

fjahr commented at 6:11 PM on April 22, 2026:

I would prefer my suggestion honestly

b-l-u-e commented at 7:42 PM on April 22, 2026:

Done simplified CRLF empty-line assertion and dropped the bare-\r + ReadLength follow-up
fjahr commented at 10:08 AM on April 22, 2026: contributor

Concept ACK

Looks to add a little bit of additional coverage but it could be simplified.
pinheadmz commented at 5:34 PM on April 22, 2026: member

I'm ~0 about this PR. What new code regressions could be introduced that would be caught by this PR? In other words, can you write a patch for master branch that does not break any existing LineReader tests, but should?

Type	Reviewers
Concept ACK	fjahr

test: add LineReader edge cases relevant to HTTP parsing

Add two small test cases to line_reader_test that lock in
current behavior LineReader-based HTTP parsing will depend on:

- Consecutive CRLFs produce an empty line and a trailing bare \r
  is not treated as a line terminator.
- Consumed()/Remaining() stay consistent with the reset iterator
  after a max_line_length exception, so ReadLength() can be used
  to recover.

Signed-off-by: b-l-u-e <8102260+blue@users.noreply.github.com>

efe2c34773

b-l-u-e force-pushed on Apr 22, 2026
b-l-u-e commented at 9:04 PM on April 22, 2026: contributor

I'm ~0 about this PR. What new code regressions could be introduced that would be caught by this PR? In other words, can you write a patch for master branch that does not break any existing LineReader tests, but should?

yeah you right i used candidate regression if (line.empty()) return std::nullopt;

https://github.com/bitcoin/bitcoin/blob/8a05adc5f81da483fb4e1b3592acb3e274014298/src/util/string.cpp#L38-L41

i see is already caught here https://github.com/bitcoin/bitcoin/blob/8a05adc5f81da483fb4e1b3592acb3e274014298/src/test/util_string_tests.cpp#L219-L225

Thanks for review the added coverage was redundant with existing tests so closing
b-l-u-e closed this on Apr 22, 2026

Contributors

Labels

Linked (view graph)

#32061 Replace libevent with our own HTTP and socket-handling implementation #34242 Prepare string and net utils for future HTTP operations #34905 Update string and net utils for future HTTP operations

test: add LineReader edge cases relevant to HTTP parsing #35135

Reviews