[PATCH] [sanitizer] Support sandboxing in sanitizer coverage.

[Sergey Matveev]

================
Comment at: include/sanitizer/common_interface_defs.h:27
@@ -26,1 +26,3 @@
 #endif
+  struct __sanitizer_sandbox_arguments {
+    int coverage_sandboxed;
----------------
Kostya Serebryany wrote:
> Alexander Potapenko wrote:
> > I think we need some "version" parameter to distinguish between different versions of this structure.
> use typedef so that you don't need to repeat struct every time
done

================
Comment at: include/sanitizer/common_interface_defs.h:30
@@ +29,3 @@
+    int coverage_fd;
+    size_t coverage_max_block_size;
+  };
----------------
Sergey Matveev wrote:
> Kostya Serebryany wrote:
> > Alexander Potapenko wrote:
> > > Should this really be passed through the sandbox arguments? I don't think the clients need to care much.
> > comments?
> We must not send a longer message than the other side is prepared to receive, otherwise the message gets truncated.
done

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:46
@@ -45,3 +45,3 @@
 // pc_array is the array containing the covered PCs.
 // To make the pc_array thread- and AS- safe it has to be large enough.
 // 128M counters "ought to be enough for anybody" (4M on 32-bit).
----------------
Alexander Potapenko wrote:
> While at it, can you please replace "AS- safe" with "async-signal-safe"?
done

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:55
@@ +54,3 @@
+static bool cov_sandboxed = false;
+static int cov_fd = -1;
+static uptr cov_max_block_size = 0;
----------------
Alexander Potapenko wrote:
> Please use kInvalidFd.
done

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:78
@@ -73,1 +77,3 @@
 
+struct CovHeader {
+  int pid;
----------------
Kostya Serebryany wrote:
> comments?
done

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:80
@@ +79,3 @@
+  int pid;
+  unsigned int module_length;
+  uptr data_size;
----------------
Sergey Matveev wrote:
> Alexander Potapenko wrote:
> > This should be u32, like the offsets.
> > I'd even suggest to define something like module_off_t in this file.
> This is string length, not module data length. I agree the name is confusing, but it follows pre-existing naming in this file. I should probably add "_name" everywhere.
done

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:105
@@ +104,3 @@
+    uptr max_payload_size = cov_max_block_size - header_size_with_module;
+    char *block_bytes = block.data();
+    internal_memcpy(block_bytes, &header, sizeof(header));
----------------
Alexander Potapenko wrote:
> s/block_bytes/block_pos ?
changed naming

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:116
@@ +115,3 @@
+      blob_bytes += payload_size;
+      // Note: this assumes proper alignment of block.data().
+      ((CovHeader *)block.data())->data_size = payload_size;
----------------
Alexander Potapenko wrote:
> Please define "proper"
replaced with an explicit alignment check

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:183
@@ +182,3 @@
+static void OpenPackedFileForWriting() {
+  CHECK(cov_fd < 0);  // NOLINT
+  InternalScopedBuffer<char> path(1024);
----------------
Alexander Potapenko wrote:
> CHECK(cov_fd != kInvalidFd), remove NOLINT.
done

http://reviews.llvm.org/D3726