This is an archive of the discontinued LLVM Phabricator instance.

libc++: add NaCl and PNaCl support for std::random_device
ClosedPublic

Authored by jfb on Nov 27 2014, 1:20 PM.

Details

Summary

The NaCl sandbox doesn't allow opening files under /dev, but it offers an API which provides the same capabilities. This is the same random device emulation that nacl_io performs for POSIX support, but nacl_io is an optional library so libc++ can't assume that device emulation will be performed. Note that NaCl only supports /dev/urandom, not /dev/random.

This patch also cleans up some of the preprocessor #endif, and fixes the test for Win32 (it accepts any token, and would therefore never throw regardless of the token provided).

Diff Detail

Event Timeline

jfb updated this revision to Diff 16707.Nov 27 2014, 1:20 PM
jfb retitled this revision from to libc++: add NaCl and PNaCl support for std::random_device.
jfb updated this object.
jfb edited the test plan for this revision. (Show Details)
jfb added reviewers: dschuff, mclow.lists, danalbert.
jfb added a subscriber: Unknown Object (MLST).
EricWF added a subscriber: EricWF.Nov 27 2014, 1:30 PM

The purpose of the token is to permit different sources of randomness. Seeing as how you don't allow anything other than nacl_secure_random_init, perhaps you should just make the default token be the empty string in the <random> header.

Checking for an invalid token would just be a simple !__token.empty().

src/random.cpp
79

This read a little funny to me, maybe:

random_device failed to obtain enough bytes

jfb added a comment.EditedNov 27 2014, 3:34 PM

The purpose of the token is to permit different sources of randomness. Seeing as how you don't allow anything other than nacl_secure_random_init, perhaps you should just make the default token be the empty string in the <random> header.

That would be a breaking change if we do decide to support other sources of
randomness. What we have now emulates the token it accepts (its POSIX
meaning), so it's the right thing IMO.

mclow.lists edited edge metadata.Nov 28 2014, 11:55 AM

I'd really prefer a different identifier other than __native_client__.
IMHO, that's too "generic".

Something that mentioned NaCl and/or PNaCl.
_LIBCPP_USING_NACL_RANDOM, maybe?

jfb updated this revision to Diff 16748.EditedNov 29 2014, 11:06 AM
jfb edited edge metadata.

Rephrase error message.

jfb updated this revision to Diff 16749.EditedNov 29 2014, 11:59 AM

Add _LIBCPP_USING_NACL_RANDOM to __config, and use that instead of __native_client__.

jfb added a comment.Nov 29 2014, 12:00 PM

I'd really prefer a different identifier other than __native_client__.
IMHO, that's too "generic".

Something that mentioned NaCl and/or PNaCl.
_LIBCPP_USING_NACL_RANDOM, maybe?

Done, let me know if what I implemented was what you were suggesting.

src/random.cpp
79

Changed.

jfb updated this revision to Diff 16750.Nov 29 2014, 12:05 PM

Fix missing parens.

Your changes look good; but now I'm wondering if you want to accept both "/dev/random" and "/dev/urandom" as valid tokens.

Now I'm wondering what bozo checked in the windows implementation w/o noticing that it broke this test on Windows. Oh - that was me.

Should the windows implementation also accept only "/dev/random" and "/dev/urandom" as valid tokens?

That would enable us to make the test portable; and help people to write cross-platform code.

include/__config
118

I don't think you need to actually set _LIBCPP_USING_NACL_RANDOM to a value.

#define _LIBCPP_USING_NACL_RANDOM

is sufficient.

test/numerics/rand/rand.device/ctor.pass.cpp
34

I don't really like this; it's leaking implementation information into the test framework.

jfb updated this revision to Diff 16768.Dec 1 2014, 9:11 AM

Define macro without setting a value.

jfb added a comment.Dec 1 2014, 9:18 AM

Your changes look good; but now I'm wondering if you want to accept both "/dev/random" and "/dev/urandom" as valid tokens.

I don't want /dev/random because the NaCl interface I'm using doesn't guarantee "very high quality randomness" and doesn't block when such randomness isn't available. User code that expects such randomness should hard-fail (and fall back as makes sense for them), instead of getting lower quality randomness. NaCl has, for example, SSH clients that rely on similar (non-C++) random device interfaces, and knowing that the source of randomness is bad is pretty important to them (not that OpenSSL and friends have a great track record).

Should the windows implementation also accept only "/dev/random" and "/dev/urandom" as valid tokens?

I wouldn't offer /dev/random unless the implementation actually meets the expectations of users.

That would enable us to make the test portable; and help people to write cross-platform code.

I think the lack of platform portability should go back to WG21 LEWG ;-)

Agreed that the test is now ugly... but hey it's correct!

include/__config
118

Done.

jfb updated this revision to Diff 16773.Dec 1 2014, 9:53 AM

Return ENOENT instead of ENODEV as suggested by @majnemer.

mclow.lists accepted this revision.Dec 1 2014, 10:27 AM
mclow.lists edited edge metadata.
This revision is now accepted and ready to land.Dec 1 2014, 10:27 AM
jfb closed this revision.Dec 1 2014, 11:20 AM
jfb updated this revision to Diff 16776.

Closed by commit rL223068 (authored by @jfb).