Subject: Issue with the H2 state machine

From: Willy Tarreau <[email-masked]>
Date: Fri, 21 Jul 2017 10:17:44 +0200

Hi,

I'm having issues with the H2 state machine, it doesn't differenciate the
direction enough, explicitly allowing some incorrect transitions that
caused me some long head-scratching sessions. One of them is the following :

    IDLE
      |
      | recv HEADERS
      V
    OPEN
      |
      | recv ES
      V
    HALF
   CLOSED
   REMOTE
      |
      :

Here the spec explicitly says regarding DATA frames in 6.1 :

  DATA frames are subject to flow control and can only be sent when a
  stream is in the "open" or "half-closed (remote)" state.
      
Then 6.2 for the HEADERS frame :

   HEADERS frames can be sent on a stream in the "idle", "reserved (local)",
   "open", or "half-closed (remote)" state.

This causes the fact that if we stick to the text, it's perfectly valid
to receive a request, stay in OPEN or go to the HALF-CLOSED(REMOTE) states
depending on the presence of ES on the received HEADERS frame, then decide
to send DATA frames without a prior HEADERS frame, and even to send a
HEADERS frame after the DATA frames if desired. And I didn't find anything
in the text allow the client to reject such a malformed response.

I'm not seeing anything explicitly forbidding to send this sequence. I'm
obviously not interested in doing it, but I found it while trying to use
the stream state to know what I have to do when getting back to processing
a stream, and found that each time I adapted my model differently, I broke
it another way. I'm thinking about adding sub-states to deal with this here,
but I wanted to check if 1) I'm overlooking something (or am completely
stupid) and 2) if some implementations sticking strictly to the specified
state machine could possibly face issues when dealing with such sequences.

Cheers,
Willy

---

From: Willy Tarreau <[email-masked]>
Date: Fri, 21 Jul 2017 10:39:41 +0200

On Fri, Jul 21, 2017 at 10:17:44AM +0200, Willy Tarreau wrote:
(...)
> state machine could possibly face issues when dealing with such sequences.

Finally I found that "8.1 HTTP Req/Resp Exchange" more or less addresses
it by remining a normal sequence though without being very strict on what
to do when it doesn't match (lack of HEADERS frame not mentionned for
example). I don't have much better to propose for now however.

Also I think I found a typo here in 8.1 :

   An endpoint that receives a HEADERS
   frame without the END_STREAM flag set after receiving a final (non-
   informational) status code MUST treat the corresponding request or
   response as malformed (Section 8.1.2.6).

I think it should use s/END_STREAM/END_HEADERS/ given that DATA frames
are still allowed here. I can file an errata if confirmed.

Willy

---

From: Ilari Liusvaara <[email-masked]>
Date: Fri, 21 Jul 2017 11:57:20 +0300

> are still allowed here. I can file an errata if confirmed.

I think it is correct, just somewhat confusing. This seem to be
requirement that there can be at most one header block at the very end
after headers that sends the final status code.

And yes, section 8.1 is pretty clear that the structure of response
stream is.

-Ilari

---

From: Willy Tarreau <[email-masked]>
Date: Fri, 21 Jul 2017 11:03:17 +0200

> after headers that sends the final status code.

But if a DATA frame follows, the END_STREAM is on the DATA frame, not the
headers frame. There's even exactly this example in 8.1.3 page 59. That's
indeed a bit confusing :-/

Willy

---

From: Ilari Liusvaara <[email-masked]>
Date: Fri, 21 Jul 2017 12:33:29 +0300

> indeed a bit confusing :-/

Those examples are written in bit confusing way. In the last example,
the END_STREAM flag on the DATA frame is NOT set. However, the
END_STREAM flag on the last HEADERS frame IS set.

The last example could be written as:

HEADERS(END_HEADERS):
:status => 100
extension-field => bar

HEADERS(END_HEADERS)
:status => 200
content-length => 123
content-type => image/jpeg
trailer => Foo

DATA()
{binary data}

HEADERS(END_STREAM,END_HEADERS)
foo = bar

Also, END_STREAM in HEADERS is bit special in that if
HEADERS has END_STREAM but not END_HEADERS, then the stream
will go on a bit to receive the CONTINUATION frames until
END_HEADERS is asserted.

Example:

HEADERS(END_HEADERS):
:status => 100
extension-field => bar

HEADERS(END_HEADERS)
:status => 200
content-length => 123
content-type => image/jpeg
trailer => Foo, Zot

DATA()
{binary data}

HEADERS(END_STREAM)
foo = bar

CONTINUATION(END_HEADERS)
zot = qux

<-- The half-stream is closed here.

(But I think CONTINUATION SHOULD NOT be used...).

-Ilari

---

From: Willy Tarreau <[email-masked]>
Date: Fri, 21 Jul 2017 16:26:50 +0200

> <-- The half-stream is closed here.

Sorry I added confusion myself, what I meant is that it's the last frame
of the stream which holds the END_STREAM and not the HEADERS frame of the
last HEADERS block. Look at the previous example on the same page, it
properly carries END_STREAM on DATA, not on HEADERS.

