Subject: Conflicting MUST in RFC7540

From: Willy Tarreau <[email-masked]>
Date: Thu, 24 Jan 2019 06:44:06 +0100

Hi,

I was occasionally seeing a random report from h2spec on haproxy about an
inappropriate RST_STREAM sent by haproxy in response to a CONTINUATION
frame after END_STREAM. I looked at the spec and there are indeed some
conflicting MUST. It's not the first time I notice this but this one is
relatively clear. I'm not sure how we should handle these ones.

Basically, half of the protocol rules are defined in the list of states,
suggesting what to do if some frames are seen, and the other ones are
defined in the list of frames, suggesting how they interact with other
frames.

Here the test (http2/6.10/5) does this :

      HEADERS (ES=1)
      CONTINUATION (EH=1)
      CONTINUATION (EH=1)

After receiving the first CONTINUATION frame, the stream becomes
half-closed (remote). The rule in 5.1 for this state says :

      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.

This is what haproxy does before entering the frame parsing loop. But
later in 6.10 the rule for CONTINUATION frame says :

   A CONTINUATION frame MUST be preceded by a HEADERS, PUSH_PROMISE or
   CONTINUATION frame without the END_HEADERS flag set.  A recipient
   that observes violation of this rule MUST respond with a connection
   error (Section 5.4.1) of type PROTOCOL_ERROR.

This is what h2spec is looking for and haproxy indeed checks this inside
the frame parsing loop.

Depending on the timing and how frames are packed together, we hit either
the first or the second condition.

I'd be inclined to consider that errors in response to any frame which might
have resulted in a compression state inconsistency must always be treated as
a connection error. As such at every place where we recommend to produce a
stream error, we should add "or connection error if the frame is of type
HEADERS, PUSH_PROMISE, or CONTINUATION". Ideally the state-dependent rules
should only be defined per state, and the rules for each frame should only
be related to the frame's format and not its relation to other frames.

What do you think ? Am I missing something ? Should I file an errata ?

Thanks,
Willy

---

From: Martin Thomson <[email-masked]>
Date: Thu, 24 Jan 2019 18:29:54 +1100

Hi Willy,

It looks like you have a classic case of the error precedence problem, loved the world over by test engineers.  The CONTINUATION frame here is clearly in error in two different ways and which error you hit could depend on the structure of your code.

Code that performs processing on every byte in order might hit the type byte first and perform checks for CONTINUATION.  Similarly, if you handle CONTINUATION at the framing layer, you might do this as well.  These stacks might conclude that a PROTOCOL_ERROR is the right answer because the preceding frame was not an open HEADERS or CONTINUATION.

Code that parses the frame header and attributes frames to streams first might generate the STREAM_CLOSED error because the associated stream is closed.

The problem is that both are right: that error exists and they are obligated to report it.  Sending both error codes isn't really a good idea because it probably violates other MUST statements, so you have to pick one.

It looks like we did a reasonable job of avoiding this sort of problem in h2, probably as a result of not using many different error codes (Mike can consider h3 to be on notice here :), but this one is probably worth some text on.  I'm inclined to address the problem generically rather than specifically.  We might not have many other cases of this in RFC 7540, but I can't guarantee that some h2 extension won't create a similar sort of conflict when two error cases collide.

My view is that there is no correct answer.  There are two errors and we shouldn't ask implementations to conform here.  Implementations should be able to choose any structure they like that meets protocol requirements AND they should be allowed to report the first error they encounter.  To that end, if there is an erratum, I would prefer that it say something like this, added to Section 5.4 (Error Handling):

This specification or its extensions could define error conditions that result in conflicting requirements about which error code is sent when multiple errors are present.  Implementations MAY respond to the first error condition that they detect.  Consequently, if multiple errors are present, different implementations could produce different error codes.  However, a connection error MUST NOT be suppressed as a result of reporting a stream error.

Cheers,
Martin

---

From: Willy Tarreau <[email-masked]>
Date: Thu, 24 Jan 2019 08:49:56 +0100

Hi Martin,

On Thu, Jan 24, 2019 at 06:29:54PM +1100, Martin Thomson wrote:
> Hi Willy,
> 
> It looks like you have a classic case of the error precedence problem,
> loved the world over by test engineers.

Absolutely!

> The CONTINUATION frame here is clearly in error in two different ways and
> which error you hit could depend on the structure of your code.
> 
> Code that performs processing on every byte in order might hit the type byte
> first and perform checks for CONTINUATION.  Similarly, if you handle
> CONTINUATION at the framing layer, you might do this as well.  These stacks
> might conclude that a PROTOCOL_ERROR is the right answer because the
> preceding frame was not an open HEADERS or CONTINUATION.
> 
> Code that parses the frame header and attributes frames to streams first
> might generate the STREAM_CLOSED error because the associated stream is
> closed.

