Skip to content

fix: Add range check to signature index#5157

Merged
mversic merged 1 commit intomainfrom
range_check
Oct 22, 2024
Merged

fix: Add range check to signature index#5157
mversic merged 1 commit intomainfrom
range_check

Conversation

@SamHSmith
Copy link
Copy Markdown
Contributor

@SamHSmith SamHSmith commented Oct 17, 2024 •
edited by mversic
Loading

Context

The bug is triggered if a a peer is unregistered from the topology while Sumeragi Messages are still in flight. This
is the only place where we use and array index operation that can panic. It is very hard to write a test for this case
that is guaranteed to show the issue each time.

Closes #5104

Solution

  • Describe the approach taken to achieve the objective / resolve the issue.

Migration Guide (optional)

  • If this PR contains a breaking change relative to the main branch, provide an instruction on how affected parties might need to adapt to the change.

Review notes (optional)

  • For complex PRs, try to provide some information on how to approach the review more effectively.
  • For example, is there a natural order in which the affected files should be reviewed?

Checklist

  • I've read CONTRIBUTING.md.
  • (optional) I've written unit tests for the code changes.
  • All review comments have been resolved.
  • All CI checks pass.

@SamHSmith SamHSmith force-pushed the range_check branch 2 times, most recently from 2011f88 to 97aa867 Compare October 17, 2024 22:10
@mversic
Copy link
Copy Markdown
Contributor

mversic commented Oct 18, 2024

what's the corresponding issue for this?

@0x009922 0x009922 linked an issue Oct 18, 2024 that may be closed by this pull request
[BUG] Sumeragi panics with "index out of bounds" message after unregistering a peer #5104
Closed
@0x009922
Copy link
Copy Markdown
Contributor

0x009922 commented Oct 18, 2024

Linked it: #5104

0x009922
0x009922 reviewed Oct 18, 2024
View reviewed changes
Comment thread crates/iroha_core/src/sumeragi/main_loop.rs Outdated
mversic
mversic reviewed Oct 18, 2024
View reviewed changes
Comment thread crates/iroha_core/src/sumeragi/main_loop.rs Outdated
@mversic mversic requested a review from 0x009922 October 21, 2024 09:22
@SamHSmith @mversic
fix: Add range check to signature index
ffcdcd5 
Signed-off-by: Sam H. Smith <sam.henning.smith@protonmail.com>
@mversic mversic merged commit ae5eb8a into main Oct 22, 2024
@mversic mversic deleted the range_check branch October 22, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0x009922 0x009922 0x009922 approved these changes

@mversic mversic mversic approved these changes

@s8sato s8sato Awaiting requested review from s8sato s8sato is a code owner

@outoftardis outoftardis Awaiting requested review from outoftardis outoftardis is a code owner

@Erigara Erigara Awaiting requested review from Erigara

@DCNick3 DCNick3 Awaiting requested review from DCNick3 DCNick3 is a code owner

@dima74 dima74 Awaiting requested review from dima74 dima74 is a code owner

Assignees

@SamHSmith SamHSmith

@mversic mversic

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Sumeragi panics with "index out of bounds" message after unregistering a peer

3 participants

@SamHSmith @mversic @0x009922