Connection-level rejection of oversized DATA frames?

Hi all,

At the moment, section 4.2 of draft-ietf-httpbis-http2-16 says:

   An endpoint MUST send a FRAME_SIZE_ERROR error if a frame exceeds the
   size defined in SETTINGS_MAX_FRAME_SIZE, any limit defined for the
   frame type, or it is too small to contain mandatory frame data.  A
   frame size error in a frame that could alter the state of the entire
   connection MUST be treated as a connection error (Section 5.4.1);
   this includes any frame carrying a header block (Section 4.3) (that
   is, HEADERS, PUSH_PROMISE, and CONTINUATION), SETTINGS, and any frame
   with a stream identifier of 0.

I read this to mean that if the server sets the maximum frame size it
will accept to 256KB, and a client sends a DATA frame with a length of
16MB (defined in the header) followed by 16MB of client data, the server
has no choice but to read the data, as the error is at the stream-level
(i.e. RST_STREAM), as the second sentence in that paragraph doesn’t apply.

The draft text doesn’t allow rejecting this frame early without reading
_ALL_ the data, which is a waste of bandwidth. I think the server should be
able to send a GOAWAY connection error of type FRAME_SIZE_ERROR, followed
by closing the TCP/TLS socket. With the current draft spec, the only way
to handle this now is to send a RST_STREAM of type FRAME_SIZE_ERROR and
then send a GOAWAY( NO_ERROR ) to avoid reading the large amount of client
data following, but this is more ambiguous.

Is there any reason why we couldn't add the following text to the bottom
of section 6.1 which says:    (could be either MAY or MUST)

   Receipt of a DATA frame with a length greater than the receiver's
   SETTINGS_MAX_FRAME_SIZE setting MAY be treated as a connection error
   (Section 5.4.1) of type FRAME_SIZE_ERROR.

Or alternatively, we could define all frame size errors to be connection
errors, which would simplify the spec and implementation logic. Given that
this is a new protocol, this would help avoid poor implementations. Frame
size errors should only happen due to poor programming or fuzzing/cracking
attempts.

We could then remove this entire paragraph from section 4.2:

   A frame size error in a frame that could alter the state of the entire
   connection MUST be treated as a connection error (Section 5.4.1);
   this includes any frame carrying a header block (Section 4.3) (that
   is, HEADERS, PUSH_PROMISE, and CONTINUATION), SETTINGS, and any frame
   with a stream identifier of 0.

and replace it with:

   A frame size error in a frame MUST be treated as a connection error
   (Section 5.4.1).

The only stream-level FRAME_SIZE_ERROR defined in the specification appears
to be:

   A PRIORITY frame with a length other than 5 octets MUST be treated as
   a stream error (Section 5.4.2) of type FRAME_SIZE_ERROR.

What do others think? I appreciate this is "last-minute", but I think we
should sort out this nit -- the smaller change of just adding the extra text
for the DATA frame would be sufficient for me, but I’d prefer it if we could
tidy up and make the text and specification clearer. Happy to prepare a PR
of revised text for this.

Thanks,
Jonathan.

Received on Monday, 26 January 2015 22:23:54 UTC