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

[Jordan Rose]

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>