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

[Zach Davis]

Jordan-

getDecayedType() faults when the QualType passed to it is a "size_t
*identifier" type.  The only other place getDecayedType is called it
goes like:

  if (T->isArrayType() || T->isFunctionType())
    return getDecayedType(T)

What we need is some way to re-decay the QualType to a valid
"function-parm-type" (which I assume happens somewhere?)

I agree now that getCanonicalParmType is overkill. Maybe we just need
to do some checks so we can do the right decay based on the QualType?

Zach

On Wed, Mar 5, 2014 at 4:09 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> 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>
>