h3 icon indicating copy to clipboard operation
h3 copied to clipboard

poll_read data is buffered without bounds

Open jvimal opened this issue 7 months ago • 1 comments

https://github.com/hyperium/h3/blob/84316da426d1825a871cd850ff4f90aa3fe5860b/h3/src/stream.rs#L433

The poll_read() on BufRecvStream (wrapping the underlying QUIC provider's stream) is buffering chunks in-memory in a BufList that appears to grows without bounds:

https://github.com/hyperium/h3/blob/84316da426d1825a871cd850ff4f90aa3fe5860b/h3/src/stream.rs#L448-L450

In some tests that I ran over loopback to send and receive data as fast as possible, if the client does not consume from BufRecvStream quickly (for any reason) I observed that the client memory grows without bounds. This should ideally not be the case -- a slow consumer should back-pressure the sender. This problem does not exist if I use the underlying QUIC provider's stream directly.

I don't have a handy reproducer that I can share at the moment, but I can work on one if it'll be useful.

jvimal avatar May 19 '25 22:05 jvimal

Thanks for reporting this.

Looking at the code, it could happen, when the first chunk of memory read from the quic stream holds multiple http3 frames. Then for each frame it tries to read from the buffer again adding a potential received memory chunk to the buffer for each read. This only happens when the server sends faster then the reader reads.

IMO the best way to solve this is to get rid of buffer BufList. The purpose of BufList, is to buffer data when a read chunk of memory holds more than one frame. IIRC, another purpose is to hold the data until a complete frame is arrived. Removing the BufList would require to refactor the Frame parsing to a kind of stateful parser in case a read chunk holds only a part of the data. Then only 1 memory chunk would need to be buffered. The most Frames hold only protocol data so it should be possible without extra buffering. The exception:

  • Data: The data in Data Frames goes directly to the user, so no buffering required.
  • Header: Header data needs to be buffered, but the specification already introduces a limit here: See MAX_FIELD_SECTION_SIZE settings parameter. Note, the MAX_FIELD_SECTION_SIZE setting is not applied correctly at the moment. See #294. I am working on a solution for this in #296

Ruben2424 avatar May 24 '25 19:05 Ruben2424