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 panic on closing nil db #3175

Merged

Conversation

algonautshant
Copy link
Contributor

@algonautshant algonautshant commented Nov 2, 2021

Summary

FillDBWithParticipationKeys will return empty PersistedParticipation when lastValid is less than firstValid.
When this happens, newPart will not have partdb in newPart.Store, instead, will have an empty object.
Calling Close on the empty object will call close on nil Accessor pointer and panic.

This change avoids using the returned object with non-nil error, and properly closes the db with the valid object.

Test Plan

Added test to verify the proper handling of the different errors returned from FillDBWithParticipationKeys and handled by GenParticipationKeysTo.

When account.FillDBWithParticipationKeys fails and returns an error, e.g. if the key already exists, newPart will be invalid and should not be closed.
When Close is called, it will panic.
FillDBWithParticipationKeys will return empty PersistedParticipation when lastValid is less than firstValid.
When this happens, newPart will not have partdb in newPart.Store, instead, will have an empty object.
Calling Close on the empty object will call close on nil Accessor pointer and panic.

This change avoids using the returned object with non-nil error, and properly closes the db with the valid object.

Added test to varify the proper handling of the different errors returned from FillDBWithParticipationKeys and handled by GenParticipationKeysTo.
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

looks great, but please fix the review dog comments..

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2021

Codecov Report

Merging #3175 (ed3a084) into master (168cfe5) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3175      +/-   ##
==========================================
- Coverage   43.81%   43.77%   -0.05%     
==========================================
  Files         392      392              
  Lines       86885    86885              
==========================================
- Hits        38073    38033      -40     
- Misses      42782    42812      +30     
- Partials     6030     6040      +10     
Impacted Files Coverage Δ
data/account/participation.go 32.95% <0.00%> (ø)
libgoal/participation.go 0.00% <0.00%> (ø)
util/metrics/reporter.go 56.31% <0.00%> (-4.86%) ⬇️
ledger/blockqueue.go 81.03% <0.00%> (-4.03%) ⬇️
util/metrics/gauge.go 68.00% <0.00%> (-2.67%) ⬇️
util/metrics/counter.go 78.57% <0.00%> (-2.39%) ⬇️
network/wsPeer.go 69.86% <0.00%> (-2.14%) ⬇️
catchup/service.go 69.07% <0.00%> (-1.50%) ⬇️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
crypto/merkletrie/node.go 92.55% <0.00%> (-0.94%) ⬇️
... and 3 more

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 168cfe5...ed3a084. Read the comment docs.

algorandskiy
algorandskiy previously approved these changes Nov 2, 2021
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

LGTM

@tsachiherman tsachiherman merged commit 009b798 into algorand:master Nov 3, 2021
@egieseke egieseke mentioned this pull request Nov 23, 2021
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