- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 297
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
Fix for a fuzzer-discovered integer underflow of the flow control window size #692
Conversation
self.inner.streams.set_target_connection_window_size(size); | ||
let _res = self.inner.streams.set_target_connection_window_size(size); | ||
// TODO: proper error handling | ||
debug_assert!(_res.is_ok()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some debug left around here and in other files. Are these on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Whenever it was obvious on how to handle a potential overflow, I did. However, many of the call sites do not have a Result
return type, so it is not easy to propagate the Error. I assume that those functions should not be able not fail. I did not want to incur additional overhead in release builds, so I stuck with debug_assert!
and left the TODO
comment. For those TODOs someone with more experience with the code should check whether they are fine to be left as debug_assert!
, whether they should be checked with assert!
or whether error propagation is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: I did more fuzzing runs with this patch and none of the debug_assert!
are triggered, so that gives some assurance that the they are indeed not reachable. So I think we could remove the todo comments, but I would leave the debug_assert!
as is.
ead98f1
to
09d4849
Compare
…dow size Removed the SubAssign, etc. syntactic sugar functions and switched to return Result on over/underflow Whenever possible, switched to returning a library GoAway protocol error. Otherwise we check for over/underflow only with `debug_assert!`, assuming that those code paths do not over/underflow. Signed-off-by: Michael Rodler <mrodler@amazon.de> Co-Authored-By: f0rki <m@mrodler.eu> Reviewed-by: Daniele Ahmed <ahmeddan@amazon.de>
09d4849
to
eb10db0
Compare
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@f0rki is it ready for review? |
looks good to me @82marbag @seanmonstar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, thank you!
…dow size (hyperium#692) Removed the SubAssign, etc. syntactic sugar functions and switched to return Result on over/underflow Whenever possible, switched to returning a library GoAway protocol error. Otherwise we check for over/underflow only with `debug_assert!`, assuming that those code paths do not over/underflow. Signed-off-by: Michael Rodler <mrodler@amazon.de> Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de> Co-authored-by: Michael Rodler <mrodler@amazon.de> Co-authored-by: Daniele Ahmed <ahmeddan@amazon.de>
…dow size (hyperium#692) Removed the SubAssign, etc. syntactic sugar functions and switched to return Result on over/underflow Whenever possible, switched to returning a library GoAway protocol error. Otherwise we check for over/underflow only with `debug_assert!`, assuming that those code paths do not over/underflow. Signed-off-by: Michael Rodler <mrodler@amazon.de> Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de> Co-authored-by: Michael Rodler <mrodler@amazon.de> Co-authored-by: Daniele Ahmed <ahmeddan@amazon.de>
…dow size (hyperium#692) Removed the SubAssign, etc. syntactic sugar functions and switched to return Result on over/underflow Whenever possible, switched to returning a library GoAway protocol error. Otherwise we check for over/underflow only with `debug_assert!`, assuming that those code paths do not over/underflow. Signed-off-by: Michael Rodler <mrodler@amazon.de> Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de> Co-authored-by: Michael Rodler <mrodler@amazon.de> Co-authored-by: Daniele Ahmed <ahmeddan@amazon.de> Signed-off-by: Sven Pfennig <s.pfennig@reply.de>
In an effort to understand a bit more what's going on in #607, I've been looking at the test from this PR to see if the root cause could be the same here. I've managed to simplify the test there a great deal, which led me to this (I'll make a PR simplifying it, separately): #[tokio::test]
async fn window_size_decremented_past_zero() {
h2_support::trace_init!();
let (io, mut client) = mock::new();
let client = async move {
let settings = client.assert_server_handshake().await;
assert_default_settings!(settings);
// HEADERS with invalid hpack stuff.
client
.send_bytes(&[
0, 0, 23, 1, 1, 0, 0, 0, 1, 131, 1, 1, 1, 70, 1, 1, 1, 1, 65, 1, 1, 65, 1, 1,
65, 1, 1, 1, 1, 1, 1, 190,
])
.await;
client.send_frame(frames::settings().initial_window_size(666413898)).await;
client.send_frame(frames::settings().initial_window_size(3809661)).await;
client.send_frame(frames::settings().initial_window_size(666413898)).await;
client.send_frame(frames::settings().initial_window_size(3809661)).await;
client.send_frame(frames::settings().initial_window_size(1467177332)).await;
client.send_frame(frames::settings().initial_window_size(3844989)).await;
};
let srv = async move {
let builder = server::Builder::new();
let mut srv = builder.handshake::<_, Bytes>(io).await.expect("handshake");
// just keep it open
let res = poll_fn(move |cx| srv.poll_closed(cx)).await;
tracing::debug!("{:?}", res);
};
join(client, srv).await;
} The test fails if we put back an assertion in So I looked into what happens when the initial window size setting is incremented or decremented, and I've found something suspicious. Lines 469 to 480 in 3bce93e
When a stream is already closed and the initial window size setting is incremented, nothing happens to the stream because it is already closed. h2/src/proto/streams/prioritize.rs Lines 306 to 309 in 3bce93e
But when the initial window size setting is decremented, the stream window size is also decremented, so there is an imbalance there. To double check my finding, I managed to reduce the number of frames to cause the assertion failure with: client.send_frame(frames::settings().initial_window_size(1329018135)).await;
client.send_frame(frames::settings().initial_window_size(3809661)).await;
client.send_frame(frames::settings().initial_window_size(1467177332)).await;
client.send_frame(frames::settings().initial_window_size(3844989)).await; Note that |
This helps finding the root cause of why the assertion in FlowControl::dec_send_window failed in the first place. See #692 for more details.
This helps finding the root cause of why the assertion in FlowControl::dec_send_window failed in the first place. See #692 for more details.
…erium#25) This helps finding the root cause of why the assertion in FlowControl::dec_send_window failed in the first place. See hyperium#692 for more details. Co-authored-by: Anthony Ramine <123095+nox@users.noreply.github.com>
Fuzzing discovered a integer underflow in the flow control handling that can be triggered by a certain sequence of frames. In release builds this would lead to wrap-around of the negative window. This seems incorrect to me. I did not find anything definitive in the http2 spec.
Removed the SubAssign, etc. syntactic sugar functions and switched to return Result on over/underflow
Whenever possible, switched to returning a library GoAway protocol error. Otherwise we check for over/underflow only with
debug_assert!
, assuming that those code paths do not over/underflow. In this case I leftTODO:
comments.