Refactoring of random provider #53

jeking3 · 2017-11-08T21:42:19Z

Changed the default random_generator implementation to use
operating-system provided entropy as it is more secure and
faster for the typical use case of generating one uuid at
a time.

This is a breaking change for anyone passing a mt19937
into one of the explicit constructors of random_generator,
which would be quite rare.

Changed the default random provider on Windows to use BCrypt
where available, falling back to Wincrypt when necessary or
when explicitly requested through a macro.

Provide a new random_generator_mt19937 type definition for
use cases where a large number of uuids need to be created
with high performance. This is equivalent to the previous
definition of random_generator.

Provide a random generation benchmark test showing the
cutoff where the mt19937-based generator will outperform the
standard generator based on wall time.

Removed template specialization for boost::random::random_device
so that any UniformRandomNumberGenerator can be used properly
with random_generator.

Replaced the seed_rng detail implementation (which had a number
of flaws) with a replacement header-only random_provider
implementation.

Note: entropy generation errors will cause an entropy_error
to be thrown from random_generator. The previous implementation
ignored errors and silently failed.

Added internal support for entropy generation on cloudabi
platform leveraging the new random_device implementation.

Added internal support for Universal Windows Platform (UWP)
development leveraging the new random_device implementation.

Added internal support for getentropy() on Linux and OpenBSD
if certain requirements are met.

This is an evolution of #52
This fixes #24

codecov · 2017-11-08T21:58:59Z

Codecov Report

Merging #53 into develop will decrease coverage by 0.21%.
The diff coverage is 98.14%.

@@             Coverage Diff             @@
##           develop      #53      +/-   ##
===========================================
- Coverage    97.41%   97.19%   -0.22%     
===========================================
  Files           11       13       +2     
  Lines          619      607      -12     
===========================================
- Hits           603      590      -13     
- Misses          16       17       +1

