V1F: Read end-of-chunk as part of the chunk

[nigoroll]

Taken from #3809 as requested by @bsdphk

Until now, we read the (CR)?LF at the end of a chunk as part of the next chunk header (see: /* Skip leading whitespace */).

For a follow up commit, we are going to want to know if the next chunk header is available for read, so we now consume the chunk end as part of the chunk itself.

This also fixes a corner case: We previously accepted chunks with a missing end-of-chunk (see fix of r01729.vtc).

Ref: https://datatracker.ietf.org/doc/html/rfc7230#section-4.1

[bsdphk]

I'm pretty sure we implemented it the current way to cater to some bogus implementation of chunked, but that would be more than 14 years ago, so one would hope that it no longer applies.

[dridi]

Could you maybe split this change into two commits? One to move the end-of-chunk parsing to its own function, and one to use that function in another code path (with the test case update).

[nigoroll]

phk OK via bugwash

[nigoroll]

martin said -1

[nigoroll]

@mbgrydeland raised the concern that waiting for the chunk end [CR]LF could increase latency, in particular if that final byte was sent in another packet.
So maybe a middle ground could be to have a flag set by VFPs downstream if they need an accurate VFP_END? This way, the request body caching code could still use it, leaving the existing code unaffected.

[mbgrydeland]

I don't like the change of behaviour this patch brings with it. Up to now we have always passed on the bytes as soon as we receive them. With this patch, we will change behaviour at the chunk boundries and require to see the chunk end before passing the last byte on up the VFP stack.

This will possibly have a minor latency increase in some corner cases (last chunk byte and chunk end in different packets). But more importantly, for (silly?) backends not sending the end chunk bytes right away in a long-polling scenario appear as a stuck connection.

Also for an outside observer, and where there may be proxies outside of their control involved, the difference in behaviour at different byte boundries will make for hard debugging.

I think a cleaner way to achieve the wanted outcome would be to introduce a new state for the chunked VFP. Ie priv2 == -2, we are waiting for the chunk end, and only after having received that do we set priv2 == -1 to indicate we are to parse the next chunked header. With this the VFP wouldn't have to block waiting for the chunk end, but know to require it first on the next call.

[nigoroll]

@mbgrydeland thank you for your constructive suggestion, something like this seems sensible to require the [CR]LF at the end of the chunk for the case where VFP_END is not required with the last bytes.

As laid out in #3811 (comment), I think we do need a way to get VFP_END signaled with the last bytes for partial request body caching, see #3809

@mbgrydeland what do you think about this?

• Fix the existing v1f_chunked along the lines of what you suggested
• Add a v1f_chunked_strict variant which implements the strict VFP_END signaling