[PATCH] 18412 - Add warning when scanf %s is used without a field width

[Zach Davis]

> > On Mar 21, 2014, at 14:14 , Zach Davis <zdavkeos at gmail.com> wrote:
> > Background: Bug 18412 suggests that the compiler should issue a
> > security warning when a scanf %s format specifier does not
> > include a field width.  This is the third patche working toward
> > this (r202114, 204300).
> >
> > This patch adds the actual warning. The warning is part of the
> > FormatSecurity warning group.
> >
> > Example:
> >
> >    test.c:14:10: warning: no field width in scanf string format
> > specifier (potentially insecure)
> >      scanf("%s", str);
> >             ^~
> >
> > Presently one of the tests in test/Sema/format-strings-scanf.c
> > fails due to the way the tests are executed (the file is
> > re-compiled with the -Wformat=0 option). I would appreciate any
> > advice on fixing this test case.
>
> On Wed, Mar 26, 2014 at 8:30 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> I think the right way to fix this is to put the field width into
> the fix-it when it's known. (Are you already doing that? If so,
> it needs tests!)

We do already do this, as in the test case above, however in this
case the fixit is incomplete becuase we don't know the array
size.  So the first pass does its best and corrects the "%lf"
to "%s", then the second pass warns about the "%s" lacking a
field width. This is why I suppressed the warning.

> +#pragma clang diagnostic push // Don't warn about security problems.
> +#pragma clang diagnostic ignored "-Wformat-security"
>    scanf("%lf", vstr);
> +#pragma clang diagnostic pop

> This seems wrong to me. We shouldn't emit the second warning if
> we're already fixing the format string.

> Stepping back, I'm worried about adding this warning to all of
> -Wformat-security: in some code bases there may be perfectly
> innocuous uses of sscanf. Yes, you could add a sensible buffer
> width in pretty much all cases, but for existing codebases we
> probably don't want to deal with it.

I agree, thats why I made the warning DefaultIgnore, but it might
not be enough.

> How about emitting a warning directly in -Wformat-security if the
> buffer size is known, and in some new
> -Wformat-security-unknown-width (which is under
> -Wformat-security) if it isn't? That way people can turn off the
> warnings they can't immediately fix, but still see the obvious
> ones.

I suppose we could add a new sub-group of FormatSecurity to make
the warning more fine-grained.

The fix may also just be to move the warning generation out of
SemaChecking.cpp and into the fixit code (ScanfFormatString.cpp).
This would give us more control over when to warn/fix/silent.
I'll explore this and see if it makes more sense.

> Jordan

Thanks for the help.

Zach