[PATCH] Sanitize printf functions

Evgeniy Stepanov eugenis at google.com
Wed Jan 15 04:44:42 PST 2014



================
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:
> > 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.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:161
@@ +160,3 @@
+  if (convSpecifier == 'p') {
+    if (lengthModifier[1] != 0)
+      return FSS_INVALID;
----------------
Yury Gribov wrote:
> Evgeniy Stepanov wrote:
> > Should it be lengthModifier[0]?
> > Looks like a bug in the original code.
> > 
> 1) Bug or some weird GNU scanf (mis)feature. Who is the author of this piece?
> 2) How do you guys normally treat situations when both change code and move it to a different place for clarity? Do you make two separate commits to make reviewers life easier?
Me :)
I think it's a bug, it's meant to check that %p has no length modifier.
Two commits are good, but people often don't bother.
In this case it's ok to make the change in the same commit, or whatever is more convenient for you.


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



More information about the llvm-commits mailing list