Replace std::random_shuffle with std::shuffle #5727
Conversation
src/p2p/net_peerlist.h
Outdated
| @@ -290,7 +290,7 @@ namespace nodetool | |||
|
|
|||
| if (anonymize) | |||
| { | |||
| std::random_shuffle(bs_head.begin(), bs_head.end()); | |||
| std::shuffle(bs_head.begin(), bs_head.end(), std::default_random_engine(crypto::rand<unsigned>())); | |||
There was a problem hiding this comment.
I would switch this to:
std::shuffle(bs_head.begin(), bs_head.end(), crypto::random_device{});the std::default_random_engine doesn't have to be a "cryptographically secure" RNG. Otherwise this should be suitable for test purposes.
There was a problem hiding this comment.
This would of course be an easy change. If this is necessary here, though, does this solve a bug in the original code? std::random_shuffle also doesn't use a cryptographically secure RNG.
There was a problem hiding this comment.
Yes, this was/is a bug in the existing code.
tomsmeding
force-pushed
the
c++17-random_shuffle
branch
from
f81b8e3
to
0ceb991
Compare
|
I addressed @vtnerd's comments (thanks!) in the latest commit; I replaced all instances of std::shuffle, not only the important ones, to prevent people from being able to copy an "insecure" call over to a new call site where a "secure" one would be warranted. In that sense, this is a form of defense-in-depth. I think performance doesn't suffer too much from this. |
|
Can you squash the commits together? :) |
According to [1], std::random_shuffle is deprecated in C++14 and removed in C++17. Since std::shuffle is available since C++11 as a replacement and monero already requires C++11, this is a good replacement. A cryptographically secure random number generator is used in all cases to prevent people from perhaps copying an insecure std::shuffle call over to a place where a secure one would be warranted. A form of defense-in-depth. [1]: https://en.cppreference.com/w/cpp/algorithm/random_shuffle
tomsmeding
force-pushed
the
c++17-random_shuffle
branch
from
0ceb991
to
7b9a420
Compare
|
@selsta Of course, that makes a lot of sense here. :) Done just now. |
|
@selsta Thanks for the quick turnaround! |
7b9a420 Replace std::random_shuffle with std::shuffle (tomsmeding)
According to the docs, std::random_shuffle is deprecated in C++14 and removed in C++17. Since std::shuffle is available since C++11 as a replacement and monero already requires C++11, so it is a good idea to use std::shuffle instead. The choice of random number generator was inspired by the other usages of std::shuffle in the codebase; since the original std::random_shuffle made no guarantees whatsoever about the quality of its randomness, this does not introduce problems there. Ref: monero-project/monero#5727
According to the docs,
std::random_shuffleis deprecated in C++14 and removed in C++17. Sincestd::shuffleis available since C++11 as a replacement and monero already requires C++11, I think it is a good idea to usestd::shuffleinstead.The choice of random number generator was inspired by the other usages of
std::shufflein the codebase; since the originalstd::random_shufflemade no guarantees whatsoever about the quality of its randomness, this does not introduce problems there.