panic: 'attempt to subtract with overflow'

[hyperium]

panic: 'attempt to subtract with overflow'

[olix0r]

I think there are two separate issues here:

1. Stream ID accounting is buggy, which causes the connection to go into protocol error and go away.
2. Go away handling is also buggy, which causes the panic.

More on stream id accounting bug, in h2::proto::streams::recv:

    /// Update state reflecting a new, remotely opened stream
    ///
    /// Returns the stream state if successful. `None` if refused
    pub fn open(
        &mut self,
        id: StreamId,
        counts: &mut Counts<P>,
    ) -> Result<Option<StreamId>, RecvError> {
        assert!(self.refused.is_none());

        self.ensure_can_open(id)?;

        let next_id = self.next_stream_id()?;
        if id < next_id {
            return Err(RecvError::Connection(ProtocolError));
        }

        self.next_stream_id = id.next_id();

        if !counts.can_inc_num_recv_streams() {
            self.refused = Some(id);
            return Ok(None);
        }

        Ok(Some(id))
    }

    /// Transition the stream state based on receiving headers
    ///
    /// The caller ensures that the frame represents headers and not trailers.
    pub fn recv_headers(
        &mut self,
        frame: frame::Headers,
        stream: &mut store::Ptr<B, P>,
        counts: &mut Counts<P>,
    ) -> Result<(), RecvError> {
        trace!("opening stream; init_window={}", self.init_window_sz);
        let is_initial = stream.state.recv_open(frame.is_end_stream())?;

        if is_initial {
            let id = frame.stream_id();
            let next_id = self.next_stream_id()?;
            if id >= next_id {
                self.next_stream_id = id.next_id();
            } else {
                error!("invalid id: {:?} < {:?}", id, next_id);
                return Err(RecvError::Connection(ProtocolError));
            }

            ...

Both open and recv_headers attempt to update self.next_stream_id, which causes newly received headers to be interpreted as invalid.

In h2::proto::streams::streams::Streams::recv_headers, we see that both Recv::open and Recv::recv_headers are called as a stream is initialized:

    /// Process inbound headers
    pub fn recv_headers(&mut self, frame: frame::Headers) -> Result<(), RecvError> {
        let id = frame.stream_id();
        let mut me = self.inner.lock().unwrap();
        let me = &mut *me;

        let key = match me.store.find_entry(id) {
            Entry::Occupied(e) => e.key(),
            Entry::Vacant(e) => match me.actions.recv.open(id, &mut me.counts)? {
                Some(stream_id) => {
                    let stream = Stream::new(
                        stream_id,
                        me.actions.send.init_window_sz(),
                        me.actions.recv.init_window_sz(),
                    );

                    e.insert(stream)
                },
                None => return Ok(()),
            },
        };

        let stream = me.store.resolve(key);
        let actions = &mut me.actions;

        me.counts.transition(stream, |counts, stream| {
            trace!(
                "recv_headers; stream={:?}; state={:?}",
                stream.id,
                stream.state
            );

            let res = if stream.state.is_recv_headers() {
                actions.recv.recv_headers(frame, stream, counts)
            } else ...

So, it seems that at least one of the following should be changed:

1. open should not update the next stream id
2. recv_headers should not check the next stream id
3. the is_initial checkin recv_headers is wrong

I think this also means we must not be testing the Streams::recv_headers call path.

[olix0r]

d0bb3cb implements a serverside test that illustrates this problem.