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

Add length check for dirsDepth #1586

Closed
wants to merge 1 commit into from

Conversation

scovetta
Copy link

This is a simple addition of a few bounds checks to ensure that the dirsDepth array is not accessed at an index beyond its size.

Type of change

  • Improvement (improvement to code, performance, etc)

Description

This change adds index checks before dirDepths[collectionIndexDirDepth] and dirDepths[collectionNameDepth] to avoid indexing beyond the end of the array. We could just check for the collectionIndexDirDepth and avoid the other two, but that would be more brittle in case the constants defined a few lines earlier were reordered. We could also just calculate len(dirsDepth) once as a small perf improvement.

Additional details

I have never used Hyperledger, and did not test this change in any way. I also don't know how likely it would be for dirDepths to short enough to cause the panic.

Related issues

This is related to the suggestion from @manish-sethi in #1080.

This change adds index checks before `dirDepths[collectionIndexDirDepth]` and `dirDepths[collectionNameDepth]` to avoid indexing beyond the end of the array. We *could* just check for the `collectionIndexDirDepth` and avoid the other two, but that would be more brittle in case the constants defined a few lines earlier were reordered. We could also just calculate `len(dirsDepth)` once as a small perf improvement.

This is related to the [suggestion](hyperledger#1080 (review)) from @manish-sethi in hyperledger#1080.
@scovetta scovetta requested a review from a team as a code owner July 10, 2020 18:24
Copy link
Contributor

@cendhu cendhu left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. I have made one comment. Other than that, please address the following too

Comment on lines +429 to +431
len(dirsDepth) > collectionIndexDirDepth &&
dirsDepth[collectionIndexDirDepth] == "indexes" &&
len(dirsDepth) > collectionNameDepth:
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, can you update the first condition in the line 427? len(dirsDepth) > collectionDirDepth to len(dirsDepth) > collectionIndexDirDepth

@sykesm sykesm added the stale label Jul 30, 2020
@sykesm
Copy link
Contributor

sykesm commented Jul 30, 2020

@scovetta - It's been a bit since we've heard from you and I'm just wondering if you plan to sign-off on your commit and address Senthil's comments?

Thanks.

@sykesm
Copy link
Contributor

sykesm commented Aug 10, 2020

@scovetta We appreciate your time and the effort you've invested in opening this PR. Unfortunately it's been a month since we've heard from you, we're unable to merge commits without a DCO signoff, and there are pending comments so I'm going to close this out. If you find that you have some time to to revisit this in the future, please feel free to reopen.

Thanks.

@sykesm sykesm closed this Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants