Skip to content

bytesReadCount is one less than the bytes consumed, and the doc says otherwise #58

Description

@MDA2AV

TryExtractFullHeader* reports one less than the header block's real size. The value is initialised to -1 and then incremented by the total:

// UltraHardenedParser.FullHeader.ROM.cs:33
bytesReadCount = -1;
...
// UltraHardenedParser.FullHeader.ROM.cs:295
bytesReadCount += totalHeaderBytes;
return true;

FlexibleParser.FullHeader.ROM.cs (lines 17 and 119) does the same, so both managed tiers share it.

Why this is worth fixing rather than documenting

The XML doc states the opposite of what the code does:

<param name="bytesReadCount">Bytes consumed on success, or -1 if incomplete.</param>

It is not the bytes consumed. It is the bytes consumed minus one.

The C ABI promises the correct value for the same concept. From core/include/glyph11.h:

consumed : optional out-param; on GLYPH11_OK, set to the number of bytes up to and including the terminating CRLFCRLF.

So the two implementations of one library disagree about the meaning of the same output, and core/diff/ has to reconcile them by hand:

r.Consumed = br + 1; // C# returns total-1; the C ABI returns the clean total

That line is the only reason the differential harness passes. It is a known, permanent divergence being normalised away — which is the opposite of what a parity harness is for (see also #57).

The workaround has already reached the shipped examples, so users are being taught the wart rather than shielded from it:

// Examples/Glyph11.Example/Program.cs:68
// bytesRead follows glyph11's -1 convention: the body begins at bytesRead + 1.
Console.WriteLine($"  header block length = {bytesRead + 1} bytes");

The migration problem

This is the part that makes it awkward, and why it should not be a quiet patch release.

Every current consumer computes bytesRead + 1 to find where the body starts. If the convention is corrected in place, all of that code silently skips a byte: the first body byte is consumed as though it were still header. Nothing fails to compile, no exception is thrown, and a Content-Length body arrives one byte short with the missing byte prepended to whatever comes next. On a keep-alive connection that is a desync — the failure class this library exists to prevent.

So the danger is not that it breaks; it is that it breaks silently, at runtime, in the framing layer.

Options

  1. Fix in place, minor/major bump, loud changelog. Cleanest end state, but the failure mode for anyone who does not read the release notes is silent body corruption.
  2. Introduce a correctly-specified output alongside the current one — an overload whose out-param is the true total — mark the existing signature [Obsolete] with a message naming the difference, and remove it a release later. The migration then shows up at compile time, which is the only place it can be seen. This also lets the C and C# sides converge on one meaning instead of two.
  3. Leave the behaviour, fix the documentation. Cheapest, keeps the divergence and the br + 1 fudge in the diff harness forever.

I would take 2: the break is invisible at runtime, so it needs to be made visible at build time.

Note on #56

The response parser in #56 mirrors this convention deliberately — a response parser that consumed differently from the request parser would be a worse trap than the wart itself. It should be corrected in the same change as the request parsers, not separately. ioxide.httpclient currently compensates with:

// Glyph11 reports one less than the block's real size - see the note in its diff harness,
// "C# returns total-1; the C ABI returns the clean total".
int headSize = bytesRead + 1;

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions