Skip to content
This repository has been archived by the owner on Oct 4, 2019. It is now read-only.

fix atxi pagination OoB possibility #627

Merged
merged 3 commits into from
Jun 15, 2018
Merged

Conversation

whilei
Copy link
Contributor

@whilei whilei commented Jun 8, 2018

problem: atxi pagination param can cause slice OoB
solution: ensure param within len lim

Fixes #626

solution: ensure param within len lim

Fixes #626
@whilei whilei requested review from tzdybal and r8d8 June 8, 2018 12:03
core/atxi.go Outdated
@@ -367,9 +367,13 @@ func GetAddrTxs(db ethdb.Database, address common.Address, blockStartN uint64, b
}
if paginationStart < 0 {
paginationStart = 0
} else if paginationStart > len(s) {
paginationStart = len(s)
}
if paginationEnd < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified to:

if paginationEnd < 0 || paginationEnd > len(s) {
    paginationEnd = len(s)
}

solution: refactor core/ getter method to return an error, if any
api can then provide user with more helpful debugging
@whilei
Copy link
Contributor Author

whilei commented Jun 11, 2018

@tzdybal Fixed, and refactored core/GetAtxi method to return an error, if any. Also added an additional validation that ! paginationStart > paginationEnd.

solution: fix signatures, add a small test, and fix param condition
@whilei
Copy link
Contributor Author

whilei commented Jun 13, 2018

Maybe actually fixed.

@whilei
Copy link
Contributor Author

whilei commented Jun 14, 2018

Just a heads up that this failing test is #630, fixed with #632, and is unrelated to these changes.

@tzdybal
Copy link
Contributor

tzdybal commented Jun 15, 2018

:lgtm:


Reviewed 2 of 3 files at r2, 3 of 3 files at r3.
Review status: all files reviewed, 1 unresolved discussion (waiting on @tzdybal and @r8d8)


Comments from Reviewable

@whilei whilei merged commit 5291a28 into master Jun 15, 2018
@soc1c soc1c deleted the fix/atxi-pagination-slice-oob branch June 19, 2019 12:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants