From 0b289fd55db99405c4b75d6467aedffe8d87ca84 Mon Sep 17 00:00:00 2001
From: Oliver Gould <ver@buoyant.io>
Date: Tue, 26 Sep 2017 10:42:12 -0700
Subject: [PATCH] Fix stream-id double-accounting bug (#112)

Both Recv::open and Rev::recv_headers check new stream ids against the
previously stream id. The second such check fails.

Now, only Recv::open performs stream id checks.

Fixes #110
---
 src/proto/streams/recv.rs  |  7 -----
 src/proto/streams/state.rs |  5 ++--
 tests/server.rs            | 33 +++++++++++++++++++++
 tests/support/mock.rs      | 60 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+), 10 deletions(-)

diff --git a/src/proto/streams/recv.rs b/src/proto/streams/recv.rs
index ce7de74..7b0ebdd 100644
--- a/src/proto/streams/recv.rs
+++ b/src/proto/streams/recv.rs
@@ -137,13 +137,6 @@ where
         let is_initial = stream.state.recv_open(frame.is_end_stream())?;
 
         if is_initial {
-            let next_id = self.next_stream_id()?;
-            if frame.stream_id() >= next_id {
-                self.next_stream_id = frame.stream_id().next_id();
-            } else {
-                return Err(RecvError::Connection(ProtocolError));
-            }
-
             // TODO: be smarter about this logic
             if frame.stream_id() > self.last_processed_id {
                 self.last_processed_id = frame.stream_id();
diff --git a/src/proto/streams/state.rs b/src/proto/streams/state.rs
index b0dde1c..4588677 100644
--- a/src/proto/streams/state.rs
+++ b/src/proto/streams/state.rs
@@ -116,10 +116,9 @@ impl State {
         return Ok(());
     }
 
-    /// Open the receive have of the stream, this action is taken when a HEADERS
-    /// frame is received.
+    /// Opens the receive-half of the stream when a HEADERS frame is received.
     ///
-    /// Returns true if this transitions the state to Open
+    /// Returns true if this transitions the state to Open.
     pub fn recv_open(&mut self, eos: bool) -> Result<bool, RecvError> {
         let remote = Peer::Streaming;
         let mut initial = false;
diff --git a/tests/server.rs b/tests/server.rs
index f7181e3..5530f04 100644
--- a/tests/server.rs
+++ b/tests/server.rs
@@ -22,6 +22,39 @@ fn read_preface_in_multiple_frames() {
     assert!(Stream::wait(h2).next().is_none());
 }
 
+#[test]
+fn serve_request() {
+    let _ = ::env_logger::init();
+    let (io, client) = mock::new();
+
+    let client = client
+        .assert_server_handshake()
+        .unwrap()
+        .recv_settings()
+        .send_frame(
+            frames::headers(1)
+                .request("GET", "https://example.com/")
+                .eos(),
+        )
+        .recv_frame(frames::headers(1).response(200).eos())
+        .close();
+
+    let srv = Server::handshake(io).expect("handshake").and_then(|srv| {
+        srv.into_future().unwrap().and_then(|(reqstream, srv)| {
+            let (req, mut stream) = reqstream.unwrap();
+
+            assert_eq!(req.method(), &http::Method::GET);
+
+            let rsp = http::Response::builder().status(200).body(()).unwrap();
+            stream.send_response(rsp, true).unwrap();
+
+            srv.into_future().unwrap()
+        })
+    });
+
+    srv.join(client).wait().expect("wait");
+}
+
 #[test]
 #[ignore]
 fn accept_with_pending_connections_after_socket_close() {}
diff --git a/tests/support/mock.rs b/tests/support/mock.rs
index 60fb640..ee59fe6 100644
--- a/tests/support/mock.rs
+++ b/tests/support/mock.rs
@@ -162,6 +162,66 @@ impl Handle {
 
         Box::new(ret)
     }
+
+
+    /// Perform the H2 handshake
+    pub fn assert_server_handshake(
+        self,
+    ) -> Box<Future<Item = (frame::Settings, Self), Error = h2::Error>> {
+        self.assert_server_handshake_with_settings(frame::Settings::default())
+    }
+
+    /// Perform the H2 handshake
+    pub fn assert_server_handshake_with_settings<T>(
+        mut self,
+        settings: T,
+    ) -> Box<Future<Item = (frame::Settings, Self), Error = h2::Error>>
+    where
+        T: Into<frame::Settings>,
+    {
+        self.write_preface();
+
+        let settings = settings.into();
+        self.send(settings.into()).unwrap();
+
+        let ret = self.into_future()
+            .unwrap()
+            .map(|(frame, mut me)| {
+                match frame {
+                    Some(Frame::Settings(settings)) => {
+                        // Send the ACK
+                        let ack = frame::Settings::ack();
+
+                        // TODO: Don't unwrap?
+                        me.send(ack.into()).unwrap();
+
+                        (settings, me)
+                    },
+                    Some(frame) => {
+                        panic!("unexpected frame; frame={:?}", frame);
+                    },
+                    None => {
+                        panic!("unexpected EOF");
+                    },
+                }
+            })
+            .then(|res| {
+                let (settings, me) = res.unwrap();
+
+                me.into_future()
+                    .map_err(|e| panic!("error: {:?}", e))
+                    .map(|(frame, me)| {
+                        let f = assert_settings!(frame.unwrap());
+
+                        // Is ACK
+                        assert!(f.is_ack());
+
+                        (settings, me)
+                    })
+            });
+
+        Box::new(ret)
+    }
 }
 
 impl Stream for Handle {