resource manager audit #3951
resource manager audit #3951
Conversation
- doing a ratio<> multiply now checks for overflow conditions. This is safe because:
- ratio is used for elastic limits, the largest current use being max-block-cpu * max-virtual-cpu-mult * maximum-ratio aka 200,000 * 1000 * 1000 which is well under the 64bit limit
- ratio is used to maintain an exponential moving average of usage, the largest current use being maximum-single-user-value * precision * window-size aka 200,000 * 1,000,000 * 172,800 which needs 55bits and therfore would not hit this assert
- doing cast from uint128_t for math to uint64_t for results now asserts that the values are representable, for the same reasons as ratio<> this is safe with the defaults but added to protect against future deployments with significantly higher values
…ignedness matches. this prevents a potentially overflowing cast
| @@ -16,13 +16,43 @@ namespace eosio { namespace chain { namespace resource_limits { | |||
|
|
|||
| template<typename T> | |||
| T operator* (T value, const ratio<T>& r) { | |||
| EOS_ASSERT(std::numeric_limits<T>::max() / r.numerator >= value, rate_limiting_state_inconsistent, "Usage exceeds maximum value representable after extending for precision"); | |||
There was a problem hiding this comment.
Can't r.numerator potentially be 0? The EOS_ASSERT should assert on the condition r.numerator == T() || std::numeric_limits<T>::max() / r.numerator >= value instead, assuming we can rely on T() representing the arithmetic zero value of the type T.
There was a problem hiding this comment.
in current practice I dont think its ever zero but this is a fine additional check.
| template<typename LesserIntType, typename GreaterIntType> | ||
| constexpr auto downgrade_cast(GreaterIntType val) -> | ||
| std::enable_if_t<std::is_signed<LesserIntType>::value == std::is_signed<GreaterIntType>::value, LesserIntType> | ||
| { |
There was a problem hiding this comment.
It would be even nicer if all of these templated functions had static asserts to check that the template type is an integer type.
libraries/chain/resource_limits.cpp
Outdated
| @@ -344,59 +349,6 @@ int64_t resource_limits_manager::get_account_cpu_limit( const account_name& name | |||
| int64_t cpu_weight; | |||
| get_account_limits( name, unused, unused, cpu_weight ); | |||
There was a problem hiding this comment.
I would prefer if you used two different variables for the unused arguments (similar to the x and y used in get_account_net_limit_ex), so that we don't get confusing by spooky aliasing behavior while debugging get_account_limits in the future.
arhag
added
the
CONSENSUS
|
We have determined that this change can actually prevent v1.0.2 from replaying a blockchain produced with v1.0.1. This was verified by replaying the Jungle testnet blockchain and observing a discrepancy at block 388461. |
Also removed repetition between get_account* and get_account*_ex