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

[Zach Davis]

Thanks for the comments.

I have made the style changes and re-arranged the checks so wide
char types are also included.

However, when I changed

+  QualType QT = Ctx.getCanonicalParamType(ArgQT)

to getDecayedType, I got a number of seg faults. So I have left
it for now as GetCanonicalParamType.  Along this same line, the
patch does cause an existing fixit test to fail, namely:

  size_t size_t_var;
  scanf("%f", size_t_var);
        ^-
        %ul

The test expects "%zu", not "%ul". This seems to be related to
the type of decay we do, but I don't know where to go from there.

Zach

On Wed, Mar 5, 2014 at 11:50 AM, Jordan Rose <jordan_rose at apple.com> wrote:
> 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: scanf_fixit2.patch
Type: application/octet-stream
Size: 2646 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140305/f1e8b167/attachment.obj>