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

[Zach Davis]

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>
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: scanf_fixit_4.patch
Type: text/x-patch
Size: 3438 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140318/306706a0/attachment.bin>