[PATCH] D58184: [scudo][standalone] Introduce platform specific code & mutexes

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr added inline comments.


================
Comment at: lib/scudo/standalone/common.h:149
+// platform specific data to the function.
+// Returns MAP_ERROR on error or dies if MAP_ALLOWNOMEM is not specified.
+void *map(void *Addr, uptr Size, const char *Name, uptr Flags = 0,
----------------
cryptoad wrote:
> mcgrathr wrote:
> > Is it really useful to have MAP_ERROR instead of just using nullptr here?
> > In the traditional mmap interface, MAP_FAILED is not 0 because it's sometimes possible to actually mmap address 0.
> > But that's not relevant here, so why not just stick the standard C++?
> Personal preference here. I thought that sanitizer_common was bad at mixing null & (void *)-1 for errors, and it did try to map 0 in some places.
> So I went with all MAP_FAILED type error for map functions.
> Though I am not opposed to all nullptr.
I'd prefer we completely avoid casting bogons to pointer types if we can.


================
Comment at: lib/scudo/standalone/fuchsia.cc:66
+  zx_info_vmar_t Info;
+  const zx_status_t Status = _zx_object_get_info(
+      Vmar, ZX_INFO_VMAR, &Info, sizeof(Info), nullptr, nullptr);
----------------
cryptoad wrote:
> mcgrathr wrote:
> > You're only using VMARs you created yourself, so why use a syscall to get the bounds rather than keeping track locally?
> It was mostly for storage reasons.
> The vmar+vmo fits in a u64 which works well. Keeping the base & size would require 2 extra uptr, which to be fair is not the end of the world.
> The extra 16 bytes would be for every secondary backed allocation & for every region of the primary.
> I am open to doing either.
> 
Give it some comments about the trade-offs of storage cost vs syscall cost and we can optimize later.


================
Comment at: lib/scudo/standalone/fuchsia.cc:176
+
+const char *getEnv(const char *Name) {
+  if (StoredEnviron) {
----------------
cryptoad wrote:
> mcgrathr wrote:
> > Why doesn't this just call libc's getenv?
> I went the way it was done in sanitizer_common.
> Part of the reason being that on some platform (Android), getenv would not work when called either too early or from the init process for example.
> If this is not case for Fuchsia, happy to switch.
There should be no problem on Fuchsia and if something crops up we can figure it out.


================
Comment at: lib/scudo/standalone/fuchsia.cc:202
+bool getRandom(void *Buffer, uptr Length, bool Blocking) {
+  if (!Buffer || !Length || Length > 256U)
+    return false;
----------------
cryptoad wrote:
> mcgrathr wrote:
> > Where does the magic 256 constant come from?
> > If it's part of the getRandom API then it should be a macro or constexpr in the OS-independent headers.
> 256 is a magic Linux related constant.
> getrandom doesn't get interrupted for sizes < 256.
> And when reading /dev/urandom, reads of up to 256 bytes will return as many bytes as are requested and will not be interrupted by a signal handler.
> Since we don't use large random streams of bytes, I enforced 256.
> I will put that in the common header.
OK, then I think a static_assert(NewConstantForMaxSize <= ZX_CPRNG_DRAW_MAX_LEN) is what we want here.


================
Comment at: lib/scudo/standalone/internal_defs.h:126
+
+// Various integer constants.
+
----------------
cryptoad wrote:
> mcgrathr wrote:
> > Why not use <cstdint>?
> Right, it's mostly a sanitizer_common artifact, I haven't revisited it.
> cstdint might not work with -nostdinc++ but stdint.h might.
<stdint.h> is already used here, so just use that.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D58184