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

[Jordan Rose]

You're asking for the canonical type, which is looking through typedefs. I don't think that's what we want to do. What sort of segfaults did you get when using the decayed type?

Jordan


On Mar 5, 2014, at 13:19 , Zach Davis <zdavkeos at gmail.com> wrote:

> 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
>> 
> <scanf_fixit2.patch>