[PATCH] Sanitize printf functions

[Alexander Potapenko]

================
Comment at: sanitizer_common/sanitizer_common_interceptors_scanf.inc:10
@@ -9,3 +9,3 @@
 //
 // Scanf implementation for use in *Sanitizer interceptors.
 // Follows http://pubs.opengroup.org/onlinepubs/9699919799/functions/fscanf.html
----------------
Yury Gribov wrote:
> Alexander Potapenko wrote:
> > Please update this comment.
> > Also, how about renaming the file to something like "sanitizer_common_interceptors_printf_scanf.inc" or "sanitizer_common_interceptors_format.inc"?
> > Please update this comment.
> 
> Will do.
> 
> > Also, how about renaming the file to something like "sanitizer_common_interceptors_printf_scanf.inc"
> 
> But won't this make diff unreadable? Perhaps this should be a separate patch?
> But won't this make diff unreadable? Perhaps this should be a separate patch?
SGTM

================
Comment at: sanitizer_common/sanitizer_common_interceptors_scanf.inc:281
@@ -280,1 +280,3 @@
 
+  // TODO(y.gribov): check format?
+
----------------
Yury Gribov wrote:
> Alexander Potapenko wrote:
> > Yury Gribov wrote:
> > > Kostya Serebryany wrote:
> > > > good idea. feel free to implement or to leave the TODO.
> > > > Actually this code base uses FIXME (w/o name) more often. 
> > > > Actually this code base uses FIXME (w/o name) more often.
> > > 
> > > Got it. Anyway, this was more a question for reviewers than real TODO. I'll add format check in next rev.
> > We sure need to check |format|. Since you've already touched scanf_common in this CL, why not just add
> >   COMMON_INTERCEPTOR_READ_RANGE(ctx, format, internal_strlen(format) + 1);
> > 
> > ?
> Yup, already added this in new version of the patch. I didn't resubmit it because I wanted to gather reviews from all team members.
> 
> The bigger question is whether we want to check size of input buffer in sscanf and friends.
> 
> BTW what's the general rule: should I resubmit patch as often as possible or wait till everyone comments?
> BTW what's the general rule: should I resubmit patch as often as possible or wait till everyone comments?

I think it's better to resubmit once you've addressed a round of comments.
Most of the team is on holidays now, so you'll never know whether everyone is going to comment.

================
Comment at: sanitizer_common/sanitizer_common_interceptors_scanf.inc:281
@@ -280,1 +280,3 @@
 
+  // TODO(y.gribov): check format?
+
----------------
Alexander Potapenko wrote:
> Yury Gribov wrote:
> > Alexander Potapenko wrote:
> > > Yury Gribov wrote:
> > > > Kostya Serebryany wrote:
> > > > > good idea. feel free to implement or to leave the TODO.
> > > > > Actually this code base uses FIXME (w/o name) more often. 
> > > > > Actually this code base uses FIXME (w/o name) more often.
> > > > 
> > > > Got it. Anyway, this was more a question for reviewers than real TODO. I'll add format check in next rev.
> > > We sure need to check |format|. Since you've already touched scanf_common in this CL, why not just add
> > >   COMMON_INTERCEPTOR_READ_RANGE(ctx, format, internal_strlen(format) + 1);
> > > 
> > > ?
> > Yup, already added this in new version of the patch. I didn't resubmit it because I wanted to gather reviews from all team members.
> > 
> > The bigger question is whether we want to check size of input buffer in sscanf and friends.
> > 
> > BTW what's the general rule: should I resubmit patch as often as possible or wait till everyone comments?
> > BTW what's the general rule: should I resubmit patch as often as possible or wait till everyone comments?
> 
> I think it's better to resubmit once you've addressed a round of comments.
> Most of the team is on holidays now, so you'll never know whether everyone is going to comment.
> The bigger question is whether we want to check size of input buffer in sscanf and friends.

Yes, that's nice to have, too.

================
Comment at: sanitizer_common/sanitizer_common_interceptors_scanf.inc:362
@@ +361,3 @@
+      default:
+        break; // TODO(ygribov): issue warning?
+      }
----------------
Yury Gribov wrote:
> Alexander Potapenko wrote:
> > Two spaces before the comment, please.
> Do you think we need a warning here? If yes, I'll add it, if not - I'll remove the comment alltogether.
We can't get an invalid size from format_get_value_size(), so it's ok to have a CHECK here.

================
Comment at: sanitizer_common/sanitizer_platform_interceptors.h:76
@@ -75,3 +75,3 @@
 # define SANITIZER_INTERCEPT_SCANF SI_NOT_WINDOWS
 # define SANITIZER_INTERCEPT_ISOC99_SCANF SI_LINUX
 
----------------
Yury Gribov wrote:
> Alexander Potapenko wrote:
> > I wonder why the lines above have this strange indentation, but let us fix that in a separate commit.
> Looks like something lint should be able to handle.
I've removed these. There's no clear policy about these spaces anyway.


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