Re: Conflicting MUST in RFC7540

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


On Thu, Jan 24, 2019, at 16:44, Willy Tarreau wrote:
> 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
>

Received on Thursday, 24 January 2019 07:30:17 UTC