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 potential overflow in ledger metrics LatestTrieRegCountDiff() and LatestTrieMaxDepthDiff() #2096

Merged
merged 3 commits into from Mar 14, 2022

Conversation

fxamacker
Copy link
Member

@fxamacker fxamacker commented Mar 2, 2022

Changes

Prevent overflow by replacing uint64 with int64 in two functions APIs that can receive negative deltas:

  • LatestTrieRegCountDiff(number int64)
  • LatestTrieMaxDepthDiff(number int64)

Also cast potentially overflowing expressions calling these functions.

Closes #2092

Caveat

This is a minimal fix that doesn't prevent all overflow scenarios. For example, if the delta doesn't fit into int64 then we still have an overflow but that scenario is unlikely given the size of int64.

@fxamacker fxamacker added the Bug Something isn't working label Mar 2, 2022
@fxamacker fxamacker self-assigned this Mar 2, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #2096 (658545f) into master (adade03) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2096      +/-   ##
==========================================
+ Coverage   57.28%   57.29%   +0.01%     
==========================================
  Files         634      634              
  Lines       36555    36555              
==========================================
+ Hits        20939    20946       +7     
+ Misses      12985    12978       -7     
  Partials     2631     2631              
Flag Coverage Δ
unittests 57.29% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
ledger/complete/mtrie/forest.go 61.14% <100.00%> (ø)
consensus/hotstuff/eventloop/event_loop.go 68.29% <0.00%> (ø)
...sus/approvals/assignment_collector_statemachine.go 47.11% <0.00%> (+6.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adade03...658545f. Read the comment docs.

@fxamacker fxamacker merged commit 36287d2 into master Mar 14, 2022
@fxamacker fxamacker deleted the fxamacker/fix-ledger-metrics-delta-overflow branch March 14, 2022 23:05
@fxamacker fxamacker added the Execution Cadence Execution Team label Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Execution Cadence Execution Team
Projects
None yet
4 participants