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

fix(s2n-quic-core): enforce max data limit size for stream and connection #1693

Merged
merged 3 commits into from
Apr 5, 2023

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Apr 1, 2023

Resolved issues:

#1675

Description of changes:

Limits::with_data_window accepts a u64 and is a limit specified by our users. In the debug build, we currently enforce that max_data limit is <= u32::MAX with a debug_assert. In the release build, the debug_assert is compiled out and applications can erroneously provide a value larger than u32::MAX.

This PR adds validation to the Limits builder which checks the value is less than u32 and provides a better user error. I also added comments on why its reasonable to restrict the value to u32 in this case. Finally, I convert the debug_asset into an assert to ensure we do no accidentally revert this change and re-introduce the bug only for the release build.

Call-outs:

Note that is issue actually applies to 4 data limits:

Note that the limits are backed by a IncrementalValueSync which will periodically update a bigger value to the remote endpoint so restricting the initial value to u32 is not a loss of functionality.

Testing:

  • Repro the bug: I ran tests against the original code and confirmed that we were getting overflow and UB based on the overflow value.
  • I have set the perf runner to use the max value (u32::MAX) as its limit.
  • Validated the fix locally
    • by providing values >u32::MAX and getting a ValidationError
    • running perf with a large transfer and confirming a successful run 482s s2n_quic:server:conn: packet_header=OneRtt { number: 28672209 } frame=Stream { id: 0, offset: 40294967189 } ...

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed? -->

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@toidiu toidiu changed the title fix: enforce max data limit size for stream and connection fix(s2n-quic-core): enforce max data limit size for stream and connection Apr 4, 2023
@toidiu toidiu marked this pull request as ready for review April 5, 2023 00:08
@toidiu toidiu merged commit ea33111 into main Apr 5, 2023
@toidiu toidiu deleted the ak-validation branch April 5, 2023 21:57
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