How to correctly buffer an HTTP/2 request into memory?

[zacknewman]

A cursory read of RFC 9113 seems to suggest that a request stream is closed iff a frame with END_STREAM set has been received. § 6.1 also states that an empty DATA frame with END_STREAM set is allowed to indicate the stream is over.

1. A literal interpretation of the RFC suggests that empty DATA frames without END_STREAM set is allowed. Does hyper/h2 have some mechanism that checks for "an empty DATA frame attack"? For example a threshold of consecutive empty DATA frames that when exceeded causes an error?
2. Body::is_end_stream presumably is based on a frame with END_STREAM set having been received. The documentation for that function explicitly states that Body::poll_frame can return Poll::Ready(None) even when this returns false. This, in addition to my interpretation of RFC 9113, suggests to me that the body of a request that is to be buffered into memory should be based on this bool and not based on when None is received; however http_body_util::combinators::Collect::poll seems to (mis)interpret None as end of stream. Shouldn't Collect::poll instead return Poll::Ready iff T::is_end_stream returns true? Perhaps it can have a threshold of consecutive Nones that when exceeded causes an error to be returned.

[seanmonstar]

I thought we had used the word "hint" in the description, because that's what is_end_stream is. It's a hint, like size_hint. It's not required to be implemented. If it does return true, then hyper may optimize such that the last data frame can include the END_STREAM flag. If it returns false, then None from poll_frame is interpreted as the end of the stream (this is based on the same behavior of futures_core::Stream).

[zacknewman]

The documentation for both Body::is_end_stream and Incoming::is_end_stream lacks any mention of it being a hint—in fact Incoming::is_end_stream has the stronger comment:

Returns true when the end of stream has been reached.

RFC 9113 seemingly requires END_STREAM to be set for a stream to be interpreted as closed. My question is about how Incoming::poll_frame returning Poll::Ready(None) relates to this flag. When using Incoming::poll_frame on the HTTP/2 server to read a request, is it possible it returns Poll::Ready(None) without ever having received a frame with END_STREAM set? If so, then interpreting None as "request data is over" violates RFC 9113 since more frames can still be sent. Of course if h2 is implemented in such a way that it only returns None iff a frame with END_STREAM has been received, then it's fine.

For example the below is what I implemented as an alternative to Incoming::collect().await?.to_bytes(). Note self.incoming is an Incoming and not a generic Body. The fact the documentation for Incoming::is_end_stream is overridden as "Returns true when the end of stream has been reached" makes it seem plausible that Incoming::is_end_stream iff Incoming::poll_frame returns Poll::Ready(None); however I decide to conservatively rely on Body::is_end_stream's documentation instead (i.e., Incoming::is_end_stream is entirely dependent on END_STREAM and Incoming::poll_frame returns None either because END_STREAM has been received or some unknown reason).

impl Future for BufferedRequest {
    type Output = Result<Bytes, BufReqErr>;
    fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        const BAD_DATA_THRESHOLD: u8 = 5u8;
        let mut bad_data_count = 0u8;
        while !self.incoming.is_end_stream() {
            match Pin::new(&mut self.incoming).poll_frame(cx) {
                Poll::Ready(o) => {
                    if let Some(r) = o {
                        match r {
                            Ok(f) => match f.into_data() {
                                Ok(d) => {
                                    // RFC 9113 doesn't explicitly forbid empty DATA frames
                                    // that don't have END_STREAM set, but perhaps that's
                                    // a too literal reading. We decide to allow it since
                                    // we increment `bad_data_count`.
                                    if d.is_empty() {
                                        if bad_data_count == BAD_DATA_THRESHOLD {
                                            // We received 5 consecutive empty DATA frames or `None`.
                                            self.data.set_to_none();
                                            return Poll::Ready(Err(BufReqErr::BadData));
                                        } else {
                                            // Not sure why we received an empty DATA frame, but RFC 9113 requires END_STREAM
                                            // to be set iff the stream is over so we continue `poll`ing.
                                            bad_data_count += 1;
                                        }
                                    } else {
                                        // We received a "good" DATA frame (i.e., neither `None`
                                        // nor an empty DATA frame was sent); thus we reset the counter.
                                        bad_data_count = 0;
                                        self.data.add_bytes(d);
                                    }
                                }
                                Err(_) => {
                                    self.data.set_to_none();
                                    return Poll::Ready(Err(BufReqErr::Trailers));
                                }
                            },
                            Err(e) => {
                                self.data.set_to_none();
                                return Poll::Ready(Err(BufReqErr::Hyper(e)));
                            }
                        }
                    } else if bad_data_count == BAD_DATA_THRESHOLD {
                        // We received 5 consecutive empty DATA frames or `None`.
                        self.data.set_to_none();
                        return Poll::Ready(Err(BufReqErr::BadData));
                    } else {
                        // Not sure why we received `None`, but RFC 9113 requires END_STREAM
                        // to be set iff the stream is over so we keep `poll`ing.
                        bad_data_count += 1;
                    }
                }
                Poll::Pending => return Poll::Pending,
            }
        }
        Poll::Ready(Ok(self.data.take_as_bytes()))
    }
}

Ignoring the application-specific addition of forbidding trailer frames, this implementation differs with the implementation of Collect<Incoming>::poll; however since I don't maintain hyper/h2, I'm curious if my implementation is unnecessarily strict (e.g., I don't allow more than 5 consecutive None or empty DATA frames which RFC 9113 doesn't expressly forbid) or permissive (e.g., I do allow empty DATA frames without END_STREAM set because RFC 9113 doesn't expressly forbid it but perhaps it was intended for such frames to be forbidden).

For example, should I even bother checking if a DATA frame is empty; or does h2 forbid empty DATA frames already unless it has END_STREAM set? Do I need to worry about None being sent? I'm not sure why h2 would return Poll::Ready(None) and not Poll::Pending when no frames have been received but the request stream is not over (i.e., END_STREAM has not been received).

If I can assume None does mean END_STREAM was set and empty DATA frames without END_STREAM are already forbidden by hyper/h2, then the above code is simpler:

impl Future for BufferedRequest {
    type Output = Result<Bytes, BufReqErr>;
    fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        while let Poll::Ready(o) = Pin::new(&mut self.incoming).poll_frame(cx) {
            if let Some(r) = o {
                match r {
                    Ok(f) => match f.into_data() {
                        Ok(d) => self.data.add_bytes(d),
                        Err(_) => {
                            self.data.set_to_none();
                            return Poll::Ready(Err(BufReqErr::Trailers));
                        }
                    },
                    Err(e) => {
                        self.data.set_to_none();
                        return Poll::Ready(Err(BufReqErr::Hyper(e)));
                    }
                }
            } else {
                return Poll::Ready(Ok(self.data.take_as_bytes()));
            }
        }
        Poll::Pending
    }
}

Essentially I want RFC 9113-conforming behavior with some small protection against an "empty DATA frame attack"; however I do not know how to interpret None as it pertains to END_STREAM nor do I know if hyper/h2 already prevents or has some threshold against empty DATA frames without END_STREAM.

[seanmonstar]

The docs on Incoming seem to be just the first sentence, with a "Read more" link. I bet that rustdoc now does that for all implementations, allowing the read more to point at the longer description.

I've filed hyperium/http-body#143 to improve the docs to show that it is indeed just a hint.

[zacknewman]

I'll just assume that None is returned iff END_STREAM has been received.