621818 - [OS/2] implement GenerateRandomBytes()

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Rich Walsh]

Bug 617111 added this function to places/src/Helpers.cpp for Win and Unix.  This patch implements it for OS/2 (and fixes a build break).

Comment 1

[Rich Walsh]

Attachment: implement GenerateRandomBytes()

Comment 2

[Shawn Wilsher :sdwilsh]

I found that generating guids with random numbers wasn't that great (guids were very similar).  Additionally, arc4random does not appear to be threadsafe.

I think it would be better to use nsIRandomGenerator::GenerateRandomBytes here, and you will have to init NSS here (by simply doing a do_GetService):
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/SQLFunctions.cpp#596

You might want to take a look at bug 617111 where we removed the old NSS code due to a Ts regression.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 500139 [details] [diff] [review]
implement GenerateRandomBytes()

r- per comment 2

Comment 4

[Rich Walsh]

(In reply to comment #2)

> I found that generating guids with random numbers wasn't that great
> (guids were very similar).

There must be some subtlety here that I'm missing.  What is the difference between "random bytes" and "random numbers"?  To my naive eye, it certainly looks like random numbers are being used for other platforms (generated a byte at a time perhaps?).

> I think it would be better to use nsIRandomGenerator::GenerateRandomBytes
> here

If this wasn't acceptable for other platforms, why would it be acceptable for OS/2?

> Additionally, arc4random does not appear to be threadsafe.

This is the one objection I fully understand.  I'll check our FreeBSD-based libc to see if it has this limitation.

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> There must be some subtlety here that I'm missing.  What is the difference
> between "random bytes" and "random numbers"?  To my naive eye, it certainly
> looks like random numbers are being used for other platforms (generated a byte
> at a time perhaps?).
Sorry, I should have elaborated a bit more.  There are two issues here really:
1) arc4random isn't a good PRNG for how you are using it.  I'm seeing conflicting information online about the range of arc4random, but it doesn't cover the full range of an unsigned int, which means it won't have an even distribution of bits.
2) Because of (1), you end up having a set of bits that is more likely to be repeated, which makes the guid we generate from the random bytes far less unique.

You could possibly work around this by looking at byte chunks:
for (PRUint32 i = 0; i < aSize; i++) {
  _buffer[i] = arc4random() % sizeof(PRUint8);
}

Running test_sql_guid_functions.js can show you how unique guids are.  One of the tests in that file generates guids until every valid character is seen in every position, and it runs for a very long time when you don't have a good random number generator (my first attempt at not using nsIRandomGenerator::GenerateRandomBytes was very similar to yours).

> If this wasn't acceptable for other platforms, why would it be acceptable for
> OS/2?
Well, on other platforms we had other options (/dev/urandom or the windows API).

Comment 6

[Rich Walsh]

Attachment: implement GenerateRandomBytes - v2

This version uses nsIRandomGenerator, as suggested.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 510137 [details] [diff] [review]
implement GenerateRandomBytes - v2

>+++ b/toolkit/components/places/src/Helpers.cpp
>@@ -293,6 +296,17 @@ GenerateRandomBytes(PRUint32 aSize,
>     (void)PR_Close(urandom);
>   }
>   return rv;
>+#elif defined(XP_OS2)
>+  nsCOMPtr<nsIRandomGenerator> rg =
>+    do_GetService("@mozilla.org/security/random-generator;1");
>+  NS_ENSURE_STATE(rg);
>+
>+  PRUint8* temp;
>+  nsresult rv = rg->GenerateRandomBytes(aSize, &temp);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  memcpy(_buffer, temp, aSize);
>+  NS_Free(temp);
just pass in _bugger to GenerateRandomBytes (it's OK to do so).  Saves the temporary, and the avoids the memcpy.

r=sdwilsh

Comment 8

[Walter Meinl]

(In reply to comment #7)
> Comment on attachment 510137 [details] [diff] [review]
> implement GenerateRandomBytes - v2
> 

> just pass in _bugger to GenerateRandomBytes (it's OK to do so).  Saves the
> temporary, and the avoids the memcpy.
> 
> r=sdwilsh

s/_bugger/_buffer I'd guess (I'm not a native English speaker, therefore I googled for bugger and was blushing ;-)

Comment 9

[Rich Walsh]

(In reply to comment #7)
> >+  PRUint8* temp;
> >+  nsresult rv = rg->GenerateRandomBytes(aSize, &temp);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  memcpy(_buffer, temp, aSize);
> >+  NS_Free(temp);
>
> just pass in _buffer to GenerateRandomBytes (it's OK to do so).  Saves the
> temporary, and the avoids the memcpy.

I don't see how I can without leaking 12+ bytes on every call.  GenerateRandomBytes() uses NS_Alloc() to allocate |aSize| bytes.  When would they ever be freed?

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> s/_bugger/_buffer I'd guess (I'm not a native English speaker, therefore I
> googled for bugger and was blushing ;-)
Er, right.  Sorry about that.

(In reply to comment #9)
> I don't see how I can without leaking 12+ bytes on every call. 
> GenerateRandomBytes() uses NS_Alloc() to allocate |aSize| bytes.  When would
> they ever be freed?
Bah, this isn't an XPCOM method, so you are correct.  You'll have to keep that there, my bad.

Comment 11

[Walter Meinl]

Comment on attachment 510137 [details] [diff] [review]
implement GenerateRandomBytes - v2

per comment #10 should be ready to go as it is.
Practically zero risk for tier_1, all in ifdefed sections for OS/2

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 510137 [details] [diff] [review]
implement GenerateRandomBytes - v2

a=beltzner, NPOTB really

Comment 13

[Walter Meinl]

> NPOTB really

beltzner, we, the OS/2 people are always a bit unsure if we have to ask for approval in cases like this, when there are changes in cross-platform files that are ifdefed for OS/2. I'd be happy to ease the burden for you and others who have to approve it.

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/8e79ca5d33fe