[PATCH] Sanitize printf functions

[Alexey Samsonov]

================
Comment at: lib/asan/lit_tests/TestCases/printf-2.c:20
@@ +19,3 @@
+  // CHECK-ON: heap-use-after-free
+  // CHECK-ON-NOT: 0 12 1.239 34
+  // CHECK-OFF: 0 12 1.239 
----------------
Could you match a file/line in the stack trace in error report? E.g. see TestCases/null_deref.cc

================
Comment at: lib/asan/lit_tests/TestCases/printf-1.c:4
@@ +3,3 @@
+// RUN: ASAN_OPTIONS=check_printf=0 %t 2>&1 | FileCheck %s
+// RUN: ASAN_OPTIONS="" %t 2>&1 | FileCheck %s
+
----------------
Just use "%t 2>&1" to test it with default ASAN_OPTIONS.

================
Comment at: lib/asan/lit_tests/TestCases/printf-1.c:6
@@ +5,3 @@
+
+#include <stdio.h>
+int main() {
----------------
I'd suggest to merge all these test cases into one, as we're really testing one thing here. That would save some lines of code and would make modifying and extending the test case easier, and the tests will run faster (you will compile/link a single binary instead of 5 binaries). You can control the printf(...) call you're testing with an argv. See TestCases/stack-buffer-overflow-with-position.cc

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:573
@@ -573,1 +572,3 @@
+#if SANITIZER_INTERCEPT_SCANF || SANITIZER_INTERCEPT_PRINTF
+# include "sanitizer_common_interceptors_scanf.inc"
 
----------------
Just "#include", we don't seem to use "#  " indentation in another places in this file.

================
Comment at: lib/sanitizer_common/tests/sanitizer_scanf_interceptor_test.cc:61
@@ +60,3 @@
+static const char test_buf[] = "Test string.";
+static size_t test_buf_size = sizeof(test_buf);
+
----------------
This one is also const.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:61
@@ +60,3 @@
+  // width.
+
+  return p;
----------------
remove empty line

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:25
@@ -33,1 +24,3 @@
 
+static const char *maybe_parse_signed(const char *p, int *out) {
+  if (*p == '-')
----------------
Quick check: is it OK you will consume the first "-" in "--" string?

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:30
@@ +29,3 @@
+    p = parse_number(p, out);
+    if (*out <= 0)
+      return 0;
----------------
Please clarify in function name or comment why "0" or "-0" are unexpected.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:233
@@ -77,6 +232,3 @@
     // Field width.
-    if (*p >= '0' && *p <= '9') {
-      p = parse_number(p, &dir->fieldWidth);
-      if (dir->fieldWidth <= 0)
-        return 0;
-    }
+    p = maybe_parse_signed(p, &dir->fieldWidth);
+    if (!p)
----------------
Is it ok the code originally parsed only unsigned integers as the field width, but now it will treat "-9" as a field width "9"?

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:348
@@ -300,1 +347,3 @@
+    if (size == FSS_INVALID) {
+      Report("WARNING: unexpected format specifier in scanf interceptor\n");
       break;
----------------
I don't like this diagnostics: we don't even print what this unknown specifier is. We should either do this, or silently exit.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:508
@@ +507,3 @@
+    if (size == FSS_INVALID) {
+      Report("WARNING: unexpected format specifier in printf interceptor\n");
+      break;
----------------
same here.


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