Sentinel 2pc controller bug fixes #151
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HalosGhost
reviewed
Aug 16, 2022
mszulcz-mitre
force-pushed
the
sentinel_2pc-controller-bug-fix
branch
from
a0a5cd1
to
69eb59d
Compare
This was
linked to
issues
Aug 17, 2022
|
@mszulcz-mitre this has a very minimal merge conflict with |
mszulcz-mitre
force-pushed
the
sentinel_2pc-controller-bug-fix
branch
from
69eb59d
to
53efae3
Compare
|
@metalicjames when you have a chance to review this, I'd love a second set of eyes. |
metalicjames
reviewed
Sep 9, 2022
metalicjames
reviewed
Sep 9, 2022
Previously, sentinel_2pc::controller would incorrectly calculate the upper bound of the sentinel ID distribution if the number of sentinel clients was 0. The incorrect calculation was due to casting a negative number to an unsigned int. As a result of this error, the sentinel IDs taken from the distribution could exceed the range of the possible sentinel IDs, which could lead to a non-existent element access when a bad ID was used as an index to a vector. Signed-off-by: Michael L. Szulczewski <mszulczewski@mitre.org>
sentinel_2pc::controller allows the sentinel ID to exceed the number of sentinels, which causes a non-existent element access (see GitHub Issue mit-dci#140). This commit checks for that condition and throws an error if it's true. It also adds a unit test to confirm that the code behaves correctly after the fix. Signed-off-by: Michael L. Szulczewski <mszulczewski@mitre.org>
Check that the number of required attestations doesn't exceed the number of sentinels that can provide them. Also add a unit test to confirm that the check works. Signed-off-by: Michael L. Szulczewski <mszulczewski@mitre.org>
Previously, the init() method of the 2PC sentinel controller would exhibit a non-existent element access if no sentinel endpoints were defined. With this commit, the code now checks that at least one endpoint is defined and logs an error if it's not. Signed-off-by: Michael L. Szulczewski <mszulczewski@mitre.org>
mszulcz-mitre
force-pushed
the
sentinel_2pc-controller-bug-fix
branch
from
53efae3
to
99116fc
Compare
|
I modified the code to avoid the possibility of underflow. For clarity, I added another check to confirm that at least one sentinel endpoint is defined and I added a related unit test too. |
metalicjames
approved these changes
Sep 15, 2022
|
@HalosGhost It looks like James approved the changes I made in response to his review. Would you like me to do anything more for this pull request? |
|
Sorry for the delay; I've finally had a chance to walk through the code locally and test it. Tested ACK. All these changes eliminate several failure points and do so minimally. Looks good to me. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the bugs identified in Issue #140 and Issue #141. The bug fixes and associated unit tests are the same as those described in the issues.