Add support for Linux-specific getrandom call to obtain random data. #75
Conversation
There was a problem hiding this comment.
Is the performance difference between getentropy and getrandom so much that it warrants another code path?
In the test Jamfile you may want to add explicit cases that force both of the linux implementations to execute in positive and negative test cases to ensure test coverage. Without that, most (maybe all) CI builds will test getrandom and nothing will be testing getentropy any more.
| @@ -15,6 +15,8 @@ | |||
| # include <boost/uuid/detail/random_provider_arc4random.ipp> | |||
| #elif defined(BOOST_UUID_RANDOM_PROVIDER_BCRYPT) | |||
| # include <boost/uuid/detail/random_provider_bcrypt.ipp> | |||
| #elif defined(BOOST_UUID_RANDOM_PROVIDER_GETRANDOM) | |||
There was a problem hiding this comment.
Was trying to keep these in alphabetical order :)
| { | ||
| #if defined(BOOST_UUID_RANDOM_PROVIDER_GETRANDOM_IMPL_GETRANDOM) | ||
| return BOOST_UUID_RANDOM_PROVIDER_GETRANDOM_IMPL_GETRANDOM(buf, size, flags); | ||
| #elif defined(BOOST_UUID_RANDOM_PROVIDER_GETRANDOM_HAS_LIBC_WRAPPER) |
There was a problem hiding this comment.
To implement the unit test could you have the mock implement ::getrandom and have the mock header define BOOST_UUID_RANDOM_PROVIDER_GETRANDOM_HAS_LIBC_WRAPPER? Unfortunately we're going to have a drop in coverage on the syscall since that (probably) can't be mocked.
There was a problem hiding this comment.
I don't like redefining system functions. I've seen you did this with getentropy and I deliberately did not follow this route. I think, that mock should be changed as well, but didn't do it in this PR as it was unrelated.
| #if BOOST_LIB_C_GNU >= BOOST_VERSION_NUMBER(2, 25, 0) | ||
| #define BOOST_UUID_RANDOM_PROVIDER_GETRANDOM_HAS_LIBC_WRAPPER | ||
| #elif defined(__has_include) | ||
| #if __has_include(<sys/random.h>) |
There was a problem hiding this comment.
I've never seen __has_include before. I was just thinking about how nice it would be to have the following include syntax in C++:
#include <boost/uuid/sha1.hpp> or <boost/uuid/detail/sha1.hpp>
To handle headers that move without leaving behind forwarding headers.
There was a problem hiding this comment.
I've never seen __has_include before.
It's one of the SD-6 macros most recent compilers support.
I don't expect performance to be significantly different. The main reasons for this backend are (a) supporting more platforms (glibc < 2.25) and (b) using the more appropriate API for random data generation. As I noted,
I'm not sure it is worth doing. On Linux, |
|
Yes, since we don't have a Solaris CI environment, it's nice to ensure the code doesn't rot somehow. |
getrandom is the base implementation for getentropy on Linux. It is also available in the form of a syscall, which can be called directly on systems with glibc versions older than 2.25 which don't yet provide wrappers for getrandom or getentropy but have a recent enough Linux kernel (for example, Debian Stretch). On systems other than Linux (e.g. Solaris) getentropy is documented as a source for initialization of a user-space PRNG instead of a direct source for random data. Since we use the random data directly to initialize UUIDs, using getrandom on other platforms, where available, would be more preferable than getentropy. Unfortunately, the only other platform that is known to support both getentropy and getrandom (Solaris) documents getrandom to return 0 on error, which is a valid return value on Linux. It's not clear if this is a documentation error or a true incompatibility, and I have no way to test, so do not use getrandom on Solaris for now. For this reason (and in case if it is also used on some other platforms) getentropy backend is still preserved. For running tests in the CI on the getentropy backend added the BOOST_UUID_RANDOM_PROVIDER_DISABLE_GETRANDOM macro, which will disable getrandom if one is detected. Currently, the macro is only used for tests on Linux.
Lastique
force-pushed
the
add_getrandom_backend
branch
from
c0cebdc
to
6c060f6
Compare
Codecov Report
@@ Coverage Diff @@
## develop #75 +/- ##
========================================
Coverage 78.34% 78.34%
========================================
Files 13 13
Lines 605 605
Branches 156 156
========================================
Hits 474 474
Misses 17 17
Partials 114 114Continue to review full report at Codecov.
|
|
I've updated the PR. For tests, I've added the internal |
|
::syscall(SYS_getrandom, buf, size, flags) failed on ubuntu 16.04.1 throws an exception blow: system kernel info: |
|
I've replied in #79. |
getrandom is the base implementation for getentropy on Linux. It is also
available in the form of a syscall, which can be called directly on systems with
glibc versions older than 2.25 which don't yet provide wrappers for getrandom or
getentropy but have a recent enough Linux kernel (for example, Debian Stretch).
On systems other than Linux (e.g. Solaris) getentropy is documented as a
source for initialization of a user-space PRNG instead of a direct source
for random data. Since we use the random data directly to initialize UUIDs,
using getrandom on other platforms, where available, would be more preferable
than getentropy. Unfortunately, the only other platform that is known to support
both getentropy and getrandom (Solaris) documents getrandom to return 0
on error, which is a valid return value on Linux. It's not clear if this is
a documentation error or a true incompatibility, and I have no way to test,
so do not use getrandom on Solaris for now. For this reason (and in case
if it is also used on some other platforms) getentropy backend is still
preserved.