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 time arithmetic in Validations and Consensus #3656

Closed
carlhua opened this issue Nov 5, 2020 · 1 comment
Closed

Fix time arithmetic in Validations and Consensus #3656

carlhua opened this issue Nov 5, 2020 · 1 comment
Assignees
Labels
Reviewed RIPD Export Exported from legacy JIRA issue tracking Tech Debt Non-urgent improvements

Comments

@carlhua
Copy link
Contributor

carlhua commented Nov 5, 2020

Several spots in generic validation and consensus do arithmetic on times/durations to see if messages are stale. Since NetClock uses uint32_t for its representation, we should be careful to avoid the potential for overflow. In most cases, this means using addition rather than subtraction, e.g.

signed > now - early 

to

signed + early > now

Exported from RIPD-1489

@carlhua carlhua added Tech Debt Non-urgent improvements RIPD Export Exported from legacy JIRA issue tracking Reviewed Medium Priority labels Nov 5, 2020
HowardHinnant added a commit to HowardHinnant/rippled that referenced this issue Mar 9, 2021
* NetClock::rep is uint32_t and can be error-prone when
  used with subtraction.
* Fixes XRPLF#3656
@HowardHinnant
Copy link
Contributor

To address this issue a comprehensive survey of all uses of NetClock::time_point in rippled was made. Only two minor were found.

  1. src/ripple/consensus/Validations.h has this expression:
signTime > (now - p.validationCURRENT_EARLY)

This is the expression refered to as being vulnerable to unsigned overflow. However it turns out this expression is fine because p.validationCURRENT_EARLY is a 64 bit signed integral type. Thus the 32 but unsigned now is losslessly converted to signed 64 bit prior to the conversion. Additionally the 32 but unsigned signTime is converted to signed 64 bit prior to the comparison.

The expression is correct and bullet-proof as is. A comment to this effect has been added.

  1. src/ripple/app/tx/impl/CreateOffer.cpp has this expression:
NetClock::time_point const when{ctx_.view().parentCloseTime()};

While correct, using explicit conversion syntax for implicit construction is error-prone because one might accidentally choose an unwanted explicit constructor. This code is safer using implicit syntax to ensure that only an implicit constructor is chosen:

NetClock::time_point const when = ctx_.view().parentCloseTime();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed RIPD Export Exported from legacy JIRA issue tracking Tech Debt Non-urgent improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants