[PATCH] D56146: [Sanitizer] enable arc4random api on Darwin

[Conrad Meyer via Phabricator via llvm-commits]

cem requested changes to this revision.
cem added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/sanitizer_common/sanitizer_mac.cc:1120
     return false;
-  // arc4random never fails.
-  arc4random_buf(buffer, length);
+  uptr rnd = getentropy(buffer, length);
+  int rverrno = 0;
----------------
I don't think this change makes much sense.

  # arc4random_buf is a perfectly good userspace random API and has a long history; getentropy is only present on the latest FreeBSD.  This breaks backwards compatibility with FreeBSD 11.x, for example.
  # arc4random_buf maintains a userspace Chacha keystream seeded periodically by kernel getrandom(2) syscalls; getentropy(3) invokes a syscall every time it is used.  This change makes GetRandom() significantly more expensive per call, without benefit.
  # The previous path was failure-free; this version has to implement additional error handling, and does so questionably.  See below.


================
Comment at: lib/sanitizer_common/sanitizer_mac.cc:1122-1123
+  int rverrno = 0;
+  if (internal_iserror(rnd, &rverrno) && rverrno == EINVAL)
+     return false;
   return true;
----------------
 # Why would getentropy produce an `EINVAL` code?  The only documented errors are EFAULT (depends on `buffer`) and EIO (`length` checks above should prevent).  Linux documents an additional ENOSYS code if getrandom is absent, but not EINVAL, AFAIK.

  - https://man.openbsd.org/getentropy.2
  - http://man7.org/linux/man-pages/man3/getentropy.3.html
  - https://www.freebsd.org/cgi/man.cgi?query=getentropy&sektion=3&manpath=freebsd-release-ports

 (I am unfamiliar with `internal_iserror`; please forgive me if it is expected to squash other errors to `EINVAL` for some reason. :-))

 # Why should errors other than `EINVAL` be treated as success conditions?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56146