[PATCH] Add field width to scanf %s format fixit

[Jordan Rose]

Good idea! I have a few comments on the patch itself, but the general implementation seems sound.

+  QualType QT = Ctx.getCanonicalParamType(ArgQT);

This probably doesn't need to be canonical; just "getDecayedType" should be good enough.


+    else {
+      // if we can determine the array length,
+      // we should include it as a field width
+      const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(ArgQT);
+      if (CAT && CAT->getSizeModifier() == ArrayType::Normal)

Three notes:

- This would more idiomatically be spelled "else if (const ConstantArrayType *CAT = ...)". 
- Please use complete sentences and punctuation for comments in the LLVM code.
- Is there any reason the same logic won't work with wide string buffers?


+    bool success = fixedFS.fixType(Ex->IgnoreImpCasts()->getType(), S.getLangOpts(),

Please reflow the arguments to stay within 80 columns.

Thanks for working on this!
Jordan


On Mar 4, 2014, at 21:07 , 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 second of 3 patches working toward this
> (first was r202114).
> 
> This patch updates the fixit system to suggest a field width for %s
> specifiers when the length of the target array is a know fixed size.
> 
> Example:
> 
>    char a[10];
>    scanf("%s", a);
>           ^-
>           %9s
> 
> In order to determine the array length, the fixType function needs to
> know the complete type of the argument, otherwise it is just the raw
> pointer type that we can't reason about.
> <scanf_fixit.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits