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

Yield protocol violation for packets without frames #1693

Merged
merged 4 commits into from
Oct 23, 2023
Merged

Conversation

djc
Copy link
Member

@djc djc commented Oct 20, 2023

Received a private bug report from @QUICTester. Thanks!

This should probably have a test, which I don't have energy for right now. @Ralith any suggestion for where such a test should live?

I don't love that this equates "no frames" to "empty payload" which feels like a potential conceptual jump, but the for-loop formulation makes the alternative a decent bit more verbose...

@djc djc requested a review from Ralith October 20, 2023 20:16
@djc djc changed the title Error empty packets Yield protocol violation for packets without frames Oct 20, 2023
@Ralith
Copy link
Collaborator

Ralith commented Oct 20, 2023

any suggestion for where such a test should live?

If we move this check into frame::Iter::new it becomes easy to test in isolation, but also so trivial that we might not bother.

I don't love that this equates "no frames" to "empty payload" which feels like a potential conceptual jump

Aren't these equivalent? If payload data isn't a well-formed frame, that's an error too.

@djc djc force-pushed the error-empty-packets branch from 0c2526f to 55330fa Compare October 21, 2023 13:02
@djc
Copy link
Member Author

djc commented Oct 21, 2023

If we move this check into frame::Iter::new it becomes easy to test in isolation, but also so trivial that we might not bother.

Yup, that's better -- done.

Aren't these equivalent? If payload data isn't a well-formed frame, that's an error too.

Yup, I do think they're equivalent.

quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
@djc djc force-pushed the error-empty-packets branch from 55330fa to e716e2f Compare October 23, 2023 09:20
@djc djc merged commit 7bd92f6 into main Oct 23, 2023
8 checks passed
@djc djc deleted the error-empty-packets branch October 23, 2023 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants