[PATCH] Sanitize printf functions

Yury Gribov tetra2005 at gmail.com
Mon Dec 30 03:26:53 PST 2013



================
Comment at: sanitizer_common/sanitizer_common_interceptors_scanf.inc:281
@@ -280,1 +280,3 @@
 
+  // TODO(y.gribov): check format?
+
----------------
Alexander Potapenko wrote:
> 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.
Any idea how to achieve this? Scanfs won't tell us number of bytes read and I can't make use of %n in interceptor...

================
Comment at: sanitizer_common/sanitizer_common_interceptors_scanf.inc:362
@@ +361,3 @@
+      default:
+        break; // TODO(ygribov): issue warning?
+      }
----------------
Alexander Potapenko wrote:
> 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.
Will do.


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



More information about the llvm-commits mailing list