Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty frame cause panic #88

Closed
chrislearn opened this issue Feb 18, 2023 · 5 comments · Fixed by #92
Closed

Empty frame cause panic #88

chrislearn opened this issue Feb 18, 2023 · 5 comments · Fixed by #92

Comments

@chrislearn
Copy link

chrislearn commented Feb 18, 2023

self.bufs.push(data);

debug_assert!(buf.has_remaining());

When frame is empty, this cause panic, just wondering if this is expected behavior?
Some HTTP/2 implementations may product empty frames, when I create a reverse proxy server,and call body.collect().await.to_bytes() to read data, server will give error in debug mode. Or just give a warning here?

chrislearn added a commit to salvo-rs/salvo that referenced this issue Feb 18, 2023
@seanmonstar
Copy link
Member

The collect combinator should probably filter out the empty data frames, to prevent the panic.

@chrislearn
Copy link
Author

pub(crate) fn push_frame(&mut self, frame: Frame<B>) {
let frame = match frame.into_data() {
Ok(data) => {
self.bufs.push(data);
return;
}
Err(frame) => frame,
};

In push_frame, just push data to bufs and does not check is data is empty.

impl<T: Body + ?Sized> Future for Collect<T> {
type Output = Result<crate::Collected<T::Data>, T::Error>;
fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> std::task::Poll<Self::Output> {
let mut me = self.project();
loop {
let frame = futures_util::ready!(me.body.as_mut().poll_frame(cx));
let frame = if let Some(frame) = frame {
frame?
} else {
return Poll::Ready(Ok(me.collected.take().expect("polled after complete")));
};
me.collected.as_mut().unwrap().push_frame(frame);
}
}
}

In Collect's poll function, call push_frame directly and push the frame.

@meowjesty
Copy link
Contributor

Hi what's the status on this? Anyone working on adding this change?

Had to make this change for our project, can send a PR if no one is working on this.

@davidpdrsn
Copy link
Member

No I don't think anyone is working on it. You're welcome to submit a PR.

meowjesty added a commit to meowjesty/http-body that referenced this issue Apr 3, 2023
seanmonstar pushed a commit that referenced this issue Apr 21, 2023
* Fixes issue #88 (empty frame panic).

* Better check for empty frame.
@aviramha
Copy link

Hey, we'd love to use an officially released version. Is it possible to please create a release? @seanmonstar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants