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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>>> >>>> <scanf_fixit2.patch> >>>> >>>> >>>> >>> <scanf_fixit_4.patch> >> > <scanf_fixit5.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
