[PATCH] Sanitize printf functions
Evgeniy Stepanov
eugenis at google.com
Fri Jan 10 01:13:14 PST 2014
================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:90
@@ -78,2 +89,3 @@
+ ++p;
if (*p >= '0' && *p <= '9') {
p = parse_number(p, &dir->fieldWidth);
----------------
Yury Gribov wrote:
> Evgeniy Stepanov wrote:
> > According to the manual, field width can be "*", too. The number-or-star parsing logic could be factored out in a separate function.
> >
> > According to the manual, field width can be "*", too
>
> Yup, this is handled by dir->suppressed above)
>
> > The number-or-star parsing logic could be factored out in a separate function.
>
> Agreed.
Oh, right. This is a bit confusing, could you add a separate field to FormatDirective instead of reusing "suppressed" here?
================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:98
@@ +97,3 @@
+ if (output)
+ dir->printErrno = true;
+ else
----------------
Yury Gribov wrote:
> Yury Gribov wrote:
> > Evgeniy Stepanov wrote:
> > > This does not look right. %m in printf is a complete conversion specifier, it should end parsing of the current directive. And it seems like "m" could go into convSpecifier, and printErrno could be removed.
> > >
> > > %m in printf is a complete conversion specifier, it should end parsing of the current directive
> >
> > I agree that we are currently parsing a wider format language than really necessary. But same is true for %n in original scanf implementation and we didn't bother handling it.
> > And it seems like "m" could go into convSpecifier, and printErrno could be removed.
>
> Agreed to this one, will fix.
What's wrong with %n? We parse it as convSpecifier which can not have anything going after it.
It seems like simply changing the code here to
if (*p == 'm' && !output) {
dir->allocate=true;
++p;
}
would do the trick.
================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc:293
@@ +292,3 @@
+ return FSS_INVALID;
+ if (output || dir->fieldWidth == 0)
+ return FSS_STRLEN;
----------------
Yury Gribov wrote:
> Evgeniy Stepanov wrote:
> > Are these guys ([...]) even allowed in printf?
> No, they aren't. But why do you ask the question here?
Then it would make sense to handle this case earlier, in format_parse_next. Probably not a big deal.
http://llvm-reviews.chandlerc.com/D2480
More information about the llvm-commits
mailing list