Skip to content

tests: Don't access out of bounds array index: array[sizeof(array)]#14398

Merged
maflcko merged 1 commit into
bitcoin:masterfrom
Empact:array-access
Oct 8, 2018
Merged

tests: Don't access out of bounds array index: array[sizeof(array)]#14398
maflcko merged 1 commit into
bitcoin:masterfrom
Empact:array-access

Conversation

@Empact

@Empact Empact commented Oct 5, 2018

Copy link
Copy Markdown
Contributor

Split from #13525

@maflcko maflcko added the Tests label Oct 5, 2018
@maflcko maflcko changed the title Don't access out of bounds array index: array[sizeof(array)] tests: Don't access out of bounds array index: array[sizeof(array)] Oct 5, 2018
@practicalswift

practicalswift commented Oct 5, 2018

Copy link
Copy Markdown
Contributor

Concept ACK

The fix seems to be exhaustive:

$ git grep -E '&[a-zA-Z0-9_:]*\[sizeof\([a-zA-Z0-9_:]*\)\]'
src/bench/checkblock.cpp:            (const char*)&block_bench::block413567[sizeof(block_bench::block413567)],
src/bench/checkblock.cpp:            (const char*)&block_bench::block413567[sizeof(block_bench::block413567)],
src/test/script_tests.cpp:    BOOST_CHECK(EvalScript(directStack, CScript(&direct[0], &direct[sizeof(direct)]), SCRIPT_VERIFY_P2SH, BaseSignatureChecker(), SigVersion::BASE, &err));
src/test/script_tests.cpp:    BOOST_CHECK(EvalScript(pushdata1Stack, CScript(&pushdata1[0], &pushdata1[sizeof(pushdata1)]), SCRIPT_VERIFY_P2SH, BaseSignatureChecker(), SigVersion::BASE, &err));
src/test/script_tests.cpp:    BOOST_CHECK(EvalScript(pushdata2Stack, CScript(&pushdata2[0], &pushdata2[sizeof(pushdata2)]), SCRIPT_VERIFY_P2SH, BaseSignatureChecker(), SigVersion::BASE, &err));
src/test/script_tests.cpp:    BOOST_CHECK(EvalScript(pushdata4Stack, CScript(&pushdata4[0], &pushdata4[sizeof(pushdata4)]), SCRIPT_VERIFY_P2SH, BaseSignatureChecker(), SigVersion::BASE, &err));

@maflcko

maflcko commented Oct 6, 2018

Copy link
Copy Markdown
Member

Can you explain how this is oob?

@Empact

Empact commented Oct 6, 2018

Copy link
Copy Markdown
Contributor Author

@MarcoFalke arrays of defined size N are bounded from 0 to N-1, while these index at N. Not directly harmful as only the addresses are taken, but invalid in the sense that they exceed the deined bounds. Suggested by @practicalswift here: #13525 (comment)

@maflcko maflcko merged commit b09c814 into bitcoin:master Oct 8, 2018
maflcko pushed a commit that referenced this pull request Oct 8, 2018
Merge #14398: tests: Don't access out of bounds array index: array[si…
2a74733 
…zeof(array)]

b09c814 Don't access out of bounds array entry array[sizeof(array)] (Ben Woosley)

Pull request description:

  Split from #13525

Tree-SHA512: bf44609fc5d497cd1894b85dce33a55977b3e0f3d03b986333a85967c1f3aa89089461f830939072bbb4d2477ccce26b9caeb627215bfb86a331f58d3466a4bd
@Bushstar Bushstar mentioned this pull request Oct 11, 2018
Merged
@Empact Empact deleted the array-access branch December 10, 2018 05:37
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 21, 2021
@Munkybooty
Merge bitcoin#14398 tests: Don't access out of bounds array index: ar…
bb24cd7 
…ray[sizeof(array)]
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 22, 2021
@Munkybooty
Merge bitcoin#14398 tests: Don't access out of bounds array index: ar…
8efb90f 
…ray[sizeof(array)]
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Empact @practicalswift @maflcko