Skip to content

Fix invalid instantiation and possibly unsafe accesses of array in class base_uint<BITS>#10530

Merged
laanwj merged 1 commit intobitcoin:masterfrom
pavlosantoniou:master
Jun 22, 2017
Merged

Fix invalid instantiation and possibly unsafe accesses of array in class base_uint<BITS>#10530
laanwj merged 1 commit intobitcoin:masterfrom
pavlosantoniou:master

Conversation

@pavlosantoniou
Copy link
Copy Markdown
Contributor

The implementation of base_uint::operator++(int) and base_uint::operator--(int) is now safer.
Array pn is accessed via index i after bounds checking has been performed on the index, rather than before.

The logic of the while loops has also been made more clear.

@pavlosantoniou pavlosantoniou changed the title Fix possibly unsafe accesses of array in class base_uint<BITS>. Fix possibly unsafe accesses of array in class base_uint<BITS> Jun 5, 2017
Comment thread src/arith_uint256.h Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As I see it, this code can cause an out-of-bounds access only in the case of WIDTH=0, which would be pointless.
Or am I missing something?
I do agree that this change makes the code somewhat more readable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, the out-of-bounds access occurs in the edge case of WIDTH == 0.
This template class can, although it probably won't, be instantiated with BITS < 32, something that would lead to WIDTH == 0.
I was mainly interested in ensuring the out-of-bounds-check before array access regardless of such assumptions.
Making it easier to follow the code logic is an extra gain.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This template class can, although it probably won't, be instantiated with BITS < 32

Indeed - as the code throughout the entire class is implemented, it can only be instantiated for a positive multiple of 32. Might make sense to add a compile-time assertion for that.

Copy link
Copy Markdown
Contributor Author

@pavlosantoniou pavlosantoniou Jun 7, 2017

Choose a reason for hiding this comment

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

Do you think adding:
static_assert(BITS/32 > 0 && BITS%32 == 0, "Template parameter BITS must be a positive multiple of 32.");
in the class constructor would be fine?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, that'd be nice!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have added the assertions, is there something more I can do for this PR?

The implementation of base_uint::operator++(int) and base_uint::operator--(int) is now safer.
Array pn is accessed via index i after bounds checking has been performed on the index, rather than before.
The logic of the while loops has also been made more clear.

A compile time assertion has been added in the class constructors to ensure that BITS is a positive multiple of 32.
@pavlosantoniou pavlosantoniou reopened this Jun 7, 2017
@pavlosantoniou
Copy link
Copy Markdown
Contributor Author

Reopened with addition of static_assert for BITS value.

@pavlosantoniou pavlosantoniou changed the title Fix possibly unsafe accesses of array in class base_uint<BITS> Fix invalid instantiation and possibly unsafe accesses of array in class base_uint<BITS> Jun 10, 2017
@sipa
Copy link
Copy Markdown
Member

sipa commented Jun 11, 2017

utACK

@laanwj laanwj merged commit e5c6168 into bitcoin:master Jun 22, 2017
laanwj added a commit that referenced this pull request Jun 22, 2017
…of array in class base_uint<BITS>

e5c6168 Fix instantiation and array accesses in class base_uint<BITS> (Pavlos Antoniou)

Tree-SHA512: e4d39510d776c5ae8814cd5fb5c5d183cd8da937e339bff95caff68a84492fbec68bf513c5a6267446a564d39093e0c7fc703c645b511caab80f7baf7955b804
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 11, 2019
…cesses of array in class base_uint<BITS>

e5c6168 Fix instantiation and array accesses in class base_uint<BITS> (Pavlos Antoniou)

Tree-SHA512: e4d39510d776c5ae8814cd5fb5c5d183cd8da937e339bff95caff68a84492fbec68bf513c5a6267446a564d39093e0c7fc703c645b511caab80f7baf7955b804
schancel pushed a commit to givelotus/lotusd that referenced this pull request Jul 18, 2019
…of array in class base_uint<BITS>

Summary:
e5c6168 Fix instantiation and array accesses in class base_uint<BITS> (Pavlos Antoniou)

Tree-SHA512: e4d39510d776c5ae8814cd5fb5c5d183cd8da937e339bff95caff68a84492fbec68bf513c5a6267446a564d39093e0c7fc703c645b511caab80f7baf7955b804

Backport of Core PR10530
bitcoin/bitcoin#10530

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3569
jonspock pushed a commit to jonspock/devault that referenced this pull request Aug 28, 2019
…of array in class base_uint<BITS>

Summary:
e5c6168 Fix instantiation and array accesses in class base_uint<BITS> (Pavlos Antoniou)

Tree-SHA512: e4d39510d776c5ae8814cd5fb5c5d183cd8da937e339bff95caff68a84492fbec68bf513c5a6267446a564d39093e0c7fc703c645b511caab80f7baf7955b804

Backport of Core PR10530
bitcoin/bitcoin#10530

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3569
jonspock pushed a commit to jonspock/devault that referenced this pull request Aug 28, 2019
…of array in class base_uint<BITS>

Summary:
e5c6168 Fix instantiation and array accesses in class base_uint<BITS> (Pavlos Antoniou)

Tree-SHA512: e4d39510d776c5ae8814cd5fb5c5d183cd8da937e339bff95caff68a84492fbec68bf513c5a6267446a564d39093e0c7fc703c645b511caab80f7baf7955b804

Backport of Core PR10530
bitcoin/bitcoin#10530

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3569
proteanx pushed a commit to devaultcrypto/devault that referenced this pull request Aug 28, 2019
…of array in class base_uint<BITS>

Summary:
e5c6168 Fix instantiation and array accesses in class base_uint<BITS> (Pavlos Antoniou)

Tree-SHA512: e4d39510d776c5ae8814cd5fb5c5d183cd8da937e339bff95caff68a84492fbec68bf513c5a6267446a564d39093e0c7fc703c645b511caab80f7baf7955b804

Backport of Core PR10530
bitcoin/bitcoin#10530

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3569
@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants