[PATCH] Add user-defined callback on write() calls to labeled data.

[Sam Kerner]

  Comments addressed.

  It is not obvious to me what the label on the return value of write() would be.  Please explicitly look at what I did (dfsan_custom.c, line 849, in __dfsw_write() ) and let me know if you agree with it.


================
Comment at: lib/dfsan/dfsan_custom.cc:808
@@ +807,3 @@
+SANITIZER_INTERFACE_ATTRIBUTE int
+__dfsw_write(int fd, const void *buf, size_t count) {
+  if (dfsan_labeled_write_callback != NULL) {
----------------
Peter Collingbourne wrote:
> The signature of this function is incorrect. It should accept labels for each of the parameters and a pointer to the return label.
Done

================
Comment at: lib/dfsan/dfsan_custom.cc:810
@@ +809,3 @@
+  if (dfsan_labeled_write_callback != NULL) {
+    // If any label is set in |buf|, invoke the callback.
+    int found_label = 0;
----------------
Peter Collingbourne wrote:
> It would probably be simpler to call the callback unconditionally and let it check buf for labels. This would also allow the client to observe unlabelled writes if it needs to for whatever reason.
Good point.  Done.

================
Comment at: lib/dfsan/dfsan_custom.cc:816
@@ +815,3 @@
+      if (dfsan_read_label(addr, 1) != 0) {
+        found_label = 1;
+      }
----------------
Peter Collingbourne wrote:
> You could exit early here.
Code removed.

================
Comment at: test/dfsan/write_callback.c:1
@@ +1,2 @@
+// RUN: %clang_dfsan                        -m64 %s -o %t && %t %T/file1.txt
+// RUN: %clang_dfsan -mllvm -dfsan-args-abi -m64 %s -o %t && %t %T/file2.txt
----------------
Peter Collingbourne wrote:
> Maybe we could check that these files have the contents we expect?
It would be reasonable to assume that calls to write() do not fail in
tests.  But write() may only write some of the bytes it is passed.
For example:

char buf[] = "123456789";
int bytes_written = write(fd, buf, 10);

If (for whatever reason) only 5 bytes are written, write may return 5.
 I doubt it would do so on a local file system, but I don't want to
assume something that is not in the spec.

I considered adding retries to writes:

char buf[] = "123456789";

char* cur = buf;
int bytes_left = strlen(buf);
while ( bytes_left > 0 ) {
  int result = write(fd, cur, bytes_left);

  if (result < 0) ERROR HANDLER

  bytes_left -= result;
}

... but now the number of callbacks tests must expect changes based on
how many times write() ends up being called.



================
Comment at: lib/dfsan/dfsan.cc:261
@@ +260,3 @@
+
+  dfsan_labeled_write_callback = labeled_write_callback;
+
----------------
Peter Collingbourne wrote:
> I don't think this approach is correct. We can't store a pointer to the callback function (which uses the instrumented ABI) and call it from the dfsan runtime library using the uninstrumented ABI, as the labels received by the callback function will be wrong (nor is this guaranteed to work at all).
> 
> We have a trampoline mechanism for safely calling instrumented ABI functions from the dfsan runtime library. Please take a look at how the custom functions pthread_create and dl_iterate_phdr are implemented.
> 
> When you have done this, you should be able to improve your test to show that the values passed to the write callback have the same labels as those passed to the write function.
Changed the code to use the trampoline mechanism.  Improved tests.


http://reviews.llvm.org/D3268