Why a parser cursor has to stay failed after the first refusal

2026-11-01 · 8 min read

A C++ parser for a binary format fails in one of two ways: it trusts a length field and reads past the end of its buffer, or it has a bounds check on every single read and nobody keeps that up. The second failure is the more interesting one, because people agree with the discipline right up until they skip it. A record header is ten reads, and honest per-read checking means you write the same conditional ten times to reach one return std::nullopt.

I have spent most of the last decade on payment and crypto systems, so most of my parsing work has been on framed messages arriving off a socket from something I do not control. The conclusion I keep arriving at is not more checks. It is a cursor that stays failed once it has refused anything, so the ten checks collapse into one question you ask at the end. That is the whole premise of bytespan, a header-only C++20 cursor I wrote for exactly this shape of problem.

The check everybody agrees with and then skips#

Written out by hand, a correct parse of a three-field header looks roughly like this:

std::size_t offset = 0;

if (input.size() - offset < 1) return std::nullopt;
std::uint8_t type{};
std::memcpy(&type, input.data() + offset, 1);
offset += 1;

if (input.size() - offset < 2) return std::nullopt;
std::uint16_t version{};
std::memcpy(&version, input.data() + offset, 2);
offset += 2;
// ...and again, for every field

Nothing is wrong with this except that it does not survive contact with a real format. By the sixth field the conditional has been copied five times, and a copy is where input.size() - offset < n turns into offset + n < input.size(), or where the offset += n goes missing, or where the whole guard gets skipped because the field three lines up already proved there were bytes left. I have inherited this code more than once during legacy modernisation work. The bug is never interesting: it is the same bug wearing a different field name.

So the design question is not how to check a read. It is how to make a parse of ten fields cost one check without being wrong.

Two early returns#

Every consuming operation in bytespan's reader goes through one private method:

bool take(std::size_t n) noexcept {
    if (!ok()) return false;
    if (n > data_.size() - offset_) {
        poison({offset_, reason::short_input, n, data_.size() - offset_});
        return false;
    }
    offset_ += n;
    return true;
}

Those two early returns are the entire mechanism. The second one is the ordinary bounds check. The first is what changes the economics: once the cursor is poisoned, take refuses before it looks at anything, so every later read fails too. u8, u16, u32 and u64 all go through a read<T> that calls take(sizeof(T)) before its memcpy and returns std::nullopt on refusal; bytes and skip call take directly. No path advances the cursor without passing the check, and no path passes the check on a cursor that has already failed.

What you get at the call site is the shape from the repository's own example:

std::optional<record> parse(bytespan::reader& r) {
    record out{};
    const auto type = r.u8();
    const auto version = r.u16(bytespan::endian::big);
    const auto payload = r.length_prefixed_u16(bytespan::endian::big);

    if (!r.ok()) return std::nullopt;
    out.type = *type;
    out.version = *version;
    out.payload = *payload;
    return out;
}

The three dereferences after the check are safe, and not by convention. Stickiness makes "ok() is true" equivalent to "every read on this cursor succeeded", because the first failure would still be latched when the check runs. Without stickiness that equivalence does not hold and the single check is a lie: you would have to test each optional, which is the ten-conditional version again with worse ergonomics.

Note also that take does not move offset_ when it refuses. The cursor never ends up positioned past bytes it did not actually read, which matters if the caller wants to log offset() or resume. That decision has a consequence of its own, and it is the one people miss.

The loop that would otherwise spin#

A cursor that refuses without advancing is a cursor whose position no longer changes. Drive a loop off the byte count and you get an infinite loop:

std::vector<std::span<const std::byte>> extensions;
while (r.remaining() > 0) {
    if (const auto body = r.length_prefixed_u16(bytespan::endian::big)) {
        extensions.push_back(*body);
    }
}
if (!r.ok()) return std::nullopt;

This terminates, and it terminates for a reason you cannot see from the loop body: remaining() returns 0 once the cursor has failed, instead of data_.size() - offset_. Sticky failure and a non-advancing cursor are each individually correct and together they produce a hang, so the third piece is mandatory rather than a convenience. It is also what lets the loop above contain no error handling at all: a malformed extension list ends the loop, and the single ok() afterwards says the parse is dead. A refused length-prefixed read additionally resets offset_ back to the start of its prefix, so there is genuinely nothing left in the cursor that could make progress.

The writer mirrors all three properties: its own take compares against out_.size() - offset_, its remaining() is zero once it has failed, and put and bytes refuse after the first overrun. An overrun fails the writer rather than the process, and stays failed.

The bound is a subtraction, and that is not cosmetic#

n > data_.size() - offset_ and offset_ + n > data_.size() are the same statement in arithmetic and different statements in C++. The left-hand form subtracts; the invariant that offset_ only ever advances through a successful take means offset_ <= data_.size() holds, so the subtraction cannot underflow. The right-hand form adds, and n is frequently attacker-controlled: it comes out of the length field you just read. On a platform where std::size_t is 32 bits, a declared length near the top of the range makes offset_ + n wrap to something small, the comparison passes, and the memcpy runs for the full unwrapped count. The check becomes the exploit.

That is why the length-prefixed read can cast a 32-bit count straight to std::size_t and compare it:

const std::size_t wanted = static_cast<std::size_t>(*length);
if (wanted > data_.size() - offset_) {
    poison({start, reason::short_input, detail::saturating_add(sizeof(T), wanted),
            data_.size() - start});
    offset_ = start;
    return std::nullopt;
}
offset_ += wanted;

The prefix and the payload are one call because splitting them is where the second check gets forgotten, and the refusal is recorded against start (the first byte of the prefix) so it names the field rather than a position inside it. The demo output shows what that reads like: a four-byte input whose prefix claims 65535 bytes of payload reports refused at 3: short input (wanted 65537, had 3). The 65537 is the two-byte count folded into what the field as a whole needed.

Keeping the first refusal, not the last#

ok() decides control flow; it does not write the log line. So the cursor keeps a record, offset, reason, bytes wanted, bytes available, and poison only writes it when there is nothing there yet:

void poison(refusal record) noexcept {
    if (!refusal_) refusal_ = record;
}

That ordering falls out of stickiness. After the first failure, every later call fails as a consequence, and those consequences are uninformative: a header that died on a truncated u32 must not end up reporting the single-byte read that followed it. Last-write-wins would bury the only refusal that explains the parse. The writer's tests pin the same property from the other side: an overrun at offset 2 wanting four bytes with one left keeps saying exactly that, even after a smaller u8 and an empty bytes have both failed behind it.

What I would do differently#

One thing the sticky cursor makes harder, and I have not found a clean way around it: it does not distinguish "this message is truncated, wait for more bytes" from "this message is malformed, drop the peer". Both arrive as short_input, which is correct, the parser genuinely cannot tell, but a streaming caller has to make that call from the refusal's offset and sizes plus knowledge of its own framing. If I were starting again I would probably still refuse to guess inside the library, because a cursor that speculates about your transport is a cursor that is wrong on somebody else's transport. But the record is the only thing standing between the caller and a bad decision there, which is a good argument for it carrying sizes rather than just a reason.

The other cost is that [[nodiscard]] optionals plus a run-then-check style means binding values you will never read. const auto version = r.u16(...) exists to be dereferenced after the check, and in a parse where you only care about presence it is noise. I accepted that; the alternative is dropping [[nodiscard]], which removes the compiler's only warning about an ignored read on a cursor you forgot to check.

The discipline that survives review is the one that costs a single line. Everything above is in service of that line being if (!r.ok()), and of it meaning exactly what it appears to mean.