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 integer overflow and underflow in DeferralKey methods and add corresponding tests #20387

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

xiaodi007
Copy link
Contributor

Description

This PR addresses two critical issues in the DeferralKey implementation that could lead to integer overflow and underflow, potentially causing panics or incorrect behavior. It also includes new test cases to verify the correctness of the fixes.

Issues Fixed:

  1. Prevent Integer Overflow in range_for_up_to_consensus_round

    • Issue: When consensus_round equals u64::MAX, using checked_add(1).unwrap() in DeferralKey::range_for_up_to_consensus_round causes a panic due to overflow.
    • Solution: Replace checked_add(1).unwrap() with saturating_add(1) to safely handle the maximum value without causing a panic.
  2. Prevent Integer Underflow in transaction_deferral_within_limit

    • Issue: In transaction_deferral_within_limit, when future_round is less than deferred_from_round, subtracting them causes an underflow, resulting in incorrect behavior.
    • Solution: Use checked_sub to safely perform the subtraction, and handle the None case by returning false.

Test plan

  1. Test Name: test_range_for_up_to_consensus_round_no_overflow

    Description: Verifies that DeferralKey::range_for_up_to_consensus_round does not overflow when consensus_round is u64::MAX, and that it correctly returns future_round equal to u64::MAX.

  2. Test Name: test_transaction_deferral_within_limit_underflow

    Description: Ensures that transaction_deferral_within_limit correctly handles cases where future_round is less than deferred_from_round, preventing integer underflow and returning false.


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • Indexer:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:
  • REST API:

Fix integer overflow in range_for_up_to_consensus_round
Prevent integer underflow in transaction_deferral_within_limit
Copy link

vercel bot commented Nov 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 23, 2024 3:56am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Nov 23, 2024 3:56am
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Nov 23, 2024 3:56am
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Nov 23, 2024 3:56am

@bmwill
Copy link
Contributor

bmwill commented Nov 22, 2024

@mystenmark and/or @aschran would be a better reviewer for this change

@bmwill bmwill requested review from mystenmark and aschran and removed request for bmwill and halfprice November 22, 2024 17:52
@@ -54,7 +54,7 @@ impl DeferralKey {
deferred_from_round: 0,
},
Self::ConsensusRound {
future_round: consensus_round.checked_add(1).unwrap(),
future_round: consensus_round.saturating_add(1),
Copy link
Contributor

Choose a reason for hiding this comment

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

If an epoch runs for long enough that consensus_round reaches u64::MAX (or gets this close to it), then I think we want to panic, because there is no safe way for the system to handle that

Keep in mind that at our current rate of 14 rounds/s it would take an epoch of over 4e10 years for us to reach this limit, even if we somehow 1000x this rate (impossible I think) then it would take 10s of millions of years :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, if we did reach this point, doing a saturating add would cause incorrect behavior.

crates/sui-core/src/authority/transaction_deferral.rs Outdated Show resolved Hide resolved
@mystenmark
Copy link
Contributor

@xiaodi007 Thanks for the contribution - see the comments above. I believe it is impossible for either issue to occur, but in the second case we could be more defensive.

Add a debug_assert! to enforce the invariant during object creation; and adjust future_round to be at least deferred_from_round in release builds to maintain the invariant without panicking in new_for_consensus_round
Add debug_assert!(future_round >= deferred_from_round); and use saturating_sub in transaction_deferral_within_limit.
Revert the change in range_for_up_to_consensus_round to keep checked_add(1).unwrap()
@xiaodi007
Copy link
Contributor Author

xiaodi007 commented Nov 23, 2024

@mystenmark @aschran I'm very grateful for your valuable feedback. I change my code to be more defensive.

Add a debug_fatal! to enforce the invariant during object creation; and adjust future_round to be at least deferred_from_round in release builds to maintain the invariant without panicking in new_for_consensus_round.

Add debug_assert!(future_round >= deferred_from_round); and use saturating_sub in transaction_deferral_within_limit.

Thanks again for your insightful feedback! I've made the updates to the PR as you suggested. I'll keep contributing actively. The updates are ready now—please take a look!

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.

4 participants