[PATCH] Sanitize printf functions

[Alexey Samsonov]

================
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 == '-')
----------------
Yury Gribov wrote:
> 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.
Alright, I agree that %-- is just invalid scanf/printf syntax and we may not necessarily parse it correctly, but now we move this to function "maybe_parse_signed", and its name doesn't tell much about this hidden assumptions ("-" consumed from "--", "0" results in NULL returned), so one could use it in a wrong way somewhere else. IMO It's better to specify this assumptions explicitly here.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:30
@@ +29,3 @@
+    p = parse_number(p, out);
+    if (*out <= 0)
+      return 0;
----------------
Evgeniy Stepanov wrote:
> Yury Gribov wrote:
> > 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.
> Could it be different for printf vs scanf?
See above

================
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 
----------------
Yury Gribov wrote:
> 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.
We generally don't want many internal frames from ASan runtime to appear in the error report. So it's nice to verify that error report looks somewhat like this:
#0 __interceptor_printf ....
#1 main printf-2.c:16

================
Comment at: lib/asan/lit_tests/TestCases/printf-1.c:6
@@ +5,3 @@
+
+#include <stdio.h>
+int main() {
----------------
Yury Gribov wrote:
> 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.
Well, ok, leaving this to you. If you think that merging the cases would complicate things too much, leave it as is.


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