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

[Jordan Rose]

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.

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!)

+#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.

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.

Jordan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140326/7a3974e0/attachment.html>