Add WTF::cryptographicallyRandomNumber
Created attachment 81775 [details] Patch
Attachment 81775 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7847345
Created attachment 81780 [details] Patch
Attachment 81775 [details] did not build on win: Build output: http://queues.webkit.org/results/7819395
Created attachment 81781 [details] Patch
Attachment 81775 [details] did not build on qt: Build output: http://queues.webkit.org/results/7724346
Comment on attachment 81775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81775&action=review Seems OK, but I would give it one more round of bathing. > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:51 > +static int randomStreamInitialized; > +static struct ARC4Stream randomStream; > +static int streamCount; Really this should just all be a class and have one shared instance of said class. A bunch of globals and inline global functions isn't very webkitty. > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:56 > +inline void initMutexIfNeeded() WebKit would use a method with a static variable in it, I think. No? sharedMutex(), etc. > Source/JavaScriptCore/wtf/OSRandomSource.cpp:59 > + // FIXME: What do we do if we can't read /dev/urandom? CRASH() maybe? > Source/JavaScriptCore/wtf/OSRandomSource.cpp:63 > + // WARNING: When adding new sources of OS randomness, the randomness must > + // be of cryptographic quality! Do you want to add a line like "if you ahve any doubt, please CC abarth on the bug?" :) > Source/JavaScriptCore/wtf/OSRandomSource.h:34 > +// partially filled. Rather than calling this function directly, consider Seems odd that there is no way to return to the caller that it was a partial fill.
Created attachment 81784 [details] Patch
Comment on attachment 81784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81784&action=review Wow, looks so much better! Of course now that you've cleaned it up more, I understand it more, and thus have more questions about the rest of it. I'd like to see one more round if you have the stamina. > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:44 > +struct ARC4Stream { This could be internal to the class, but it's OK as is. > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:78 > + for (int n = 0; n < 256; n++) > + m_stream.s[n] = n; > + m_stream.i = 0; > + m_stream.j = 0; This feels like the constructor for the stream class. > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:81 > +void ARC4RandomNumberGenerator::addRandomData(unsigned char *data, int length) * goes to the left. Should this take a Vector/CString type as input? > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:85 > + m_stream.i = (m_stream.i + 1); This is just ++. > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:87 > + m_stream.j = (m_stream.j + si + data[n % length]); This is just +=. > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:89 > + m_stream.s[m_stream.i] = m_stream.s[m_stream.j]; > + m_stream.s[m_stream.j] = si; I don't really understand what this is doing, but that's probably OK. :) > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:105 > + m_count = 1600000; What's this magical value do? It looks like it makes us stir every 40,000 numbers. Why? Seems we should add that as a comment? > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:110 > + if (m_count <= 0 || !m_initialized) Does count ever wrap? Or is this just a double-check of m_initialized? (since they're both set to 0 in teh constructor). > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:116 > + m_stream.i = (m_stream.i + 1); ++. This feels familiar. > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:118 > + m_stream.j = (m_stream.j + si); Again +=. Difficult to tell what this is doing. Should this be sharing code with addRandomData? > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:132 > + val = getByte() << 24; > + val |= getByte() << 16; > + val |= getByte() << 8; > + val |= getByte(); > + return val; I would probably have just written this as one line, but maybe that's lazy of me. > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:143 > + m_count -= 4; Why is this decrement not in getWord() itself? > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:160 > + if (--m_count <= 0) > + stir(); Isn't this just stirIfNeeded()?
Attachment 81784 [details] did not build on qt: Build output: http://queues.webkit.org/results/7819417
I'm slightly hesitant to change the core algorithm for fear of screwing it up, but I'll try to be careful.
Comment on attachment 81784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81784&action=review >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:44 > > This could be internal to the class, but it's OK as is. It's already in an anonymous namespace! How much more hidden do you want. :) >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:78 >> + for (int n = 0; n < 256; n++) >> + m_stream.s[n] = n; >> + m_stream.i = 0; >> + m_stream.j = 0; > > This feels like the constructor for the stream class. Done. >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:81 >> +void ARC4RandomNumberGenerator::addRandomData(unsigned char *data, int length) > > * goes to the left. Should this take a Vector/CString type as input? I've moved the * but left it non-vectorized. >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:85 >> + m_stream.i = (m_stream.i + 1); > > This is just ++. k >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:87 >> + m_stream.j = (m_stream.j + si + data[n % length]); > > This is just +=. k >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:89 >> + m_stream.s[m_stream.j] = si; > > I don't really understand what this is doing, but that's probably OK. :) It's just RC4. >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:105 >> + m_count = 1600000; > > What's this magical value do? It looks like it makes us stir every 40,000 numbers. Why? Seems we should add that as a comment? It's just some state of the RC4 algorithm. I think the value is pretty arbitrary with a large margin of safety. >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:110 >> + if (m_count <= 0 || !m_initialized) > > Does count ever wrap? Or is this just a double-check of m_initialized? (since they're both set to 0 in teh constructor). m_count goes down every time you pull out randomness. I've removed m_initialized. >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:116 >> + m_stream.i = (m_stream.i + 1); > > ++. This feels familiar. done. >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:118 >> + m_stream.j = (m_stream.j + si); > > Again +=. Difficult to tell what this is doing. Should this be sharing code with addRandomData? Hum.. That seems like too risky a change. I've changed this to +=, but I haven't changed this to share code. >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:143 >> + m_count -= 4; > > Why is this decrement not in getWord() itself? Maybe so stirIfNeeded sees the lower count? Dunno. >> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:160 >> + if (--m_count <= 0) >> + stir(); > > Isn't this just stirIfNeeded()? Done.
Created attachment 81845 [details] Patch
Comment on attachment 81845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81845&action=review Looks good. Thanks for all the cleanup! > Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:164 > + stirIfNeeded(); I would have just put all the stirIfNeeded and count stuff in getByte and bumped the initial m_count up by how much one is supposed to disgard at the beginning (or just reset the m_count after the initial discard). But this is OK.
Comment on attachment 81845 [details] Patch Clearing flags on attachment: 81845 Committed r78149: <http://trac.webkit.org/changeset/78149>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/78149 might have broken Qt Windows 32-bit Release
Can we merge OSRandomSource.h with RandomNumber.h? They are doing very similar things and both contain only one function.
(In reply to comment #18) > Can we merge OSRandomSource.h with RandomNumber.h? They are doing very similar things and both contain only one function. Conceptually there are very different different. OSRandomSource is an abstraction for OS-specific hardware randomness used internally by WTF. RandomNumber is a weak PRNG for use by clients of WTF. It would make more sense to have RandomNumberSeed use OSRandomSource on platforms where it's available.