My point is that END_STREAM should be on the last frame half-closing the
STREAM unless the last frame is a CONTINUATION frame in which case the
flag is in the previous HEADERS frame (with CONTINUATION being considered
as an extra fragment of HEADERS frame).

If we had END_STREAM on the HEADERS frame even before DATA as 8.1 seems
to mention, there would be no way to know whether or not DATA frames are
expected.

However if we rewrite the sentence in 8.1 like this :

   An endpoint that receives a HEADERS frame without end END_HEADERS
   flag set after receiving a final (non-informational) status code
   MUST treat the corresponding request or response as malformed.

Then it totally makes sense given that the HEADERS frame must be
terminated before seeing a DATA frame.

Willy

---

From: Mike Bishop <[email-masked]>
Date: Mon, 24 Jul 2017 17:16:42 +0000

No, because that would prohibit ridiculously large trailers (i.e. using CONTINUATION), which might be stupid but are permitted.  The spec is saying that any HEADERS frame after the final status code has to be trailers, and therefore that block is the last thing on the stream.  Alternatively stated, you can't send your trailers but then send more body.

-----Original Message-----
From: Willy Tarreau [mailto:[email-masked]] 
Sent: Friday, July 21, 2017 7:27 AM
To: Ilari Liusvaara <[email-masked]>
Cc: [email-masked]
Subject: Re: Issue with the H2 state machine

On Fri, Jul 21, 2017 at 12:33:29PM +0300, Ilari Liusvaara wrote:
> > > > I think it should use s/END_STREAM/END_HEADERS/ given that DATA 
> > > > frames are still allowed here. I can file an errata if confirmed.
> > > 
> > > requirement that there can be at most one header block at the very 
> > > end after headers that sends the final status code.
> > 
> > But if a DATA frame follows, the END_STREAM is on the DATA frame, 
> > not the headers frame. There's even exactly this example in 8.1.3 
> > page 59. That's indeed a bit confusing :-/
> 
> Also, END_STREAM in HEADERS is bit special in that if HEADERS has 
> END_STREAM but not END_HEADERS, then the stream will go on a bit to 
> receive the CONTINUATION frames until END_HEADERS is asserted.
> 
> Example:
> 
> HEADERS(END_HEADERS):
> :status => 100
> extension-field => bar
> 
> HEADERS(END_HEADERS)
> :status => 200
> content-length => 123

Sorry I added confusion myself, what I meant is that it's the last frame of the stream which holds the END_STREAM and not the HEADERS frame of the last HEADERS block. Look at the previous example on the same page, it properly carries END_STREAM on DATA, not on HEADERS.

My point is that END_STREAM should be on the last frame half-closing the STREAM unless the last frame is a CONTINUATION frame in which case the flag is in the previous HEADERS frame (with CONTINUATION being considered as an extra fragment of HEADERS frame).

If we had END_STREAM on the HEADERS frame even before DATA as 8.1 seems to mention, there would be no way to know whether or not DATA frames are expected.

Then it totally makes sense given that the HEADERS frame must be terminated before seeing a DATA frame.

Willy

---

From: Willy Tarreau <[email-masked]>
Date: Tue, 25 Jul 2017 07:14:59 +0200

Hi Mike,

On Mon, Jul 24, 2017 at 05:16:42PM +0000, Mike Bishop wrote:
> No, because that would prohibit ridiculously large trailers (i.e. using
> CONTINUATION), which might be stupid but are permitted.  The spec is saying
> that any HEADERS frame after the final status code has to be trailers, and
> therefore that block is the last thing on the stream.  Alternatively stated,
> you can't send your trailers but then send more body.

Thanks, now I understand my mistake. I did not realize that the ES bit had
to be put on the HEADERS frame even if there are CONTINUATION frames but now
I found where it's mentionned and all of this makes sense. It'll require some
changes to my state machine, but that's exactly why I prefered to be sure to
be able to deal with CONTINUATION frames from the start ;-)

Cheers,
Willy

---

From: Mike Bishop <[email-masked]>
Date: Tue, 25 Jul 2017 16:37:08 +0000

No problem, I know it's a little weird.  The logic is that, because the HEADERS frame can't be processed individually, HEADERS+CONTINUATION get treated as a single very large HEADERS frame.

-----Original Message-----
From: Willy Tarreau [mailto:[email-masked]] 
Sent: Monday, July 24, 2017 10:15 PM
To: Mike Bishop <[email-masked]>
Cc: Ilari Liusvaara <[email-masked]>; [email-masked]
Subject: Re: Issue with the H2 state machine

Hi Mike,

On Mon, Jul 24, 2017 at 05:16:42PM +0000, Mike Bishop wrote:
> No, because that would prohibit ridiculously large trailers (i.e. 
> using CONTINUATION), which might be stupid but are permitted.  The 
> spec is saying that any HEADERS frame after the final status code has 
> to be trailers, and therefore that block is the last thing on the 
> stream.  Alternatively stated, you can't send your trailers but then send more body.

Thanks, now I understand my mistake. I did not realize that the ES bit had to be put on the HEADERS frame even if there are CONTINUATION frames but now I found where it's mentionned and all of this makes sense. It'll require some changes to my state machine, but that's exactly why I prefered to be sure to be able to deal with CONTINUATION frames from the start ;-)

Cheers,
Willy