Page MenuHomePhabricator

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

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

Details

Reviewers
deadalnix
Fabien
jasonbcox
markblundeberg
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCb699b707e3e8: Merge #10530: Fix invalid instantiation and possibly unsafe accesses of array…
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nakihito created this revision.Fri, Jul 5, 20:25
Owners added a reviewer: Restricted Owners Package.Fri, Jul 5, 20:25
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Jul 5, 20:25
Fabien added inline comments.Fri, Jul 5, 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.Fri, Jul 5, 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.Sat, Jul 6, 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.Sat, Jul 6, 06:39
nakihito updated this revision to Diff 10028.Sat, Jul 6, 06:44

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

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