[PATCH] Sanitize printf functions

Yury Gribov tetra2005 at gmail.com
Wed Jan 15 05:25:07 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) {                                                         \
----------------
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?

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:161
@@ +160,3 @@
+  if (convSpecifier == 'p') {
+    if (lengthModifier[1] != 0)
+      return FSS_INVALID;
----------------
Evgeniy Stepanov wrote:
> 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.
I'll update the code and see if tests seize to work then.


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



More information about the llvm-commits mailing list