Subject: Suspected errata on 7540 wrt stream states

From: Willy Tarreau <[email-masked]>
Date: Wed, 27 Dec 2017 09:57:53 +0100

Hi all,

while taking a closer look at some of the issues reported by h2spec on
haproxy's h2 implementation, I found that it reported something that I
thought I had properly handled, namely the receipt of frames after the
ES flag. Taking a closer look, I implemented what is indicated in 5.1 :

   open:
      (...) an endpoint receiving an END_STREAM flag causes the
      stream state to become "half-closed (remote)".

   half-closed (remote):
      (...) If an endpoint receives additional frames, other than
      WINDOW_UPDATE, PRIORITY, or RST_STREAM, for a stream that is in
      this state, it MUST respond with a stream error (Section 5.4.2) of
      type STREAM_CLOSED.

And indeed, when haproxy sees, say, a DATA frame after ES, it sends an
RST_STREAM with error type STREAM_CLOSED.

But h2spec expects to see a connection error here, which surprized me a
bit. So I continued to read and found what rule it relies on :

   closed:
      (...) Similarly, an endpoint
      that receives any frames after receiving a frame with the
      END_STREAM flag set MUST treat that as a connection error
      (Section 5.4.1) of type STREAM_CLOSED, unless the frame is
      permitted as described below.

So in fact here I suspect that this last paragraph has nothing to do with
the closed state but is in fact related to the half-closed(remote) one,
and it contradicts the rule dictated in the half-closed(remote) description.

Am I overlooking anything ? Should I implement a connection error to be
more conservative ? Should I file an errata given that we're seeing two
conflicting MUST ?

Thanks,
Willy

---

From: Ilari Liusvaara <[email-masked]>
Date: Wed, 27 Dec 2017 15:19:51 +0200

> more conservative ? Should I file an errata given that we're seeing two

One of the rules is about half-open state and the other is about closed
state.

So if RST_STREAM is not involved, it depends on if you have sent
END_STREAM.

1) If no, it is stream error.
2) If yes, it is connection error.

(Not that this makes very much sense, but at least the spec does not
seem to conflict with itself).

Then there is more whacky case if RST_STREAM is involved:

If you have sent RST_STREAM and then receive END_STREAM and then DATA
(all in "short" succession), that is no error.

-Ilari

---

From: Willy Tarreau <[email-masked]>
Date: Wed, 27 Dec 2017 14:54:46 +0100

Hi Ilari,

> One of the rules is about half-open state and the other is about closed

But there are not that many ways to reach the closed state :
  - END_STREAM in both directions
  - RST_STREAM in any direction

Also by definition, a closed stream is ... closed. So when you receive a
frame for such a stream most of the time you don't remember what condition
caused it to be closed.

> 2) If yes, it is connection error.

But if you haven't sent END_STREAM (case 1), the only way it may end
up in closed state is by receiving RST_STREAM, which contradicts the
condition :-)

> (all in "short" succession), that is no error.

Well, I'm starting to think that I will fall back all these situations to
connection error, given that a stream error may always be elevated to a
connection error. It will cover all these cases at once.

Willy

---

From: Ilari Liusvaara <[email-masked]>
Date: Wed, 27 Dec 2017 16:55:52 +0200

> frame for such a stream most of the time you don't remember what condition

The spec requires distinguishing:

1) Stream was "recently" fully closed by RST_STREAM you sent.
2) Stream was "recently" fully closed by END_STREAM you sent (impiles
   that the other side has sent END_STREAM before).
3) Any other case (not "recently" closed, or remote side fully closed
   it via RST_STREAM or END_STREAM).

There is no upper bound what "recently" means. I would expect at least
tens of seconds if not minutes.

In case 1)

- PUSH_PROMISE is processed normally.
- PRIORITY is processed normally.
- DATA updates connection window, but is otherwise ignored.
- All other frames are ignored.

In case 2)

- PRIORITY is processed normally.
- WINDOW_UPDATE is ignored.
- RST_STREAM is ignored.
- All other frames trigger connection error.

In case 3)

- PRIORITY is processed normally.
- All other frames trigger connection error.

> up in closed state is by receiving RST_STREAM, which contradicts the

Case 1 is half-closed case. Case 2 is full close.

