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

[Jordan Rose]

Thanks, Zach...committed with very minor changes in r204300!

I realized that this doesn't check existing use of %s without a field width. I guess that would have to be a separate warning under -Wformat, because some people might not want to bother fixing their existing code.

Jordan


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

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140319/702705ed/attachment.html>