Page MenuHomePhabricator

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

Authored by nakihito on Jul 5 2019, 20:25.

Details

Summary

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

Tree-SHA512: e4d39510d776c5ae8814cd5fb5c5d183cd8da937e339bff95caff68a84492fbec68bf513c5a6267446a564d39093e0c7fc703c645b511caab80f7baf7955b804

Backport of Core PR10530
https://github.com/bitcoin/bitcoin/pull/10530

Test Plan
make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR10530
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6695
Build 11437: Bitcoin ABC Teamcity Staging
Build 11436: arc lint + arc unit

Event Timeline

nakihito created this revision.Jul 5 2019, 20:25
Owners added a reviewer: Restricted Owners Package.Jul 5 2019, 20:25
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 5 2019, 20:25
Fabien added inline comments.Jul 5 2019, 21:35
src/arith_uint256.h
176 ↗(On Diff #10020)

I'd like to suggest std::numeric_limits<uint32_t>::max() as a replacement for (uint32_t)-1, but I wonder if this will be built as a constexpr by the compiler or generate a call at each loop increment ? Ping @deadalnix

markblundeberg added inline comments.Jul 5 2019, 23:40
src/arith_uint256.h
176 ↗(On Diff #10020)

I looked this up a bit the other day -- apparently Visual Studio in earlier versions didn't have constexpr for ...::max(), but C++11 make constexpr mandatory and I guess Visual Studio has fixed that since.

https://stackoverflow.com/questions/12240085/are-numeric-limits-min-max-constexpr
https://en.cppreference.com/w/cpp/types/numeric_limits/max
https://docs.microsoft.com/en-us/cpp/standard-library/numeric-limits-class?view=vs-2019
https://stackoverflow.com/questions/28138103/c11-numeric-limitsmax-at-compile-time

Fabien requested changes to this revision.Jul 6 2019, 06:39
Fabien added inline comments.
src/arith_uint256.h
176 ↗(On Diff #10020)

Ah good catch I missed the (until C++11) for the non constexpr version !

I don't think we support the VS compiler, or at least there is no instruction for it, only cross build with MinGw, so I guess it should be safe to use std::numeric_limits. @nakihito can you please update accordingly ?

This revision now requires changes to proceed.Jul 6 2019, 06:39
nakihito updated this revision to Diff 10028.Jul 6 2019, 06:44

Changed (uint32_t)-1 to std::numeric_limits<uint32_t>::max().

Fabien accepted this revision.Jul 6 2019, 06:46
This revision is now accepted and ready to land.Jul 6 2019, 06:46