These are indeed the two cases that happen depending on the context.

> The problem is that both are right: that error exists and they are obligated
> to report it.  Sending both error codes isn't really a good idea because it
> probably violates other MUST statements, so you have to pick one.

I'm not suggesting to send both codes, rather to indicate to implementations
how to proceed in general when facing a state issue. I *think* that it should
be reasonable to suggest that stream errors should be replaced with connection
errors for HEADERS/CONTINUATION/PUSH_PROMISE because the compression context
cannot recover from these ones when placed out of stream (which is a protocol
error, thus an implementation error).

> It looks like we did a reasonable job of avoiding this sort of problem in h2,
> probably as a result of not using many different error codes (Mike can
> consider h3 to be on notice here :), but this one is probably worth some text
> on.  I'm inclined to address the problem generically rather than
> specifically.  We might not have many other cases of this in RFC 7540, but I
> can't guarantee that some h2 extension won't create a similar sort of
> conflict when two error cases collide.

I've found a few other ones in the past that I had to address so that
h2spec doesn't complain, I was fine with doing so because either choice
was OK. I really think that the problematic ones are for these few frames
which affect the compression state.

> My view is that there is no correct answer.  There are two errors and we
> shouldn't ask implementations to conform here.  Implementations should be
> able to choose any structure they like that meets protocol requirements AND
> they should be allowed to report the first error they encounter.  To that
> end, if there is an erratum, I would prefer that it say something like this,
> added to Section 5.4 (Error Handling):
> 
>   This specification or its extensions could define error conditions that
>   result in conflicting requirements about which error code is sent when
>   multiple errors are present.  Implementations MAY respond to the first
>   error condition that they detect.  Consequently, if multiple errors are
>   present, different implementations could produce different error codes.
>   However, a connection error MUST NOT be suppressed as a result of reporting
>   a stream error.

Do you imply by the last sentence that both a stream error and a connection
error should be sent in this case ? If so, it sounds less natural to me
because it could significantly complicate error paths depending on the
implementations. What about something like this :

    When a violation could result either in a stream error or a connection
    error depending on where the condition is matched, the connection error
    has precedence and must be applied.

Thanks!
Willy

---

From: Martin Thomson <[email-masked]>
Date: Thu, 24 Jan 2019 18:56:55 +1100

> which affect the compression state.

Ahh, I don't think that we should be subject to the tyranny of the interop suite in this case.  If you were responding to a genuine error condition with the prescribed error code, you should be conformant.

> error should be sent in this case ? 

Maybe.  The idea is not to constrain the error reporting so that both have to be juggled and balanced.  You should be able to report the stream error then the connection error, or if you encounter the connection error first, just report that.  (Or report the stream error as a connection error, which is also permitted).

> If so, it sounds less natural to me

Yeah, what you have is probably clearer, but I would simply say "the connection error MUST be reported".  The notion of precedence doesn't really matter here - that might imply a requirement to suppress the stream error, which I don't think is a good idea.  Error reporting needs to be uncomplicated and direct, and any implication of having to bundle them up so you can sort them out encourages sloppy code on critical paths that don't often get enough exercise in the real world.

---

From: Willy Tarreau <[email-masked]>
Date: Thu, 24 Jan 2019 09:12:37 +0100

On Thu, Jan 24, 2019 at 06:56:55PM +1100, Martin Thomson wrote:
> Ahh, I don't think that we should be subject to the tyranny of the interop
> suite in this case.  If you were responding to a genuine error condition with
> the prescribed error code, you should be conformant.

Oh I'm not giving way to this at all, however every error has the merit
to make me read the spec and wonder about the impacts of my choice. This
has allowed me to fix a high number of problems so for now I'd say it's
highly beneficial.

> Maybe.  The idea is not to constrain the error reporting so that both have to
> be juggled and balanced.  You should be able to report the stream error then
> the connection error, or if you encounter the connection error first, just
> report that.  (Or report the stream error as a connection error, which is
> also permitted).

Then we should probably suggest that stream errors should be upgraded to
connection errors when the same condition exists as a connection error in
another context.

> Yeah, what you have is probably clearer, but I would simply say "the
> connection error MUST be reported".

I got it now.

> The notion of precedence doesn't really
> matter here - that might imply a requirement to suppress the stream error,
> which I don't think is a good idea.

No, we don't care about a stream error being emitted as long as the connection
error is emitted. It's indeed not a good idea to dictate to implementations
how they should write their code. And the stream error might already have been
queued on the wire.

> Error reporting needs to be
> uncomplicated and direct, and any implication of having to bundle them up so
> you can sort them out encourages sloppy code on critical paths that don't
> often get enough exercise in the real world.

I wholeheartly agree!

OK thus I'm going to modify my code where I check for the frame types
to report a connection error when this is required in at least one
paragraph in the spec.

Thanks!
Willy