Closed Bug 1015547 Opened 5 years ago Closed 5 years ago

Use arc4random to generate random UUIDs on BSDs

Categories

(Core :: XPCOM, defect)

All
FreeBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jbeich, Assigned: jbeich)

Details

Attachments

(3 files, 6 obsolete files)

Attached patch fix (obsolete) — Splinter Review
arc4random(3) is the preferred method to get unique numbers on BSDs. On OpenBSD 5.5 it seems to use ChaCha20 instead of RC4.
Attachment #8428204 - Flags: review?(vladimir)
Comment on attachment 8428204 [details] [diff] [review]
fix

Looks good actually, but can you do a #define near the top that combines the giant block of if clauses?  e.g.

#if !defined(XP_WIN) && !defined(XP_MACOSX) && !defined(ANDROID) \
 && !defined(__DragonFly__) && !defined(__FreeBSD__) \
 && !defined(__NetBSD__) && !defined(__OpenBSD__)
#define NS_UUID_GENERATOR_NEEDS_SEED 0
#else
#define NS_UUID_GENERATOR_NEEDS_SEED 1
#endif

#if !defined(ANDROID) \
+ && !defined(__DragonFly__) && !defined(__FreeBSD__) \
+ && !defined(__NetBSD__) && !defined(__OpenBSD__)
#define NS_UUID_GENERATOR_BROKEN_SETSTATE 1
#define NS_UUID_GENERATOR_USE_ARC4RANDOM 1
#else
#define NS_UUID_GENERATOR_BROKEN_SETSTATE 0
#define NS_UUID_GENERATOR_USE_ARC4RANDOM 0
#endif
Attachment #8428204 - Flags: review?(vladimir) → review-
Attached patch v2 (autoconf) (obsolete) — Splinter Review
I'd rather not use cpp macros for what autoconf is better suited for. And arc4random doesn't need manual seeding, so broken setstate on GNU platforms is irrelevant. OS X supports arc4random as well but I'm leaving it out.
Attachment #8428204 - Attachment is obsolete: true
Attachment #8430522 - Flags: review?(vladimir)
Attachment #8430522 - Flags: review?(mh+mozilla)
Attached patch arc4random, v2 (autoconf) (obsolete) — Splinter Review
Oops, less pretentious speak in commit message.
Attachment #8430522 - Attachment is obsolete: true
Attachment #8430522 - Flags: review?(vladimir)
Attachment #8430522 - Flags: review?(mh+mozilla)
Attachment #8430523 - Flags: review?(vladimir)
Attachment #8430523 - Flags: review?(mh+mozilla)
This needs a Try build only on Android and Linux to make sure arc4random is properly detected. Landry, can you do it?
Flags: needinfo?(landry)
https://tbpl.mozilla.org/?tree=Try&rev=a4effb3f00cd