Impacted Files	Coverage Δ
include/boost/uuid/basic_name_generator.hpp	`100% <ø> (ø)`	⬆️
include/boost/uuid/random_generator.hpp	`100% <100%> (ø)`	⬆️
include/boost/uuid/entropy_error.hpp	`100% <100%> (ø)`
include/boost/uuid/detail/random_provider.hpp	`100% <100%> (ø)`
...nclude/boost/uuid/detail/random_provider_posix.ipp	`96% <96%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a48cab...b27755e. Read the comment docs.

pdimov · 2017-11-08T22:29:07Z

include/boost/uuid/detail/random_provider.hpp

+    template<class Iter>
+    void generate(Iter first, Iter last)
+    {
+        BOOST_STATIC_ASSERT(sizeof(Iter) >= sizeof(unsigned int));


This checks the iterator size, which is irrelevant. It should check

whether the value_type of the iterator is integral;

whether the value_type of the iterator is unsigned;

whether numeric_limits<value_type>::max() is >= 2^32-1, but this can only be tested at compile time on C++11, so

whether sizeof(value_type) * CHAR_BIT >= 32.

Okay, I will update this.

I tried this but it didn't work - I'll keep on it.

typedef Iter::value_type iter_value_type; BOOST_STATIC_ASSERT(is_integral<iter_value_type>::value == true_type); BOOST_STATIC_ASSERT(is_unsigned<iter_value_type>::value == true_type); BOOST_STATIC_ASSERT(sizeof(iter_value_type) * CHAR_BIT >= 32);

In reading SeedSeq documentation, Iter can be a class with a value_type type definition of an unsigned integral and an operator*, or it can be a pointer to an unsigned integral, correct? I'm not certain (yet) how I would test for both.

typedef std::iterator_traits<Iter>::value_type value_type;

Then just BOOST_STATIC_ASSERT( boost::is_integral<value_type>::value );, comparing to the type true_type doesn't make sense.

It will work in C++11 because true_type has a constexpr conversion to true but is still nonsensical. :-)

I want to support C++03, will it still work?
I don't think I have ever used std::iterator_traits before.
I'm learning a ton of new tricks, thanks!

pdimov · 2017-11-08T22:31:05Z

include/boost/uuid/detail/random_provider.hpp

+        for (; first != last; ++first)
+        {
+            get_random_bytes(&*first, sizeof(*first));
+            *first &= (std::numeric_limits<unsigned int>::max)();


Not correct. generate is required to produce numbers in [0..2^32-1], regardless of the size of unsigned int.

Got it, I will update this.

pdimov · 2017-11-08T22:32:40Z

include/boost/uuid/detail/random_provider.hpp

+#include <boost/uuid/entropy_error.hpp>
+
+#include <boost/uuid/detail/random_provider_detect_platform.hpp>
+#include BOOST_UUID_RANDOM_PROVIDER_BASE_INCLUDE()


It's better to just include the appropriate platform header from random_provider_detect_platform.hpp, renaming it into random_provider_base.hpp in the process, because the above confuses dependency scanners; they can't see through the macro.

I needed platform detection separate from loading the implementation to make the mocking work properly. I can expand that out so the macro isn't there and put additional preprocessor branches based on the BOOST_UUID_RANDOM_PROVIDER_* definitions made by the detection logic in that header.

Yes, it's better to spell it out with #ifdefs instead of a macro include.

pdimov · 2017-11-08T22:34:39Z

doc/uuid.html

+        <ul>
+            <li>
+                <tt>BOOST_UUID_RANDOM_PROVIDER_FORCE_POSIX</tt>: on Unix this will force the
+                selection of <tt>/dev/*random</tt> over getentropy(3).


Prefer to not provide this. By all means give people a macro to override behavior when they ask for it, no need to to it preemptively.

I'm leveraging this to get coverage and a (mostly) common sad path test. I'll look at testing against the posix.ipp implementation directly when running on linux.

Yes, I hadn't yet gotten to the testing part. Keep those macros. I was looking at them from the user side.

That'll save a bunch of additional work... thanks.

pdimov · 2017-11-08T22:35:34Z

doc/uuid.html

+</li>
+            <li>
+                <tt>BOOST_UUID_RANDOM_PROVIDER_FORCE_WINCRYPT</tt>: on Windows this will force the
+                selection of Wincrypt over BCrypt.


Ditto. No need to ever prefer the older API. If someone asks for it, fine; until then, no need.

I'm leveraging this to get coverage and a (mostly) common sad path test. This test is more complex as I have to build a DLL and link against it, but I can make another test file that's specific to the wincrypt.ipp file and loads the mock library.

pdimov · 2017-11-08T22:36:56Z

doc/uuid.html

+            </li>
+            <li>
+                <tt>BOOST_UUID_RANDOM_PROVIDER_POSIX_BLOCKING</tt>: when using <tt>/dev/*random</tt>,
+                if this is defined, skip attempting to open <tt>/dev/urandom</tt> first.


Nope. Not only should /dev/random not be configurable to be used as a preference, it should never be used, anywhere, for anything, as the OpenBSD docs say.

Easy to remove. Thanks. Less options too and it simplifies the code and the sad test path that has to account for two open()s in the failure case.

pdimov · 2017-11-08T22:39:08Z

include/boost/uuid/detail/random_provider_bcrypt.ipp

+        boost::winapi::NTSTATUS_ status =
+            boost::winapi::BCryptGenRandom(
+                hProv_,
+                reinterpret_cast<boost::winapi::PUCHAR_>(buf),


static_cast is better, when it works, which it does here.

Agree, I'll change it. I usually try static_cast first and let the compiler force me the other way.

pdimov · 2017-11-08T22:40:07Z

include/boost/uuid/detail/random_provider_detect_platform.hpp

+
+#include <boost/predef/library/c/gnu.h>
+#include <boost/predef/platform/cloudabi.h>
+// #include <boost/predef/os/bsd/open.h> re-enable when Boost.Predef #66 clears


No need to wait for Predef. The functions we need are available on OpenBSD 2.5 and above.

Good to know, I was planning on getentropy() there so... (on to the next comment)

pdimov · 2017-11-08T22:42:38Z

include/boost/uuid/detail/random_provider_detect_platform.hpp

+#elif BOOST_LIB_C_GNU >= BOOST_VERSION_NUMBER(2, 25, 0) \
+   && !defined(BOOST_UUID_RANDOM_PROVIDER_FORCE_POSIX)
+/* when Boost.Prefef #6 merges, allow this based on OpenBSD documentation:
+BOOST_OS_BSD_OPEN >= BOOST_VERSION_NUMBER(5, 6, 0)) */


"getentropy() is not intended for regular code; please use the arc4random(3) family of functions instead."

https://man.openbsd.org/getentropy.2

Heed what the OpenBSD people say. They know their stuff. :-)

and there you go.

pdimov · 2017-11-08T22:46:39Z

include/boost/uuid/detail/random_provider_wincrypt.ipp

+#include <boost/throw_exception.hpp>
+#include <string>
+
+#if !defined(BOOST_UUID_RANDOM_PROVIDER_NO_LIB)


BOOST_ALL_NO_LIB, as I already said.

Sorry I missed that! Isn't each library capable of having its own preprocessor macros in this area, like BOOST_SYSTEM_NO_LIB, BOOST_CONFIG_NO_LIB, BOOST_PYTHON_NO_LIB...?

It is, but you have to check the global macro as well and not autolink if it's set. It's a global user switch that requests all autolinking to be disabled, anywhere.

In my builds on Windows with msvc-14.1, BOOST_ALL_NO_LIB is set to 1 by default. This seems odd - why would it be set on the way in? Boost.Uuid does not set it.

Interesting. No idea why it's set; you'll need to investigate.

It is defined for all builds of boost, it's in boost.jam. I will add BOOST_UUID_SOURCE and make it conditional on that as well. This pattern is found elsewhere.

pdimov · 2017-11-08T22:47:57Z

include/boost/uuid/detail/random_provider_wincrypt.ipp

+#if defined(BOOST_NO_ANSI_APIS)
+          boost::winapi::CryptAcquireContextW
+#else
+          boost::winapi::CryptAcquireContextA


No reason to ever use the A function here, if you ask me.

I believe you may be right now that the provider string / enumeration stuff is gone. If so I'll simplify and remove the additional test for this macro.

pdimov · 2017-11-08T22:57:25Z

Ah. It now occurs to me that you need the configuration macros in order to force-test a provider when it wouldn't be selected otherwise. Makes sense.

jeking3 · 2017-11-08T23:13:20Z

That's what I was using them for, however I could write one test per provider implementation instead I suppose. I just wanted it to be as uniform as possible since they all follow the same pattern.

pdimov · 2017-11-08T23:16:47Z

Yes, you could include the implementation header directly and test that, which is still not a bad idea, but the other way is good too, as it can test the component integrated into the rest of the code. So, either way is fine with me, I suppose.

jeking3 · 2017-11-08T23:18:14Z

As an aside, we need to get this level of CI integration into every project.

pdimov · 2017-11-08T23:20:20Z

It still seems to me that RtlGenRandom is quite a convenient choice on Windows as it requires no setup and is quite likely a bit faster than firing up a bcrypt/wincrypt provider. I'm not entirely clear on why you dislike it so. :-)

jeking3 · 2017-11-08T23:35:03Z

I believe it would still have a setup cost insofar as needing to perhaps statically initialize the address of the API call through LoadLibrary and GetProcAddress, and in a previous PR I was told to avoid using statics. I agree those two calls are probably much lighter than wincrypt. I can give it a try.

pdimov · 2017-11-08T23:37:22Z

There's no need for LoadLibrary.

extern "C" unsigned char __stdcall SystemFunction036( void * p, unsigned long n );

works for me.

jeking3 · 2017-11-08T23:42:40Z

You are assuming that advapi32.dll has already been loaded, and I think that's an unsafe assumption to make.

pdimov · 2017-11-08T23:53:34Z

It works for me in a sample program. Not sure how I could be assuming anything if the program links.

pdimov · 2017-11-08T23:57:05Z

extern "C" unsigned char __stdcall SystemFunction036( void * p, unsigned long n );

bool RtlGenRandom( void * p, unsigned long n )
{
	return SystemFunction036( p, n );
}

#include <iostream>

int main()
{
	unsigned x = 0;
	bool r = RtlGenRandom( &x, sizeof(x) );

	std::cout << x << std::endl;
}

C:\Projects\testbed>g++ testbed.cpp

C:\Projects\testbed>a
725953787

C:\Projects\testbed>clang++ testbed.cpp

C:\Projects\testbed>a
1701069008

jeking3 · 2017-11-09T00:14:57Z

You are still relying on the fact that ADVAPI32.DLL was loaded. While it is in the default library list for most build systems, that can be overridden. ADVAPI32.DLL is not available on Windows CE. Pretty much everyone will be using bcrypt at this point with wincrypt left to cover older platforms. Since that code has worked for a very long time here and in Boost.Random, and seeing as to how it's out of the normal code path, doesn't it make sense to leave it as is?

Lastique · 2017-11-09T00:17:18Z

@pdimov The LoadLibrary/GetProcAddress approach is what MS suggests here. All in all, this function seems like one of those semi-legal half-documented APIs that are Windows implementation details and can change at any time. IMHO, we can use such APIs but only if we have to. If there is an official public API that covers our case, we should preferably use it.

jeking3 · 2017-11-09T00:24:23Z

Okay that actually wasn't too bad, I'm doing some testing...

pdimov · 2017-11-09T00:25:09Z

It's impossible for a library to not be loaded if you link to it. If that's unsafe, your code is at least 20 times as unsafe.

pdimov · 2017-11-09T01:24:13Z

include/boost/uuid/random_generator.hpp

          , distribution_type
-            ( (std::numeric_limits<unsigned long>::min)()
-            , (std::numeric_limits<unsigned long>::max)()
+            ( (std::numeric_limits<unsigned int>::min)()


Not clear why these changes from ulong to uint are needed. Would probably be better to keep the original to cut down on the delta unless there's a good reason to switch.

pdimov · 2017-11-09T01:29:15Z

test/Jamfile.v2

+	  <define>BOOST_UUID_SOURCE=1
+
+      # BOOST_AUTO_LINK does not work outside of MSVC on Windows so this makes cygwin, mingw-w64 work:
+      <toolset>gcc,<target-os>windows:<linkflags>-lbcrypt 


We could link here regardless of whether the toolset is gcc (there's also clang btw), and this will fix the lack of autolink when using Boost.Build. But the problem is that we don't necessarily know what to link to.

<linkflags>-lbcrypt isn't correct though, I think that we need lib bcrypt ; and then <library>bcrypt here.

I will try that and see what it does to the mocks.

Is there any way to negate this (<target-os>windows:<library>bcrypt), so that I can set it in project requirements so everything links against it by default, but then in a specific mock based test negate it so it does NOT link against bcrypt?

I vaguely recall that one could use <x>y:-<library>bcrypt to remove it, but I won't be surprised if it doesn't work and I'm imagining things. :-)

Turns out I'm not, in this case. http://www.boost.org/build/doc/html/bbv2/overview/targets.html#bbv2.overview.targets.requirements

Odd, test_serialization passed the second time. Mystery.

Instead of not testing anything, it might be better to disable just test_serialization on mingw and keep testing the rest.

Cygwin, on the other hand, passes with flying colors.

I am unable to use the syntax: <target-os>windows:-<library>bcrypt, or <target-os>windows:-<define>BOOST_ALL_NO_LIB - both fail in bjam as an invalid target-os. I'm going back to the logic I had.

If only there existed a simple function in advapi32 that we could just call and avoid all that hassle.

pdimov · 2017-11-09T01:30:28Z

test/Jamfile.v2

+    [ run test_detail_random_provider.cpp 
+      : : :
+        <define>BOOST_UUID_RANDOM_PROVIDER_FORCE_POSIX                  # will force POSIX over getentropy
+        <define>BOOST_UUID_RANDOM_PROVIDER_POSIX_BLOCKING               # will force /dev/random over /dev/urandom


No longer necessary.

pdimov · 2017-11-09T01:32:15Z

test/test_entropy_error.cpp

+
+    entropy_error err(6, "foo");
+    BOOST_TEST_EQ(6, err.errcode());
+    BOOST_TEST_EQ(std::string("foo"), std::string(err.what()));


Could use BOOST_TEST_CSTR_EQ here.

swatanabe · 2017-11-09T02:35:10Z

AMDG

On 11/08/2017 07:29 PM, James E. King, III wrote: jeking3 commented on this pull request. > @@ -7,10 +7,18 @@ project libs/uuid/test : requirements - # boost::random requires this setting for a warning free build: + # define BOOST_UUID_SOURCE to defeat the default BOOST_ALL_NO_LIB in boost.jam + <define>BOOST_UUID_SOURCE=1 + + # BOOST_AUTO_LINK does not work outside of MSVC on Windows so this makes cygwin, mingw-w64 work: + <toolset>gcc,<target-os>windows:<linkflags>-lbcrypt I am unable to use the syntax: `<target-os>windows:-<library>bcrypt`, or `<target-os>windows:-<define>BOOST_ALL_NO_LIB` - both fail in bjam as an invalid target-os. I'm going back to the logic I had.

The '-property' syntax is not very user-friendly. Basically, the property that you remove must be exactly the same as the property that was added: -<toolset>gcc,<target-os>windows:<linkflags>-lbcrypt In Christ, Steven Watanabe

jeking3 · 2017-11-09T14:20:38Z

I have some changes to support building for CloudABI compatibility since it's in this pull request, now that I was able to build the Boost.Uuid tests under CloudABI. These depend on boostorg/predef#68 so I won't push them until that merges.

jeking3 · 2017-11-09T14:58:55Z

One of the developers from CloudABI pointed me to arc4random in cloudlibc so I'm going to remove the cloudabi specific implementation and have it use arc4random.

jeking3 · 2017-11-09T15:00:42Z

I'm curious what you think about the scope of this change relative to the upcoming release. I have it marked as a milestone for 1.67.0 and not 1.66.0 right now because it is a lot of change, however it's well tested on every platform, both happy and sad paths.

pdimov · 2017-11-09T15:02:03Z

1.66.0 is closed for beta now so I'd say we missed that boat.

pdimov · 2017-11-09T15:14:23Z

appveyor.yml

+    - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
+      ADDPATH: C:\mingw\bin;
+      TOOLSET: gcc
+      CXXFLAGS: cxxflags=-std=c++03


The new way is cxxstd=03 instead of using cxxflags, which allows testing cxxstd=03,11 or even cxxstd=03,11,14,1z in one go instead of using multiple jobs (slow on Appveyor as they're serialized).

Sorry, I just re-enabled an old stanza here. The other ones should be using that. If not, I'll adjust.

All of the jobs at https://ci.appveyor.com/project/jeking3/uuid-gaamf/build/1.0.73-develop seem to be using cxxflags.

I was thinking about Travis CI, I'll get this fixed; it's queued up behind boostorg/predef#68 right now.

pdimov · 2017-11-09T15:17:01Z

include/boost/uuid/detail/random_provider_wincrypt.ipp

+#      define BOOST_LIB_NAME "advapi32"
+#   endif
+#   define BOOST_AUTO_LINK_NOMANGLE
+#   include <boost/config/auto_link.hpp>


I've a strong suspicion that these autolinks are unnecessary. Can't imagine "coredll" not being linked on CE, and "advapi32" is on desktop.

Hm, nope, a plain cl doesn't link advapi32 by default, so we do need it, unless Winapi autolinks it for us.

Interestingly, Cygwin/Mingw g++/clang++ do autolink advapi32 though.

pdimov · 2017-11-09T15:29:05Z

This looks good. I however still think that it's way more complex than it would have been with RtlGenRandom on desktop. It's even available by default on Cygwin/Mingw, so things automatically work there even though autolink's not available.

operating-system provided entropy as it is more secure and faster for the typical use case of generating one uuid at a time. This is a breaking change for anyone passing a mt19937 into one of the explicit constructors of random_generator, which would be quite rare. Changed the default random provider on Windows to use BCrypt where available, falling back to Wincrypt when necessary or when explicitly requested through a macro. Provide a new random_generator_mt19937 type definition for use cases where a large number of uuids need to be created with high performance. This is equivalent to the previous definition of random_generator. Provide a random generation benchmark test showing the cutoff where the mt19937-based generator will outperform the standard generator based on wall time. Removed template specialization for boost::random::random_device so that any UniformRandomNumberGenerator can be used properly with random_generator. Replaced the seed_rng detail implementation (which had a number of flaws) with a replacement header-only random_provider implementation. Note: entropy generation errors will cause an entropy_error to be thrown from random_generator. The previous implementation ignored errors and silently failed. Added internal support for entropy generation on cloudabi platform leveraging the new random_device implementation. Added internal support for Universal Windows Platform (UWP) development leveraging the new random_device implementation. Added internal support for getentropy() on Linux and OpenBSD if certain requirements are met.

jeking3 · 2017-11-10T12:34:29Z

This (hopyfully last) refresh adds changes to support actually running the test suite on cloudabi in a FreeBSD VM. I switched CloudABI to using arc4random as well.

jeking3 · 2017-11-11T03:57:54Z

@swatanabe with one more turn of the crank I think this would be something useful to consider moving into Boost.Random as a replacement for the existing random_device implementation. Given that the random_device inside Boost.Random today is the only piece of code referencing Boost.System, if we did this then Boost.Random would no longer depend on Boost.System. I don't know if that would free up Boost.Random from all library based dependencies though.

A fairly simple random_device implementation based on this would be (incomplete...):

jking@ubuntu:~/boost/libs/uuid/test$ cat ../include/boost/uuid/detail/random_device.hpp 
#include <boost/uuid/detail/random_provider.hpp>

namespace boost {
namespace uuids {
namespace detail {

template<class Entropy>
class random_device : public random_provider
{
public:
    Entropy operator()()
    {
        Entropy e;
        get_random_bytes(&e, sizeof(Entropy));
        return e;
    }
};
}}}


jking@ubuntu:~/boost/libs/uuid/test$ git diff HEAD test_random_generator.cpp
diff --git a/test/test_random_generator.cpp b/test/test_random_generator.cpp
index bd9566d..6d4e4a0 100644
--- a/test/test_random_generator.cpp
+++ b/test/test_random_generator.cpp
@@ -13,6 +13,7 @@
 #include <boost/detail/lightweight_test.hpp>
 #include <boost/predef/library/c/cloudabi.h>
 #include <boost/random.hpp>
+#include <boost/uuid/detail/random_device.hpp>
 #include <boost/uuid/entropy_error.hpp>
 #include <boost/uuid/random_generator.hpp>
 #include <boost/uuid/uuid_io.hpp>
@@ -117,5 +118,10 @@ int main(int, char*[])
     }
 #endif
 
+    boost::uuids::detail::random_device<unsigned int> randgen;
+    unsigned int i1 = randgen();
+    unsigned int i2 = randgen();
+    BOOST_TEST_NE(i1, i2);
+
     return boost::report_errors();
 }

This would improve the platform coverage of random_device to include UWP and CloudABI, and use more optimal calls on newer versions of Linux. The implementation is also fully tested (happy and sad paths), although the OpenBSD and CloudABI paths are tested manually unless I turn the build into a docker based build. (I have extensive work doing in the Apache Thrift project, so I know this would be possible).

jeking3 · 2017-11-15T17:22:39Z

Should I complete the implementation as a random_device replacement and submit it to Boost.Random?

jeking3 · 2017-12-05T15:38:13Z

Now that 1.66.0 is closing out, I want to revisit this, but I need an answer, so please reply:

Should I complete the implementation as a random_device replacement and submit it to Boost.Random instead?

The benefits are:

Broader platform support for entropy generation
More optimal entropy generation leveraging the latest APIs
Lower maintenance cost
Reduced library dependencies (by decoupling random from system)
Comprehensive happy and sad path testing with every build (to the extent current CI allows)

Thanks - Jim

Lastique · 2017-12-05T15:55:09Z

I thought we've been over this and Boost.Random submission failed, haven't we?

My opinion has not changed. You can do it however you like in implementation details of Boost.UUID. A fully compliant implementation of random_device in Boost.UUID details seem like an overkill to me; the functionality needed for Boost.UUID can be implemented much more simply. Depending on Boost.Random is only ok if it doesn't require linking with it (which basically means using random_device from Boost.Random is not an option).

pdimov · 2017-12-05T17:04:30Z

The right place to ask this question is the list.

jeking3 · 2017-12-07T23:33:52Z

I submitted a PR into Boost.Random: boostorg/random#34 - I hadn't heard back from Steven and didn't want to drag the issue up on the mailing list again.

jgzhuang · 2018-06-02T01:55:29Z

@jeking3 Would you mind sharing the details of the "flaws" you mentioned in "Replaced the seed_rng detail implementation (which had a number of flaws) with a replacement header-only random_provider implementation."?
I did notice a few strange things in the seed_rng in 1.66 and earlier (such as using the uninitialized content, new unsigned int and use the address and delete that unsigned int). But I am interested in the flaws you discovered.

Another implementation difference between seed_rng in 1.66 and the random_provider_base in 1.67 is that seed_rng uses std::fread while the random_provider_base uses ::read. With a synthetic testing it seems the latter is slower when siz is 4 although this only affects seeding.

jeking3 · 2018-06-02T23:00:07Z

In particular the code was:

relying on uninitialized variables
did not check for errors while obtaining entropy
had become difficult to maintain because the entropy generation was not well separated per platform.
was doing way too much to try and be random, reading in process ID, thread ID, time of day, etc. and then running it through a sha1 hash, then pulling some bits. Much better to use the operating system's entropy generator which was the primary purpose of the change, and to add more platform support.

If you have evidence that shows ::read is slower than std::fread then I suggest opening an issue or a pull request with the evidence and the fix.

Thanks!

jeking3 added the enhancement label Nov 8, 2017

jeking3 added this to the v1.67.0 milestone Nov 8, 2017

jeking3 self-assigned this Nov 8, 2017

jeking3 requested a review from pdimov November 8, 2017 21:42

pdimov reviewed Nov 8, 2017

View reviewed changes

pdimov reviewed Nov 9, 2017

View reviewed changes

jeking3 force-pushed the random_v2 branch from 056e4d9 to 873a9e1 Compare November 9, 2017 04:26

pdimov reviewed Nov 9, 2017

View reviewed changes

jeking3 force-pushed the random_v2 branch from 873a9e1 to 5eaaf97 Compare November 10, 2017 12:30

jeking3 force-pushed the random_v2 branch from 5eaaf97 to b27755e Compare November 10, 2017 12:32

pdimov mentioned this pull request Nov 27, 2017

CVE: Insecure generation of mask keys for websocket clients boostorg/beast#912

Closed

jeking3 mentioned this pull request Dec 7, 2017

Update the random_device implementation boostorg/random#34

Closed

jeking3 closed this in a8a1ec3 Dec 18, 2017

jeking3 deleted the random_v2 branch April 29, 2018 13:59

cronicc mentioned this pull request Jul 16, 2019

Upgrading libboost from 1.66 to 1.70 breaks shielded transactions in some OpenVZ environments HorizenOfficial/zen#184

Closed

Refactoring of random provider #53

Refactoring of random provider #53

Uh oh!

Conversation

jeking3 commented Nov 8, 2017

Uh oh!

codecov bot commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeking3 Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 8, 2017 •

edited

Loading

jeking3 Nov 9, 2017 •

edited

Loading

Lastique commented Nov 9, 2017 •

edited

Loading