[llvm-commits] [PATCH] [msan] MemorySanitizer runtime

[Alexander Potapenko]

================
Comment at: llvm/projects/compiler-rt/include/sanitizer/msan_interface.h:44
@@ +43,3 @@
+
+SANITIZER_INTERFACE_ATTRIBUTE
+void __msan_unpoison(void *a, uptr size);
----------------
It's good to have comments for all the interface functions (a FIXME is fine for now).
Also, are __msan_memset and __msan_memcpy interface functions?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:70
@@ +69,3 @@
+
+static bool IsRunningUnderDr() {
+  bool result = false;
----------------
Why isn't this in the __msan namespace?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:103
@@ +102,3 @@
+
+
+static void ParseFlagsFromString(Flags *f, const char *str) {
----------------
Nit: spare newline?

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:193
@@ +192,3 @@
+}
+
+
----------------
Nit: spare newline.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:227
@@ +226,3 @@
+      Printf("Unlimited stack, doing reexec\n");
+    SetStackSizeLimitInBytes(32 * 1024 * 1024);
+    ReExec();
----------------
Please add a comment justifying this size.

================
Comment at: llvm/projects/compiler-rt/include/sanitizer/msan_interface.h:22
@@ +21,3 @@
+using __sanitizer::u32;
+
+#ifdef __cplusplus
----------------
How about adding a compile-time check for sizeof(uptr) (or __LP64__) to ensure we're building for 64 bits?
Not sure if it should be in this particular file (probably yes, since it's included everywhere).

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_allocator.cc:30
@@ +29,3 @@
+typedef SizeClassAllocator64<kAllocatorSpace, kAllocatorSize, kMetadataSize,
+  DefaultSizeClassMap> PrimaryAllocator;
+typedef SizeClassAllocatorLocalCache<PrimaryAllocator> AllocatorCache;
----------------
4-space indentation.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_allocator.cc:34
@@ +33,3 @@
+typedef CombinedAllocator<PrimaryAllocator, AllocatorCache,
+          SecondaryAllocator> Allocator;
+
----------------
Please fix the indentation.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:363
@@ +362,3 @@
+  wchar_t *res = (wchar_t *)fast_memset(s, c, n * sizeof(wchar_t));
+  if (MEM_TO_SHADOW((uptr)s) != (uptr)s)
+    __msan_unpoison(s, n * sizeof(wchar_t));
----------------
Please add an assertion instead.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:394
@@ +393,3 @@
+
+// TODO: intercept the following functions:
+// Note, they only matter when running without a dynamic tool.
----------------
s/TODO/FIXME

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:423
@@ +422,3 @@
+  char *res = REAL(fcvt)(x, a, b, c);
+  if (!__msan_has_dynamic_component()) {
+    __msan_unpoison(b, sizeof(*b));
----------------
It's inevident why you unpoison memory only in the absence of a dynamic component here and below.
Please add a comment.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:491
@@ +490,3 @@
+    return REAL(pipe)(pipefd);
+
+  ENSURE_MSAN_INITED();
----------------
Nit: spare newline.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:613
@@ +612,3 @@
+  if (res > 0) {
+    __msan_unpoison(events, __msan::struct_epoll_event_sz * res);
+  }
----------------
Please remove the space after "*"

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:659
@@ +658,3 @@
+  if (!msan_inited) {
+    // Hack: dlsym calls calloc before REAL(calloc) is retrieved from dlsym.
+    const size_t kCallocPoolSize = 1024;
----------------
You can use LowLevelAlloc instead (I don't insist since we have the same in ASan now)

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:794
@@ +793,3 @@
+  void *res = fast_memset(s, c, n);
+  if (MEM_TO_SHADOW((uptr)s) != (uptr)s)
+    __msan_unpoison(s, n);
----------------
Please add an assertion or (if it's possible that MEM_TO_SHADOW((uptr)s) == (uptr)s) a comment.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:810
@@ +809,3 @@
+  CHECK_EQ(inited, 0);
+  inited = 1;
+  CHECK(INTERCEPT_FUNCTION(mmap));
----------------
Not sure if it affects anything (what if you get a signal while in InitializeInterceptors?), but maybe set inited=1 at the end of the function?
Leaving this up to you.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan_linux.cc:28
@@ +27,3 @@
+
+
+#include "sanitizer_common/sanitizer_common.h"
----------------
Nit: spare newline.

================
Comment at: llvm/projects/compiler-rt/lib/msan/msan.cc:188
@@ +187,3 @@
+  }
+  if (__msan::flags()->exit_code >= 0) {
+    Printf("Exiting\n");
----------------
Are you going to have negative exit codes? I don't think it's a good idea.
If no, it's better to sanitize the exit code when you parse the flags and call MsanDie here.


http://llvm-reviews.chandlerc.com/D191