Skip to content

fix(s2n-quic-core): enforce checked arithmetics to prevent overflows - #3081

Open
boquan-fang wants to merge 1 commit into
aws:mainfrom
boquan-fang:boquan-fang/varint-overflow
Open

fix(s2n-quic-core): enforce checked arithmetics to prevent overflows#3081
boquan-fang wants to merge 1 commit into
aws:mainfrom
boquan-fang:boquan-fang/varint-overflow

Conversation

@boquan-fang

Copy link
Copy Markdown
Contributor

Release Summary:

Resolved issues:

Description of changes:

VarInt add, mul, and other arithmetics are currently doing checked arithmetics operations during tests, but use non checked operations in production. This can cause integer overflows in production.

This problem should be fixed with checked operation without the separation for production and tests. The original decision was made based on performance concerns: we believe unchecked arithmetics is faster than checked arithmetics. However, we determine that the performance regression is negligible, but the potential exposure for overflow is concerning.

Call-outs:

Testing:

Current CI should pass.

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

@boquan-fang
boquan-fang force-pushed the boquan-fang/varint-overflow branch from c63f398 to d2e623e Compare May 13, 2026 00:08
@boquan-fang
boquan-fang marked this pull request as ready for review May 13, 2026 00:09
@boquan-fang
boquan-fang requested a review from a team as a code owner May 13, 2026 00:09
@camshaft

camshaft commented May 13, 2026

Copy link
Copy Markdown
Contributor

the intent was to allow the application to control if it got overflow checks or not, but unfortunately, that cfg isn't stable currently: rust-lang/rust#111466, so debug_assertions was the next-best thing.

@boquan-fang

Copy link
Copy Markdown
Contributor Author

the intent was to allow the application to control if it got overflow checks or not, but unfortunately, that cfg isn't stable currently: rust-lang/rust#111466, so debug_assertions was the next-best thing.

ACK. I think the decision right now is to always have a check for VarInt arithmetics to prevent overflow altogether.

@boquan-fang
boquan-fang requested a review from maddeleine May 13, 2026 16:49
@maddeleine

Copy link
Copy Markdown
Contributor

Have we done any type of performance analysis to prove that these checks don't add overhead? I assume we didn't want them in production for a reason.

@boquan-fang

Copy link
Copy Markdown
Contributor Author

Document this behavior on the top of the file.

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.

3 participants