Skip to content

Commit ab6f148

Browse files
committed
Fix potential hang if extensions contain same StreamRef
If a user stored a `StreamRef` to the same stream in the request or response extensions, they would be dropped while the internal store lock was held. That would lead to a deadlock, since dropping a stream ref will try to take the store lock to clean up. Clear extensions of Request and Response before locking store, prevent this. Fixes hyperium/hyper#2621
1 parent 288a5f0 commit ab6f148

2 files changed

Lines changed: 47 additions & 3 deletions

File tree

src/proto/streams/streams.rs

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -207,13 +207,16 @@ where
207207

208208
pub fn send_request(
209209
&mut self,
210-
request: Request<()>,
210+
mut request: Request<()>,
211211
end_of_stream: bool,
212212
pending: Option<&OpaqueStreamRef>,
213213
) -> Result<StreamRef<B>, SendError> {
214214
use super::stream::ContentLength;
215215
use http::Method;
216216

217+
// Clear before taking lock, incase extensions contain a StreamRef.
218+
request.extensions_mut().clear();
219+
217220
// TODO: There is a hazard with assigning a stream ID before the
218221
// prioritize layer. If prioritization reorders new streams, this
219222
// implicitly closes the earlier stream IDs.
@@ -1062,9 +1065,11 @@ impl<B> StreamRef<B> {
10621065

10631066
pub fn send_response(
10641067
&mut self,
1065-
response: Response<()>,
1068+
mut response: Response<()>,
10661069
end_of_stream: bool,
10671070
) -> Result<(), UserError> {
1071+
// Clear before taking lock, incase extensions contain a StreamRef.
1072+
response.extensions_mut().clear();
10681073
let mut me = self.opaque.inner.lock().unwrap();
10691074
let me = &mut *me;
10701075

@@ -1082,7 +1087,12 @@ impl<B> StreamRef<B> {
10821087
})
10831088
}
10841089

1085-
pub fn send_push_promise(&mut self, request: Request<()>) -> Result<StreamRef<B>, UserError> {
1090+
pub fn send_push_promise(
1091+
&mut self,
1092+
mut request: Request<()>,
1093+
) -> Result<StreamRef<B>, UserError> {
1094+
// Clear before taking lock, incase extensions contain a StreamRef.
1095+
request.extensions_mut().clear();
10861096
let mut me = self.opaque.inner.lock().unwrap();
10871097
let me = &mut *me;
10881098

tests/h2-tests/tests/server.rs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1059,3 +1059,37 @@ async fn request_without_authority() {
10591059

10601060
join(client, srv).await;
10611061
}
1062+
1063+
#[tokio::test]
1064+
async fn serve_when_request_in_response_extensions() {
1065+
h2_support::trace_init!();
1066+
let (io, mut client) = mock::new();
1067+
1068+
let client = async move {
1069+
let settings = client.assert_server_handshake().await;
1070+
assert_default_settings!(settings);
1071+
client
1072+
.send_frame(
1073+
frames::headers(1)
1074+
.request("GET", "https://example.com/")
1075+
.eos(),
1076+
)
1077+
.await;
1078+
client
1079+
.recv_frame(frames::headers(1).response(200).eos())
1080+
.await;
1081+
};
1082+
1083+
let srv = async move {
1084+
let mut srv = server::handshake(io).await.expect("handshake");
1085+
let (req, mut stream) = srv.next().await.unwrap().unwrap();
1086+
1087+
let mut rsp = http::Response::new(());
1088+
rsp.extensions_mut().insert(req);
1089+
stream.send_response(rsp, true).unwrap();
1090+
1091+
assert!(srv.next().await.is_none());
1092+
};
1093+
1094+
join(client, srv).await;
1095+
}

0 commit comments

Comments
 (0)