varnish-cache icon indicating copy to clipboard operation
varnish-cache copied to clipboard

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

Open nigoroll opened this issue 3 years ago • 5 comments

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

nigoroll avatar May 30 '22 13:05 nigoroll

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.

bsdphk avatar May 30 '22 13:05 bsdphk

phk OK via bugwash

nigoroll avatar Aug 15 '22 13:08 nigoroll

martin said -1

nigoroll avatar Aug 15 '22 13:08 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.

nigoroll avatar Aug 15 '22 14:08 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 https://github.com/varnishcache/varnish-cache/pull/3811#issuecomment-1215050989, 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

nigoroll avatar Aug 16 '22 13:08 nigoroll