-
Notifications
You must be signed in to change notification settings - Fork 204
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
allow CRYPTO_BUFFER_EXCEEDED when overflowing a CRYPTO frame #3186
allow CRYPTO_BUFFER_EXCEEDED when overflowing a CRYPTO frame #3186
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! LGTM.
I can see how this can simplify some decoding logic, but I am not sure we should consider such a value valid in the first place. If the value is invalid it is meaningless to discuss length and then there is only a frame error left. Otherwise we could probably revisit a gazillion other places where this principle could also apply. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I thought the principle we have is that if you could identify a frame as being invalid with no other information, it should be a FRAME_ENCODING_ERROR, so the existing text is correct?
@ianswett yes this is largely what I meant. The PR introduces a new principle. |
@marten-seemann : I'm confused too. For consistency, I would keep it at FRAME_ENCODING_ERROR, since that's what we do for the flow control error too. |
@ianswett @janaiyengar I am with @marten-seemann. Let me explain why. There are two reasons we decided to allow use of FLOW_CONTROL_ERROR for STREAM frames:
These reasons equally hold for CRYPTO streams sending CRYPTO_BUFFER_EXCEEDED, as CRYPTO_BUFFER_EXCEEDED is the error code used to indicate flow control error with CRYPTO streams. To paraphrase this PR applies the principles that we agreed for STREAM frame to CRYPTO frames. |
Thanks for clarifying, @kazuho. This LGTM. |
From the original issue, the proposed principle was: "If you can detect an error while parsing the frame, and without consulting any connection state, it should be a FRAME_ENCODING_ERROR." It seems clear you can do that in this case, but maybe we changed the principle? |
@ianswett That would also apply to the STREAM frame then. I'd be fine just using the FRAME_ENCODING_ERROR in all these cases, but people pushed back against that. |
@ianswett Yeah I think I should have said that the exceptional rule within the principles. Specifically, I mean the discussion starting from #3042 (review), that talks if we should allow FLOW_CONTROL_ERROR and why. |
@kazuho I thought that might have been the relevant discussion. In general, I'd prefer to have principles decided on issues, but I understand these things happen. I don't think this is the end of the world, but I prefer the original principle because I think it's easy to reason about. If we've decided against that principle, I'd suggest we state a new principle. |
@ianswett I don't think we ever actually discussed this in the working group. If I remember correctly, using two error codes was a suggestion that came up on my original PR, and then the editors decided in Cupertino that I should update the PR accordingly. Actually, I'd prefer to take the principled approach, and just use FRAME_ENCODING_ERROR wherever possible. |
@marten-seemann I think we did discuss at the interim. https://github.com/quicwg/wg-materials/blob/master/interim-19-10/minutes.md#issue-3027-codes-for-frame-encoding-errors |
To be precise, we discussed that two error codes were OK at the interim, then declared that as a consensus in the call started on Oct 22 https://mailarchive.ietf.org/arch/msg/quic/LrM0w3L3mHSLMn1mH98Xq1E-DQs. I do not think we should be revisiting the discussion. |
I think we should either:
If we don't have a principle that matches the text, we're prone to having this discussion a few more times. |
@martinthomson kindly shared our principles, and this PR aligns with that. |
Chairs and editors, I think we agree that this is an amendment to #3042 that has been shipped as part of -24. Does that mean that this would be merged as an editorial correction? Or if not, do we need an issue? |
I think we can merge it because it agrees with the stated principles. But its @martinthomson call |
Let's open an issue. At this stage, I want to be very particular about this sort of change, even if it is quite clearly within the bounds of established principles. |
@martinthomson did as #3258. |
As @kazuho pointed out in #3042 (comment), I missed that there's a CRYPTO_BUFFER_EXCEEDED that would equally valid to send when a CRYPTO frame exceeds the 2^62-1 limit.