[PATCH] Sanitize printf functions

[Yury Gribov]

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:691
@@ +690,3 @@
+
+#define VSPRINTF_INTERCEPTOR_IMPL(vname, vname2, str, ...)                     \
+  {                                                                            \
----------------
Evgeniy Stepanov wrote:
> vname2 can be removed
+1

================
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
+
----------------
Alexey Samsonov wrote:
> Just use "%t 2>&1" to test it with default ASAN_OPTIONS.
Ok.

================
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 
----------------
Alexey Samsonov wrote:
> Could you match a file/line in the stack trace in error report? E.g. see TestCases/null_deref.cc
Well, ok although I'd consider this an overkill.

================
Comment at: lib/asan/lit_tests/TestCases/printf-1.c:6
@@ +5,3 @@
+
+#include <stdio.h>
+int main() {
----------------
Alexey Samsonov wrote:
> 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
I don't quite agree. Different tests trigger different types of memory errors so CHECKs will be different. Also printed messages in some tests differ.

================
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"
 
----------------
Alexey Samsonov wrote:
> Just "#include", we don't seem to use "#  " indentation in another places in this file.
+1

================
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 == '-')
----------------
Alexey Samsonov wrote:
> Quick check: is it OK you will consume the first "-" in "--" string?
%-- is invalid syntax anyway so I think it's ok.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:30
@@ +29,3 @@
+    p = parse_number(p, out);
+    if (*out <= 0)
+      return 0;
----------------
Alexey Samsonov wrote:
> Please clarify in function name or comment why "0" or "-0" are unexpected.
This is Evegeny's code so I have no idea.

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

================
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)
----------------
Alexey Samsonov wrote:
> 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"?
Well, the spec does not mention that width in scanf must be positive but GNU scanf rejects negatives. I'll replace with parse_number.

================
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;
----------------
Alexey Samsonov wrote:
> I don't like this diagnostics: we don't even print what this unknown specifier is. We should either do this, or silently exit.
This warning is mainly to inform us that we failed to parse some format spec and I think it's rather important for debugging purposes. What if I replace it with VReport(1, ...) ?

================
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);
+
----------------
Alexey Samsonov wrote:
> This one is also const.
+1


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