(note: i'm offline until the 10)
Flags: needinfo?(landry)
Actually, push _with_ the cset: https://tbpl.mozilla.org/?tree=Try&rev=8bab5bfbdb01

(i really need holidays)
Attachment #8430523 - Flags: review?(mh+mozilla) → review+
Try is mostly green. The following error on OS X is unlikely to be my fault:
- no other place uses #ifdef HAVE_ARC4RANDOM
- the patch is nop for XP_MACOSX and XP_WIN

InstallError: Failed to install "/builds/slave/try-osx64-d-000000000000000000/build/testing/mozbase/mozinstall/tests/Installer-Stubs/firefox.dmg"
Comment on attachment 8430523 [details] [diff] [review]
arc4random, v2 (autoconf)

Looks great, thanks!
Attachment #8430523 - Flags: review?(vladimir) → review+
Attached patch arc4random_buf (obsolete) — Splinter Review
With arc4random_buf available the code becomes even shorter:

NS_IMETHODIMP
nsUUIDGenerator::GenerateUUIDInPlace(nsID* aId)
{
  MutexAutoLock lock(mLock);

  arc4random_buf(aId, sizeof(*aId));
  aId->m2 &= 0x0fff;
  aId->m2 |= 0x4000;
  aId->m3[0] &= 0x3f;
  aId->m3[0] |= 0x80;

  return NS_OK;
}
Attachment #8431115 - Flags: review?(vladimir)
On Android and BSDs the test file by default uses arc4random(3). To force random(3) usage you have to mimic a GNU system.

$ cc uuidgen_test.c -o foo
$ cc uuidgen_test.c -D__GLIBC__ $(nspr-config --cflags --libs) -o bar
$ nm foo | fgrep random
                 U arc4random_buf@@FBSD_1.1
$ nm bar | fgrep random
                 U random@@FBSD_1.0
$ ./foo
83dc25c6-676b-465a-9710-0cee685387ef
$ ./bar
66033749-3965-43e7-a91a-a5a6a105394a
Attachment #8431143 - Attachment mime type: text/x-csrc → text/plain
Attachment #8431115 - Flags: review?(mh+mozilla)
Attachment #8430523 - Attachment description: v2 (autoconf) → arc4random, v2 (autoconf)
Attached patch arc4random_buf (obsolete) — Splinter Review
Sorry, minor style issues in previous version.
Attachment #8431115 - Attachment is obsolete: true
Attachment #8431115 - Flags: review?(vladimir)
Attachment #8431115 - Flags: review?(mh+mozilla)
Attachment #8431356 - Flags: review?(vladimir)
Attachment #8431356 - Flags: review?(mh+mozilla)
Attachment #8431356 - Flags: review?(mh+mozilla) → review+
Keywords: checkin-needed
Attachment #8430523 - Attachment is obsolete: true
Attachment #8430523 - Attachment is obsolete: false
*sigh* bionic. It has the arc4random_buf function in the libc, which is what the configure check tests, but it's not declared in stdlib.h, contrary to arc4random...
Attached patch arc4random_buf, v2 (android fix) (obsolete) — Splinter Review
Bionic has arc4random_buf() and arc4random_uniform() but they're not declared in <stdlib.h>. Let's try declaring in-place to avoid expanding autoconf check.

$ objdump -T libc.so | fgrep arc4random
0002ab79 g    DF .text  00000078 arc4random
0002ab1d g    DF .text  00000020 arc4random_stir
0002ab3d g    DF .text  0000003c arc4random_addrandom
0002abf1 g    DF .text  0000006c arc4random_buf
0002ac5d g    DF .text  00000034 arc4random_uniform

AC_CACHE_CHECK(for arc4random_buf,
               ac_cv_func_arc4random_buf,
               [AC_TRY_COMPILE([#include <stdlib.h>],
                               [arc4random_buf(0,0);],
                               [ac_cv_func_arc4random_buf=yes],
                               [ac_cv_func_arc4random_buf=no])])
if test "$ac_cv_func_arc4random_buf" = yes ; then
  AC_DEFINE(HAVE_ARC4RANDOM_BUF)
fi

Can someone push the patch to Try build-only (just Android and B2G) ?
Attachment #8431356 - Attachment is obsolete: true
Attachment #8431947 - Flags: review?(vladimir)
(In reply to Jan Beich from comment #15)
> AC_CACHE_CHECK(for arc4random_buf,
[...]

That was an example how expanded autoconf check would look like in absence of AC_CHECK_DECL. Bugzilla ate comment due to leading # symbol.
Comment on attachment 8431947 [details] [diff] [review]
arc4random_buf, v2 (android fix)

Not clear if this is ready to be reviewed -- ping me once the tryserver run has been done and it's good to go?
Attachment #8431947 - Flags: review?(vladimir)
OK. Asking a XPCOM peer explicitly for Try build since I have no luck on #developers and Landry is away.

try: -b o -p android,android-armv6,android-x86,emulator,emulator-jb,emulator-kk -u none -t none
Flags: needinfo?(nfroyd)
(In reply to Jan Beich from comment #18)
> OK. Asking a XPCOM peer explicitly for Try build since I have no luck on
> #developers and Landry is away.
> 
> try: -b o -p
> android,android-armv6,android-x86,emulator,emulator-jb,emulator-kk -u none
> -t none

Happy to help, but this doesn't seem to be applying to any m-c tree that I can identify.  Please rebase?
Flags: needinfo?(nfroyd) → needinfo?(jbeich)
Attachment #8430523 - Attachment is obsolete: true
Attachment #8431947 - Attachment is obsolete: true
|git bz apply| works fine here and after rebase the only thing that changed was patch offsets

-@@ -2946,17 +2946,17 @@ then
+@@ -2941,17 +2941,17 @@ then

Also, git mirror doesn't appear to be out of sync.
Flags: needinfo?(jbeich) → needinfo?(nfroyd)
Thanks, those applied with no problem.  Assuming I didn't botch things:

https://tbpl.mozilla.org/?tree=Try&rev=fa3f97aaabb5
Flags: needinfo?(nfroyd)
Comment on attachment 8432932 [details] [diff] [review]
arc4random_buf, v2 (android fix)

Try is green with |#ifdef ANDROID| added. Backed out version (without ifdef) can be seen on inbound. Requesting review again.

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7b6b2d8dd79b
Attachment #8432932 - Flags: review?(vladimir)
And it doesn't seem arc4random_buf() runtime is broken in bionic.

https://tbpl.mozilla.org/?tree=Try&rev=1a8fcc39f6b5
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c97e6dfaa04e
https://hg.mozilla.org/mozilla-central/rev/d23c297e9498
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.