[PATCH] Sanitize printf functions

[Yury Gribov]

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:360
@@ +359,3 @@
+
+#define skip_scalar_arg(aq, convSpec, size)           \
+  do {                                                \
----------------
Evgeniy Stepanov wrote:
> Does it really need to be a macro?
I was under impression that pointers to va_list are not portable C but http://gcc.gnu.org/ml/gcc-help/1999-q3n/msg00283.html suggests that they're sane. I'll give it a try tomorrow.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:90
@@ -78,2 +89,3 @@
+      ++p;
     if (*p >= '0' && *p <= '9') {
       p = parse_number(p, &dir->fieldWidth);
----------------
Evgeniy Stepanov wrote:
> According to the manual, field width can be "*", too. The number-or-star parsing logic could be factored out in a separate function.
> 
> According to the manual, field width can be "*", too

Yup, this is handled by dir->suppressed above)

> The number-or-star parsing logic could be factored out in a separate function.

Agreed.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:98
@@ +97,3 @@
+      if (output)
+        dir->printErrno = true;
+      else
----------------
Evgeniy Stepanov wrote:
> This does not look right. %m in printf is a complete conversion specifier, it should end parsing of the current directive. And it seems like "m" could go into convSpecifier, and printErrno could be removed.
> 
> %m in printf is a complete conversion specifier, it should end parsing of the current directive

I agree that we are currently parsing a wider format language than really necessary. But same is true for %n in original scanf implementation and we didn't bother handling it.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:168
@@ -129,3 +167,3 @@
         ++p;
-      } else if (*p == '[') {
+      } else if (!output && *p == '[') {
         // Watch for %a[h-j%d], if % appears in the
----------------
Evgeniy Stepanov wrote:
> At this point allowGnuMalloc is true. It does not make sense for output to be true. Please remove this and instead check this invariant at the beginning of the functions (!(allowGnuMalloc && output)).
> 
Agreed.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:293
@@ +292,3 @@
+      return FSS_INVALID;
+    if (output || dir->fieldWidth == 0)
+      return FSS_STRLEN;
----------------
Evgeniy Stepanov wrote:
> Are these guys ([...]) even allowed in printf?
No, they aren't. But why do you ask the question here?


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