> connection error. It will cover all these cases at once.

Just beware that you don't send connection errors on any possibly racy
case. Because such implementation is buggy and such spec is errorneous.

-Ilari

---

From: Willy Tarreau <[email-masked]>
Date: Wed, 27 Dec 2017 16:14:24 +0100

>    it via RST_STREAM or END_STREAM).

That's exactly what I'm doing with various stream-specific flags reporting
what was seen on each of them.

> tens of seconds if not minutes.

Well for a gateway even tens of seconds is not practical as it can quickly
lead to millions or tens of millions of closed streams stored in memory.
In my case I still have the ability to proceed with "recently" seen streams
that have not yet been purged (ie those not yet closed at the application
level), which is enough to try to do what is best most of the time.

> - All other frames trigger connection error.

Well I think that's what I'm trying to aim for. In addition to this, 5.1.1
mandates that HEADERS and PUSH_PROMISE always trigger a connection error
on closed streams regardless of how they were closed :

   The identifier of a newly established stream MUST be numerically
   greater than all streams that the initiating endpoint has opened or
   reserved.  This governs streams that are opened using a HEADERS frame
   and streams that are reserved using PUSH_PROMISE.  An endpoint that
   receives an unexpected stream identifier MUST respond with a
   connection error (Section 5.4.1) of type PROTOCOL_ERROR.

As you can see this automatically covers closed streams since their IDs
are numerically lower than the next allowed one by definition.

> Case 1 is half-closed case. Case 2 is full close.

Yes I agree, which is why we can't have case 1 in closed state. That's
what I meant, which led me to consider that its specific rule being
listed as part of the "closed" stream was not much relevant there and
more to the half-closed one.

> case. Because such implementation is buggy and such spec is errorneous.

Yes I've noticed this after writing it and preferred to rollback on this.
If I don't have the stream in the local list I'm not back to sending RST
to avoid such race. However I'm trying hard to send the GOAWAY when I'm
absolutely certain the peer did something not allowed (ie I still have
the stream in the list and the stream was closed normally).

I'm trying to stick as close as possible to the spec without risking to
fall on the unreliable side.

Thanks!
Willy

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 28 Dec 2017 05:19:16 +0900

Hi,

> more conservative ? Should I file an errata given that we're seeing two

Let me point out that section 6.1 also has a definition dealing with the
case, which states:

   If a DATA frame is received
   whose stream is not in "open" or "half-closed (local)" state, the
   recipient MUST respond with a stream error (Section 5.4.2
<https://tools.ietf.org/html/rfc7540#section-5.4.2>) of type
   STREAM_CLOSED.

So I agree that there might be an issue here. My personal bet would be that
this is an error in section 5.1 failing to nominate DATA frame as one of
the frames that can end up in a stream-level error when received in closed
state.

FWIW, h2spec version 1 does not report an error if a stream-level error is
returned in this case. So it might be a regression in h2spec version 2.

-- 
Kazuho Oku

---

From: Willy Tarreau <[email-masked]>
Date: Thu, 28 Dec 2017 07:19:01 +0100

Hi Kazuho,

> <https://tools.ietf.org/html/rfc7540#section-5.4.2>) of type

Good point, and in fact it has always been a concern to me that rules
were dictated at different places (ie: in stream states and in frame
types). I think we made a mistake by proceeding like this because it's
almost impossible to maintain the exact same rules on the two sides,
we possibly ought to have used general guidelines in the states list
(i.e. "frames received in that state commonly lead to a stream error
and sometimes to a connection error" etc). Then for each frame type,
the exhaustive list of states should be enumerated.

> the frames that can end up in a stream-level error when received in closed

In fact it totally makes sense to me to abort DATA with a stream error
and that's what I used to do, that h2spec didn't catch as it focuses
on 5.1 expecting a connection error. However HEADERS or PUSH_PROMISE
should definitely end up in a connection error (as defined in 5.1.1
for stream ID reuse), regardless of whether the stream was recently
closed or not.

> returned in this case. So it might be a regression in h2spec version 2.

I wouldn't call that a regression, just a closer compliance with the
various rules in the spec, which is quite good. It's just that certain
areas in the spec do not totally agree :-)

Cheers,
Willy