[PATCH] Sanitize printf functions

[Evgeniy Stepanov]

  LGTM w/ nit
  But please wait for samsonov's review.


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:697
@@ +696,3 @@
+      va_copy(ar, ap);                                                         \
+      int size = REAL(vname2)(str, 0, format, ar);                             \
+      if (size >= 0) {                                                         \
----------------
Yury Gribov wrote:
> Evgeniy Stepanov wrote:
> > Yury Gribov wrote:
> > > Evgeniy Stepanov wrote:
> > > > Yury Gribov wrote:
> > > > > Evgeniy Stepanov wrote:
> > > > > > Yury Gribov wrote:
> > > > > > > Evgeniy Stepanov wrote:
> > > > > > > > Yury Gribov wrote:
> > > > > > > > > Evgeniy Stepanov wrote:
> > > > > > > > > > Unless I'm missing something, you could use REAL(strlen)(str) here instead of calling *printf twice.
> > > > > > > > > Hm but str isn't necessarily a string - it's just plain array of chars. I don't think we can call strlen on it.
> > > > > > > > I think *printf always end the output with a null character, and never print null in the middle, but I don't see a clear indication of that in the spec.
> > > > > > > Sorry, I must be missing something... My intent with REAL(vname2) (which is actually REAL(vsnprintf)) is to estimate number of chars that will be written. 0 tells vsnprintf that it should not _write_ anything, only estimate number of chars. We could simply pass NULL instead of str and it wouldn't change anything. So why do you bother about str?
> > > > > > I'm just worried that it makes *printf quite a bit heavier - we now parse the format string 3 times (2 in libc *printf and 1 in our code). Alternatively, we could do 1 + 1, and then figure out the length of the resulting string with strlen() after the REAL *printf.
> > > > > > 
> > > > > Agreed but then we may get stack overflow before Asan is able to react. Looks like a trade-off between speed and security. Does libsanitizer have general preference for any of those?
> > > > > 
> > > > > IMHO sprintf is already so slow that we may not need to bother so much.
> > > > Now I must be missing something. What would be the cause of stack overflow?
> > > > 
> > > > That would slow down sprintf (and snprintf, etc) by a factor of 1.5 (roughly speaking). That may be a big hit exactly because sprintf is so slow. I'm assuming that calculation the length of the output by passing 0 as size takes about the same time as actually formatting the output.
> > > > Now I must be missing something. What would be the cause of stack overflow?
> > > 
> > > A call to REAL(sprintf) with unchecked target buffer size.
> > > 
> > > > That would slow down sprintf (and snprintf, etc) by a factor of 1.5 (roughly speaking).
> > > 
> > > Yup, as I said I agree that this adds a penalty. Let's see if we can now come to conclusion on buffer overflow part and see which one is more important.
> > I see. But that's the same call that the application does, i.e. the overflow would have happened without sanitizer, too. And we will most likely catch it, but after sprintf corrupts some memory.
> > 
> > I think its common to use the strlen way. Interceptors like strcat, strcpy call the real function without checking that the string is actually null-terminated, and examine the result afterwards.
> > 
> > 
> > we will most likely catch it, but after sprintf corrupts some memory.
> 
> After some internal fighting I have to agree with that. The claim would be wrong on platforms with ascending stacks but I'm unaware of any living instance (last I heard Aarch64 failed at that).
> 
> > I think its common to use the strlen way.
> > Interceptors like strcat, strcpy call the real function
> > without checking
> 
> I see, so we favor perf indeed. I'll remove the first sprintf call then.
> 
> BTW do we mention somewhere on our wiki that our interceptors are not 100% secure?
> 
I don't think we claim anywhere that ASan is 100% reliable, or that it catches all the bugs ;)

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


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