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

[Zach Davis]

Jordan-

I've made your suggested code changes and have added a number of tests.
As you mentioned, not all of the test work right but adding them in
should give us a starting place for the future.

Zach

On Tue, Mar 18, 2014 at 10:13 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> This looks good! Can you add more tests involving the new fix-it? Some ideas:
>
> - fixed-length array with the wrong character type
> - fixed-length array with the right character type, but the size is too big (with a FIXME)
> - variable-length array
> - pointer parameter typed as an array
> - pointer parameter typed as an array with a static bound ("int buffer[static 10]"). Yes, that's a thing. (We don't have to support it specially here, but it shouldn't crash.)
>
> A few more comments on the code:
>
>    // If it's an enum, get its underlying type.
>    if (const EnumType *ETy = QT->getAs<EnumType>())
>      QT = ETy->getDecl()->getIntegerType();
>
> This isn't your change, but I think that's supposed to be PT, not QT!
>
> +    if(const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(RawQT)) {
>
> Please add a space before the open paren, now that it fits.
>
> Thanks again for working on this! It does seem much easier with both types available.
> Jordan
>
>
> On Mar 18, 2014, at 17:03 , Zach Davis <zdavkeos at gmail.com> wrote:
>
>> I think passing both types is a good way to go, it would keep it more
>> generic than just passing the whole Expr.
>>
>> Attached is the modified patch.
>>
>> On Mon, Mar 10, 2014 at 11:48 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>>> I spent several minutes looking around for the appropriate function to call
>>> here. getAdjustedParameterType() seems to do what we want, but it's
>>> definitely meant for use at the declaration site, not the call site. I'm
>>> thinking it's probably best to just pass both types, and only check the
>>> original type for things like this. What do you think?
>>>
>>> Jordan
>>>
>>>
>>> On Mar 7, 2014, at 8:58 , Zach Davis <zdavkeos at gmail.com> wrote:
>>>
>>> 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>
>>>
>>>
>>>
>> <scanf_fixit_4.patch>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: scanf_fixit5.patch
Type: application/octet-stream
Size: 5461 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140319/b3f79909/attachment.obj>