[libcxx-commits] [PATCH] D116045: [libc++] Use arc4random instead of /dev/urandom on Apple platforms

Tue Dec 21 12:37:13 PST 2021

ldionne marked an inline comment as done.
ldionne added a comment.

In D116045#3205247 <https://reviews.llvm.org/D116045#3205247>, @Mordante wrote:

> I assume that's something along the lines of
>
> - #ifdef _LIBCPP_USING_DEV_RANDOM
>
> + #if defined(_LIBCPP_USING_DEV_RANDOM) || defined(__APPLE__)
>
> With the appropriate comment and closing `#endif`.

Yes, roughly.

> I wonder whether it wouldn't be better to ignore the token for `_LIBCPP_USING_ARC4_RANDOM` instead of throwing an exception. (Probably in a separate patch.) That way even when Apple users now select a device their code will keep working after this change instead of suddenly throwing an exception.

I am really ambivalent on this. My understanding is that these `__throw`s are meant to basically say "you should not ever use a non-default-constructed `random_device`". Failing at least ensures that users don't think they are using some other source of randomness -- they will definitely notice the change. Like I said, I would be surprised if anyone ever noticed the change, however if they did, I'm unsure whether it would be better to error out explicitly (so they can notice and fix their stuff), or to swallow the error and just disregard the token. I am currently leaning slightly towards the former, but I can easily be convinced otherwise. For now I'll update with my proposed patch if we do go for the former.

================
Comment at: libcxx/include/__config:338
    // Certain architectures provide arc4random(). Prefer using
    // arc4random() over /dev/{u,}random to make it possible to obtain
    // random data even when using sandboxing mechanisms such as chroots,
----------------
Mordante wrote:
> This comment is somewhat misleading, since it doesn't work when the token is `/dev/random`.
> But as commented below I think it would be good to document here that the token is ignored when using arc4random.
> 
> Documenting the behavior of token for the other devices would be nice.
Under `_LIBCPP_USING_DEV_RANDOM`, using `/dev/random` as the parameter will work. I fixed the documentation issue in a separate NFC commit to keep those separate: https://github.com/llvm/llvm-project/commit/346ef5e5879e9143fe008c35c3dd6ea3c402634a

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116045/new/

https://reviews.llvm.org/D116045