[dfsan] Add function that prints origin stack trace to buffer

[stephan.yichao.zhao]

Please split the sanitizer common change in a separate CL.

[vitalybuka]

Maybe dfsan part can be in a separate patch, but I will not insist.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

63 ↗

(On Diff #345291)

u32

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_libcdep.cpp

29–55 ↗

(On Diff #345291)

I believe we just need to extract this code into a functions and we save some code below.
I do not see benefits of more complicated entities (classes) here over simple functions.

168 ↗

(On Diff #345291)

We need some tests in sanitizer_stacktrace_test.cpp

compiler-rt/test/dfsan/origin_stack_trace.c

73

second description arg is not tests

Could you please add tests for:

1. description
2. dfsan_sprint_origin_trace(..., NULL, 100)
3. dfsan_sprint_origin_trace(, buff, 0)

[vitalybuka]

In D102451#2758409, @vitalybuka wrote:

Maybe dfsan part can be in a separate patch, but I will not insist.

In D102451#2758397, @stephan.yichao.zhao wrote:

Please split the sanitizer common change in a separate CL.

please split then

[gbalats]

Add more tests for both dfsan interface and StackTrace::PrintTo.

[gbalats]

In D102451#2758397, @stephan.yichao.zhao wrote:

Please split the sanitizer common change in a separate CL.

I will split the sanitizer common parts into a separate patch. However, I find it easier to exercise all tests while making changes for addressing comments, so I will do that in the end.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

63 ↗

(On Diff #345291)

I chose this type in accordance with __sanitizer_symbolize_pc which includes similar parameters. Why is u32 more appropriate here?

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_libcdep.cpp

29–55 ↗

(On Diff #345291)

Out of curiosity, is there a sanitizer developer guide that states such coding preferences more explicitly?

For the given case, I'm not sure how this is possible with simple functions:

• the classes hold a lot of state; all these fields would have to be passed around as arguments making the functions unwieldy
• the classes offer encapsulation (e.g., easier to ensure out_buf_ and 'length_' are updated correctly and out_end_ never changes); passing these around to multiple functions I think would make the code much more brittle
• the common parts that are being abstracted via those classes are the "skeleton" of the algorithm, while only some local operations change (e.g., in one case we call Printf whereas in the other we append to a string buffer); not sure how it's possible to avoid code duplication by using just functions if callbacks (and std::function) isn't an option in the sanitizer world.

For the last part, I ran into this problem when trying to abstract Printf, in particular, which is a variadic function. Without callbacks, a public VPrintf version, and classes how could one write generic code e.g. to abstract the main for-loop (that renders frames) while keeping the actual operation that is performed per frame generic (e.g., a lambda)? Sorry if there's an obvious workaround here but I'm quite new to the sanitizer world.

168 ↗

(On Diff #345291)

Done.

compiler-rt/test/dfsan/origin_stack_trace.c

73

Done.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

63 ↗

(On Diff #345291)

Actually uptr because this component usually use this type for memory buffer sizes

[gbalats]

Change StackTrace::PrintTo buffer size type to uptr.

[gbalats]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

63 ↗

(On Diff #345291)

Done.

[vitalybuka]

In D102451#2761074, @gbalats wrote:

In D102451#2758397, @stephan.yichao.zhao wrote:

Please split the sanitizer common change in a separate CL.

I will split the sanitizer common parts into a separate patch. However, I find it easier to exercise all tests while making changes for addressing comments, so I will do that in the end.

You can do this by keeping both patches in the same git branch and uploading them one by one with "arc diff HEAD^"

[gbalats]

Split dfsan changes and refactor code to reduce duplication.

[stephan.yichao.zhao]

compiler-rt/lib/dfsan/dfsan.cpp

911

This d can be removed.

938

Please add an error message for this case.

940–949

Can you please also extract this part to share with dfsan_print_origin_trace?

[gbalats]

Address comments.

Change behavior on null buffer and zero buffer size.

compiler-rt/lib/dfsan/dfsan.cpp

911

Done.

938

I actually changed this, to be similar to StackTrace::PrintTo. Zero buf size is now legal behavior and returns the correct length (amended test).
As for a null out_buf, changed this to a CHECK-fail since it's always a user error.

940–949

Acknowledged. Variable label is also used below and the return value is different. Also, I don't want to place any print outs inside the PrintOriginTraceToStr helper to minimize its side effects. I thought about having it return an error value or accept an error message string parameter that it can modify, but preferred to keep the code simple and add just some minimum duplication.

compiler-rt/include/sanitizer/dfsan_interface.h

void dfsan_weak_hook_strncmp(void *caller_pc, const char *s1, const char *s2,
                              size_t n, dfsan_label s1_label,
                              dfsan_label s2_label, dfsan_label n_label);
 
 /// Prints the origin trace of the label at the address addr to stderr. It also
 /// prints description at the beginning of the trace. If origin tracking is not
 /// on, or the address is not labeled, it prints nothing.
 void dfsan_print_origin_trace(const void *addr, const char *description);
 
+/// Prints the origin trace of the label at the address \p addr to a
+/// pre-allocated output buffer. If origin tracking is not on, or the address is
+/// not labeled, it prints nothing.
+///
+/// Typical usage:
+/// \code
+///   char kDescription[] = "...";
+///   char buf[1024];
+///   dfsan_sprint_origin_trace(&tainted_var, kDescription, buf, sizeof(buf));
+/// \endcode
+///
+/// Typical usage that handles truncation:
+/// \code
+///   char buf[1024];
+///   int len = dfsan_sprint_origin_trace(&var, nullptr, buf, sizeof(buf));
+///
+///   if (len < sizeof(buf)) {
+///     ProcessOriginTrace(tmpbuf);
+///   else {
+///     char *tmpbuf = new char[len + 1];
+///     dfsan_sprint_origin_trace(&var, nullptr, tmpbuf, len + 1);
+///     ProcessOriginTrace(tmpbuf);
+///     delete[] tmpbuf;
+///   }
+/// \endcode
+///
+/// \param addr The tainted memory address whose origin we are printing.
+/// \param description A description printed at the beginning of the trace.
+/// \param [out] out_buf The output buffer to write the results to.
+/// \param out_buf_size The size of \p out_buf.
+///
+/// \returns The number of symbols that should have been written to \p out_buf
+/// (not including trailing null byte '\0'). Thus, the string is truncated iff
+/// return value is not less than \p out_buf_size.
+size_t dfsan_sprint_origin_trace(const void *addr, const char *description,
+                                 char *out_buf, size_t out_buf_size);
+
 /// Retrieves the very first origin associated with the data at the given
 /// address.
 dfsan_origin dfsan_get_init_origin(const void *addr);
 #ifdef __cplusplus
 }  // extern "C"
 
 template <typename T>
 void dfsan_set_label(dfsan_label label, T &data) { // NOLINT
   dfsan_set_label(label, (void *)&data, sizeof(T));
 }
 
 #endif
 
 #endif  // DFSAN_INTERFACE_H

compiler-rt/lib/dfsan/dfsan.cpp

 }
 
 class Decorator : public __sanitizer::SanitizerCommonDecorator {
  public:
   Decorator() : SanitizerCommonDecorator() {}
   const char *Origin() const { return Magenta(); }
 };
 
-extern "C" SANITIZER_INTERFACE_ATTRIBUTE void dfsan_print_origin_trace(
-    const void *addr, const char *description) {
-  Decorator d;
+namespace {
 
-  if (!__dfsan_get_track_origins()) {
-    Printf(
-        "  %sDFSan: origin tracking is not enabled. Did you specify the "
-        "-dfsan-track-origins=1 option?%s\n",
-        d.Warning(), d.Default());
-    return;
-  }
+void PrintNoOriginTrackingWarning() {
+  Decorator d;
 
-  const dfsan_label label = *__dfsan::shadow_for(addr);
-  if (!label) {
-    Printf("  %sDFSan: no tainted value at %x%s\n", d.Warning(), addr,
-           d.Default());
-    return;
-  }
+void PrintNoTaintWarning(const void *address) {
+  Decorator d;
+  Printf("  %sDFSan: no tainted value at %x%s\n", d.Warning(), address,
 
+void PrintInvalidOriginWarning(dfsan_label label, const void *address) {
+  Decorator d;
+  Printf(
+      "  %sTaint value 0x%x (at %p) has invalid origin tracking. This can "
+      "be a DFSan bug.%s\n",
+      d.Warning(), label, address, d.Default());
+}
+
+bool PrintOriginTraceToStr(const void *addr, const char *description,
+                           InternalScopedString *out) {
+  CHECK(out);
+  CHECK(__dfsan_get_track_origins());
+  Decorator d;
+
+  const dfsan_label label = *__dfsan::shadow_for(addr);
+  CHECK(label);
+
   const dfsan_origin origin = *__dfsan::origin_for(addr);
 
-  Printf("  %sTaint value 0x%x (at %p) origin tracking (%s)%s\n", d.Origin(),
-         label, addr, description ? description : "", d.Default());
+  out->append("  %sTaint value 0x%x (at %p) origin tracking (%s)%s\n",
+              d.Origin(), label, addr, description ? description : "",
+              d.Default());
+
   Origin o = Origin::FromRawId(origin);
   bool found = false;
+
   while (o.isChainedOrigin()) {
     StackTrace stack;
     dfsan_origin origin_id = o.raw_id();
     o = o.getNextChainedOrigin(&stack);
     if (o.isChainedOrigin())
-      Printf("  %sOrigin value: 0x%x, Taint value was stored to memory at%s\n",
-             d.Origin(), origin_id, d.Default());
+      out->append(
+          "  %sOrigin value: 0x%x, Taint value was stored to memory at%s\n",
     else
-      Printf("  %sOrigin value: 0x%x, Taint value was created at%s\n",
+      out->append("  %sOrigin value: 0x%x, Taint value was created at%s\n",
                   d.Origin(), origin_id, d.Default());
-    stack.Print();
+
+    // Includes a trailing newline, so no need to add it again.
+    stack.PrintTo(out);
     found = true;
   }
-  if (!found)
-    Printf(
-        "  %sTaint value 0x%x (at %p) has invalid origin tracking. This can "
-        "be a DFSan bug.%s\n",
-        d.Warning(), label, addr, d.Default());
+
+  return found;
+}
+
+}  // namespace
+
+extern "C" SANITIZER_INTERFACE_ATTRIBUTE void dfsan_print_origin_trace(
+    const void *addr, const char *description) {
+  if (!__dfsan_get_track_origins()) {
+    PrintNoOriginTrackingWarning();
+    return;
+  }
+
+  const dfsan_label label = *__dfsan::shadow_for(addr);
+  if (!label) {
+    PrintNoTaintWarning(addr);
+    return;
+  }
+
+  InternalScopedString trace;
+  bool success = PrintOriginTraceToStr(addr, description, &trace);
+
+  if (trace.length())
+    Printf("%s", trace.data());
+
+  if (!success)
+    PrintInvalidOriginWarning(label, addr);
+}
+
+extern "C" SANITIZER_INTERFACE_ATTRIBUTE size_t
+dfsan_sprint_origin_trace(const void *addr, const char *description,
+                          char *out_buf, size_t out_buf_size) {
+  CHECK(out_buf);
+
+  if (!__dfsan_get_track_origins()) {
+    PrintNoOriginTrackingWarning();
+    return 0;
+  }
+
+  const dfsan_label label = *__dfsan::shadow_for(addr);
+  if (!label) {
+    PrintNoTaintWarning(addr);
+    return 0;
+  }
+
+  InternalScopedString trace;
+  bool success = PrintOriginTraceToStr(addr, description, &trace);
+
+  if (!success) {
+    PrintInvalidOriginWarning(label, addr);
+    return 0;
+  }
+
+  if (out_buf_size) {
+    internal_strncpy(out_buf, trace.data(), out_buf_size - 1);
+    out_buf[out_buf_size - 1] = '\0';
+  }
+
+  return trace.length();
 }
 
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE dfsan_origin
 dfsan_get_init_origin(const void *addr) {
   if (!__dfsan_get_track_origins())
     return 0;
 
   const dfsan_label label = *__dfsan::shadow_for(addr);

compiler-rt/lib/dfsan/done_abilist.txt

 fun:dfsan_has_label_with_desc=uninstrumented
 fun:dfsan_has_label_with_desc=discard
 fun:dfsan_set_write_callback=uninstrumented
 fun:dfsan_set_write_callback=custom
 fun:dfsan_flush=uninstrumented
 fun:dfsan_flush=discard
 fun:dfsan_print_origin_trace=uninstrumented
 fun:dfsan_print_origin_trace=discard
+fun:dfsan_sprint_origin_trace=uninstrumented
+fun:dfsan_sprint_origin_trace=discard
 fun:dfsan_get_origin=uninstrumented
 fun:dfsan_get_origin=custom
 fun:dfsan_get_init_origin=uninstrumented
 fun:dfsan_get_init_origin=discard
 
 
 ###############################################################################
 # glibc

compiler-rt/test/dfsan/origin_stack_trace.c

 // RUN: %clang_dfsan -gmlt -mllvm -dfsan-track-origins=1 -mllvm -dfsan-fast-16-labels=true %s -o %t && \
 // RUN:     %run %t >%t.out 2>&1
-// RUN: FileCheck %s --check-prefix=CHECK < %t.out
+// RUN: FileCheck %s < %t.out
 //
 // RUN: %clang_dfsan -gmlt -mllvm -dfsan-track-origins=1 -mllvm -dfsan-fast-16-labels=true -mllvm -dfsan-instrument-with-call-threshold=0 %s -o %t && \
 // RUN:     %run %t >%t.out 2>&1
-// RUN: FileCheck %s --check-prefix=CHECK < %t.out
+// RUN: FileCheck %s < %t.out
 //
 // REQUIRES: x86_64-target-arch
 
 #include <sanitizer/dfsan_interface.h>
+#include <stdio.h>
+#include <string.h>
 
 #define NOINLINE __attribute__((noinline))
 
 NOINLINE int foo(int a, int b) { return a + b; }
 
 NOINLINE void bar(int depth, void *addr, int size) {
   if (depth) {
     bar(depth - 1, addr, size);
   } else {
     dfsan_set_label(1, addr, size);
   }
 }
 
 NOINLINE void baz(int depth, void *addr, int size) {
   bar(depth, addr, size);
 }
 
 int main(int argc, char *argv[]) {
   int a = 10;
   int b = 20;
   baz(8, &a, sizeof(a));
   int c = foo(a, b);
   dfsan_print_origin_trace(&c, NULL);
-}
+  // CHECK: Taint value 0x1 {{.*}} origin tracking ()
+  // CHECK: Origin value: {{.*}}, Taint value was stored to memory at
+  // CHECK: #0 {{.*}} in main {{.*}}origin_stack_trace.c:[[@LINE-4]]
+
+  // CHECK: Origin value: {{.*}}, Taint value was created at
+  // CHECK: #0 {{.*}} in dfs$bar {{.*}}origin_stack_trace.c:[[@LINE-19]]
+  // CHECK-COUNT-8: #{{[0-9]+}} {{.*}} in dfs$bar {{.*}}origin_stack_trace.c:[[@LINE-22]]
+  // CHECK: #9 {{.*}} in dfs$baz {{.*}}origin_stack_trace.c:[[@LINE-16]]
 
+  char buf[3000];
+  size_t length = dfsan_sprint_origin_trace(&c, NULL, buf, sizeof(buf));
+
+  printf("==OUTPUT==\n\n%s==EOS==\n", buf);
+  // CHECK: ==OUTPUT==
   // CHECK: Taint value 0x1 {{.*}} origin tracking ()
   // CHECK: Origin value: {{.*}}, Taint value was stored to memory at
-// CHECK: #0 {{.*}} in main {{.*}}origin_stack_trace.c:[[@LINE-6]]
+  // CHECK: #0 {{.*}} in main {{.*}}origin_stack_trace.c:[[@LINE-18]]
 
   // CHECK: Origin value: {{.*}}, Taint value was created at
-// CHECK: #0 {{.*}} in dfs$bar {{.*}}origin_stack_trace.c:[[@LINE-21]]
-// CHECK-COUNT-8: #{{[0-9]+}} {{.*}} in dfs$bar {{.*}}origin_stack_trace.c:[[@LINE-24]]
-// CHECK: #9 {{.*}} in dfs$baz {{.*}}origin_stack_trace.c:[[@LINE-18]]
+  // CHECK: #0 {{.*}} in dfs$bar {{.*}}origin_stack_trace.c:[[@LINE-33]]
+  // CHECK-COUNT-8: #{{[0-9]+}} {{.*}} in dfs$bar {{.*}}origin_stack_trace.c:[[@LINE-36]]
+  // CHECK: #9 {{.*}} in dfs$baz {{.*}}origin_stack_trace.c:[[@LINE-30]]
+  // CHECK: ==EOS==
+
+  char tinybuf[18];
+  size_t same_length = dfsan_sprint_origin_trace(&c, NULL, tinybuf, sizeof(tinybuf));
+
+  printf("==TRUNCATED OUTPUT==\n\n%s==EOS==\n", tinybuf);
+  // CHECK: ==TRUNCATED OUTPUT==
+  // CHECK: Taint value 0x1==EOS==
+
+  printf("Returned length: %zu\n", length);
+  printf("Actual length: %zu\n", strlen(buf));
+  printf("Returned length with truncation: %zu\n", same_length);
+
+  // CHECK: Returned length: [[#LEN:]]
+  // CHECK: Actual length: [[#LEN]]
+  // CHECK: Returned length with truncation: [[#LEN]]
+
+  size_t length_with_desc = dfsan_sprint_origin_trace(&c, "DESCRIPTION", buf, sizeof(buf));
+
+  printf("==OUTPUT==\n\n%s==EOS==\n", buf);
+  // CHECK: ==OUTPUT==
+  // CHECK: Taint value 0x1 {{.*}} origin tracking (DESCRIPTION)
+  // CHECK: Origin value: {{.*}}, Taint value was stored to memory at
+  // CHECK: #0 {{.*}} in main {{.*}}origin_stack_trace.c:[[@LINE-47]]
+
+  // CHECK: Origin value: {{.*}}, Taint value was created at
+  // CHECK: #0 {{.*}} in dfs$bar {{.*}}origin_stack_trace.c:[[@LINE-62]]
+  // CHECK-COUNT-8: #{{[0-9]+}} {{.*}} in dfs$bar {{.*}}origin_stack_trace.c:[[@LINE-65]]
+  // CHECK: #9 {{.*}} in dfs$baz {{.*}}origin_stack_trace.c:[[@LINE-59]]
+  // CHECK: ==EOS==
+
+  printf("Returned length: %zu\n", length_with_desc);
+  // COMM: Message length is increased by 11: the length of "DESCRIPTION".
+  // CHECK: Returned length: [[#LEN + 11]]
+
+  buf[0] = '\0';
+  length = dfsan_sprint_origin_trace(&c, NULL, buf, 0);
+  printf("Output=\"%s\"\n", buf);
+  printf("Returned length: %zu\n", length);
+  // CHECK: Output=""
+  // CHECK: Returned length: [[#LEN]]
+}