Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
141753009:
Use RtlGenRandom() directly instead of going through rand_s(). (Closed)
|
Created:
6 years, 11 months ago by DaleCurtis Modified:
6 years, 11 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
http://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUse RtlGenRandom() directly instead of going through rand_s().
Results in a massive 44x improvement to RandBytes() for larger
buffer sizes. Using the RandBytesPerf test, the results are:
Original: 2961196.6us
Modified: 63656.8us
Improvement: 44x
BUG=none
TEST=randbytes unittests.
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246736
Patch Set 1 : Fix types. #
Total comments: 12
Patch Set 2 : Comments. #
Total comments: 3
Patch Set 3 : Use RtlGenRandom directly. #Patch Set 4 : Add comments. #
Total comments: 4
Patch Set 5 : Comments. #Patch Set 6 : Add missing header. #Messages
Total messages: 18 (0 generated)
Now with more Windows!
To answer Brad's question about "Why not CryptGenRandom" - because it's slow and multiple layers of indirection and may go into something that cannot be accessed from the sandbox (eg: a smart card) the "default" CryptGenRandom (eg: when using the Microsoft Base Cryptographic Service Provider) is just a shim to RtlGenRandom, which is the FIPS 140-2 validated dual-AES RNG. This just avoids like 4 extra levels of indirection, for the same security guarantees. It would only matter if we cared about FIPS. Which we don't, and for good reasons. https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc File base/rand_util_win.cc (right): https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:41: uint64 number; Free fixup while you're here - http://src.chromium.org/viewvc/chrome/trunk/src/base/basictypes.h?revision=24... (would have been nicer if we didn't split this out :P) Not needed in this CL though, just FYI. https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:50: const uint32 output_bytes_this_pass = use uint32_t and use stdint.h - https://code.google.com/p/chromium/codesearch#chromium/src/base/basictypes.h&... https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:51: std::min(output_length, static_cast<size_t>(kuint32max)); note: Don't you need a cast here, since you're providing two size_t types to std::min(), it returns a size_t (the C++03 25.3.7 definition only takes one type - T - not a return type) const uint32_t output_bytes_this_pass = static_cast<uint32_t>(std::min(...))
https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc File base/rand_util_win.cc (right): https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:41: uint64 number; On 2014/01/22 22:12:26, Ryan Sleevi wrote: > Free fixup while you're here - > http://src.chromium.org/viewvc/chrome/trunk/src/base/basictypes.h?revision=24... > (would have been nicer if we didn't split this out :P) > > Not needed in this CL though, just FYI. I've left this one alone for now since it requires changing the headers and all implementations. I'll put together a separate cleanup CL to get them all. https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:50: const uint32 output_bytes_this_pass = On 2014/01/22 22:12:26, Ryan Sleevi wrote: > use uint32_t and use stdint.h - > https://code.google.com/p/chromium/codesearch#chromium/src/base/basictypes.h&... Thanks for the pointer. I didn't realize this was the new hotness. Done. https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:51: std::min(output_length, static_cast<size_t>(kuint32max)); On 2014/01/22 22:12:26, Ryan Sleevi wrote: > note: Don't you need a cast here, since you're providing two size_t types to > std::min(), it returns a size_t (the C++03 25.3.7 definition only takes one type > - T - not a return type) > > const uint32_t output_bytes_this_pass = static_cast<uint32_t>(std::min(...)) Done.
LGTM
https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc File base/rand_util_win.cc (right): https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:21: ::GetProcAddress(::GetModuleHandle(L"advapi32.dll"), Any accomodation need to be made for failure here? Or is it fair to assume all that talk of it being altered or unavailable in subsequent versions is moot because srand_s uses it internally?
Patch set 2 LGTM. https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc File base/rand_util_win.cc (right): https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:21: ::GetProcAddress(::GetModuleHandle(L"advapi32.dll"), On 2014/01/22 22:31:29, bradn wrote: > Any accomodation need to be made for failure here? > Or is it fair to assume all that talk of it being altered or unavailable in > subsequent versions is moot because srand_s uses it internally? RtlGenRandom is already mentioned in some official Microsoft blog posts, so it cannot be altered or removed without causing big problems. https://codereview.chromium.org/141753009/diff/140001/base/rand_util_win.cc File base/rand_util_win.cc (right): https://codereview.chromium.org/141753009/diff/140001/base/rand_util_win.cc#n... base/rand_util_win.cc:7: #include <limits.h> I don't think you are using anything from the C header <limits.h>. You should instead include the C++ header that defines std::numeric_limits<uint32_t>::max() and std::min. Also, I would use ULONG instead of uint32_t throughout this file because ULONG is the type used by RtlGenRandom. https://codereview.chromium.org/141753009/diff/140001/base/rand_util_win.cc#n... base/rand_util_win.cc:15: namespace { Nit: add a blank line after this line. https://codereview.chromium.org/141753009/diff/140001/base/rand_util_win.cc#n... base/rand_util_win.cc:49: const WinRandom* win_random_ptr = g_win_random.Pointer(); Nit: this should be const WinRandom* const win_random_ptr = ... in your style.
lgtm If the dll can't be loaded on some systems, the crash servers will tell us.
https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc File base/rand_util_win.cc (right): https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:21: ::GetProcAddress(::GetModuleHandle(L"advapi32.dll"), One thing I forgot. Contrary to popular belief, this is officially supported, does have a proper import library, and doesn't need an explicit dynamic import. See the note at this link regarding NTSecAPI.h: http://msdn.microsoft.com/en-us/library/windows/desktop/aa387694(v=vs.85).aspx
PTAL. No more lazy initializer needed. https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc File base/rand_util_win.cc (right): https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:21: ::GetProcAddress(::GetModuleHandle(L"advapi32.dll"), On 2014/01/22 23:04:24, Justin Schuh wrote: > One thing I forgot. Contrary to popular belief, this is officially supported, > does have a proper import library, and doesn't need an explicit dynamic import. > See the note at this link regarding NTSecAPI.h: > http://msdn.microsoft.com/en-us/library/windows/desktop/aa387694%28v=vs.85%29... This compiles and works using the workaround mentioned there. https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:51: std::min(output_length, static_cast<size_t>(kuint32max)); On 2014/01/22 22:27:53, DaleCurtis wrote: > On 2014/01/22 22:12:26, Ryan Sleevi wrote: > > note: Don't you need a cast here, since you're providing two size_t types to > > std::min(), it returns a size_t (the C++03 25.3.7 definition only takes one > type > > - T - not a return type) > > > > const uint32_t output_bytes_this_pass = static_cast<uint32_t>(std::min(...)) > > Done. Sadly this doesn't seem to work with MSVC, it refused to compile indicating that the type was ambiguously "size_t" or "unsigned long".
lgtm for all the windowsieness. https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc File base/rand_util_win.cc (right): https://codereview.chromium.org/141753009/diff/80001/base/rand_util_win.cc#ne... base/rand_util_win.cc:51: std::min(output_length, static_cast<size_t>(kuint32max)); On 2014/01/22 23:50:51, DaleCurtis wrote: > On 2014/01/22 22:27:53, DaleCurtis wrote: > > On 2014/01/22 22:12:26, Ryan Sleevi wrote: > > > note: Don't you need a cast here, since you're providing two size_t types to > > > std::min(), it returns a size_t (the C++03 25.3.7 definition only takes one > > type > > > - T - not a return type) > > > > > > const uint32_t output_bytes_this_pass = static_cast<uint32_t>(std::min(...)) > > > > Done. > > Sadly this doesn't seem to work with MSVC, it refused to compile indicating that > the type was ambiguously "size_t" or "unsigned long". I think the explicit cast is actually correct, as MSVC has ramped up its warnings for 64-bit unsafe coercions.
Patch set 4 LGTM. Just some nits. https://codereview.chromium.org/141753009/diff/310001/base/rand_util_win.cc File base/rand_util_win.cc (right): https://codereview.chromium.org/141753009/diff/310001/base/rand_util_win.cc#n... base/rand_util_win.cc:15: #include <stdint.h> 1. You can remove <stdint.h> now that you've replaced uint32_t with ULONG. 2. I suggest including <<NTSecAPI.h> after <windows.h>. I believe that will eliminate the need to include <wtypes.h>. Some Windows system headers need to be included in a particular order, so we can't always list them in alphabetical order as the Style Guide recommends. 3. I believe we can also remove <stdlib.h>. It is apparently included for rand_s(). https://codereview.chromium.org/141753009/diff/310001/base/rand_util_win.cc#n... base/rand_util_win.cc:37: RtlGenRandom(output_ptr, output_bytes_this_pass) == TRUE; Nit: this is a give-away for my old-school C programming background :-) It is slightly cheaper to test != FALSE instead.
Thanks for the reviews everyone! https://codereview.chromium.org/141753009/diff/310001/base/rand_util_win.cc File base/rand_util_win.cc (right): https://codereview.chromium.org/141753009/diff/310001/base/rand_util_win.cc#n... base/rand_util_win.cc:15: #include <stdint.h> On 2014/01/23 18:06:49, wtc wrote: > > 1. You can remove <stdint.h> now that you've replaced uint32_t with ULONG. > > 2. I suggest including <<NTSecAPI.h> after <windows.h>. I believe that will > eliminate the need to include <wtypes.h>. Some Windows system headers need to be > included in a particular order, so we can't always list them in alphabetical > order as the Style Guide recommends. > > 3. I believe we can also remove <stdlib.h>. It is apparently included for > rand_s(). All done. Even 2, which I swore I tried and didn't compile. https://codereview.chromium.org/141753009/diff/310001/base/rand_util_win.cc#n... base/rand_util_win.cc:37: RtlGenRandom(output_ptr, output_bytes_this_pass) == TRUE; On 2014/01/23 18:06:49, wtc wrote: > > Nit: this is a give-away for my old-school C programming background :-) > > It is slightly cheaper to test != FALSE instead. Done. :)
Patch set 5 LGTM!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/141753009/190002
Patch set #6 adds a missing <algorithm> header for std::min (which broke the VS2013 build) from https://codereview.chromium.org/146083002/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/141753009/540001
Message was sent while issue was closed.
Change committed as 246736 |
