Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close in response to invalid CONNECTION_CLOSE #3230

Closed
wants to merge 1 commit into from

Conversation

martinthomson
Copy link
Member

The looping case was a little unclear. This should make it a tiny bit
clearer. As Roni observes, we can't really let invalid CONNECTION_CLOSE
through without some sort of special handling. This proposes that.

Unfortunately, this is a little more than an editorial fix. So I'm
going to ask that the issue be promoted.

Closes #2475.

The looping case was a little unclear.  This should make it a tiny bit
clearer.  As Roni observes, we can't really let invalid CONNECTION_CLOSE
through without some sort of special handling.  This proposes that.

Unfortunately, this is a little more than an editorial fix.  So I'm
going to ask that the issue be promoted.

Closes #2475.
@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Nov 12, 2019
@@ -2709,8 +2710,10 @@ frame risks a peer missing the first such packet. The only mechanism available
to an endpoint that continues to receive data for a terminated connection is to
use the stateless reset process ({{stateless-reset}}).

An endpoint that receives an invalid CONNECTION_CLOSE frame MUST NOT signal the
existence of the error to its peer.
An endpoint that receives an invalid CONNECTION_CLOSE frame MUST treat the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is an invalid CONNECTION_CLOSE frame?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One that has an reason phrase that overruns the packet would do it. I can't see any other reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need any extra rules for this special case. Just do what you'd normally do: close the connection with a FRAME_ENCODING_ERROR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, you think that it is enough to rely on rate limiting of the CONNECTION_CLOSE packet (c.f., #3095). That would also work.

send further packets, which could result in a constant exchange of
CONNECTION_CLOSE frames until the closing period on either peer ended.
After receiving a CONNECTION_CLOSE frame, or an invalid packet that appears to
contain a CONNECTION_CLOSE frame, endpoints enter the draining state. An
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like Github swallowed my previous comment on this line:
How would you even parse an invalid packet? What does it mean to "appear to contain" a frame?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A packet might "appear to contain a frame" if it contains the truncated beginning of a frame.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"or an invalid packet that appears to contain a CONNECTION_CLOSE_ frame," is a pretty big category of potential issues. Does this need to be called out as separate from the general invalid packet case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we'll just go with #3231 and avoid having to worry about this definition.

martinthomson added a commit that referenced this pull request Nov 12, 2019
If we get one, rely on the back-off that we require endpoints to
implement when they enter the closing state.  No need for any additional
special processing rules.

Closes #2475.
Closes #3230.
An endpoint that receives an invalid CONNECTION_CLOSE frame MUST treat the
message as being equivalent to a CONNECTION_CLOSE with the INTERNAL_ERROR error
code. The endpoint MAY send a single CONNECTION_CLOSE in response, but it MUST
then enter the draining period; see {{draining}}.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would INTERNAL_ERROR not suggest a locally induced inconsistent state as opposed to an observed remote inconsistent state? If so, I think it is important to make a distinction since it affects who is to blame and consequently where to debug.

@martinthomson martinthomson deleted the invalid-close branch December 3, 2019 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid CONNECTION_CLOSE frames
5 participants