<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Thanks, Zach...committed with very minor changes in r204300!</div><div><br></div><div>I realized that this doesn't check <i>existing</i> 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.</div><div><br></div><div>Jordan</div><div><br></div><br><div><div>On Mar 19, 2014, at 12:33 , Zach Davis <<a href="mailto:zdavkeos@gmail.com">zdavkeos@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Jordan-<br><br>I've made your suggested code changes and have added a number of tests.<br>As you mentioned, not all of the test work right but adding them in<br>should give us a starting place for the future.<br><br>Zach<br><br>On Tue, Mar 18, 2014 at 10:13 PM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:<br><blockquote type="cite">This looks good! Can you add more tests involving the new fix-it? Some ideas:<br><br>- fixed-length array with the wrong character type<br>- fixed-length array with the right character type, but the size is too big (with a FIXME)<br>- variable-length array<br>- pointer parameter typed as an array<br>- 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.)<br><br>A few more comments on the code:<br><br> // If it's an enum, get its underlying type.<br> if (const EnumType *ETy = QT->getAs<EnumType>())<br> QT = ETy->getDecl()->getIntegerType();<br><br>This isn't your change, but I think that's supposed to be PT, not QT!<br><br>+ if(const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(RawQT)) {<br><br>Please add a space before the open paren, now that it fits.<br><br>Thanks again for working on this! It does seem much easier with both types available.<br>Jordan<br><br><br>On Mar 18, 2014, at 17:03 , Zach Davis <<a href="mailto:zdavkeos@gmail.com">zdavkeos@gmail.com</a>> wrote:<br><br><blockquote type="cite">I think passing both types is a good way to go, it would keep it more<br>generic than just passing the whole Expr.<br><br>Attached is the modified patch.<br><br>On Mon, Mar 10, 2014 at 11:48 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:<br><blockquote type="cite">I spent several minutes looking around for the appropriate function to call<br>here. getAdjustedParameterType() seems to do what we want, but it's<br>definitely meant for use at the declaration site, not the call site. I'm<br>thinking it's probably best to just pass both types, and only check the<br>original type for things like this. What do you think?<br><br>Jordan<br><br><br>On Mar 7, 2014, at 8:58 , Zach Davis <<a href="mailto:zdavkeos@gmail.com">zdavkeos@gmail.com</a>> wrote:<br><br>Jordan-<br><br>getDecayedType() faults when the QualType passed to it is a "size_t<br>*identifier" type. The only other place getDecayedType is called it<br>goes like:<br><br>if (T->isArrayType() || T->isFunctionType())<br> return getDecayedType(T)<br><br>What we need is some way to re-decay the QualType to a valid<br>"function-parm-type" (which I assume happens somewhere?)<br><br>I agree now that getCanonicalParmType is overkill. Maybe we just need<br>to do some checks so we can do the right decay based on the QualType?<br><br>Zach<br><br>On Wed, Mar 5, 2014 at 4:09 PM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:<br><br>You're asking for the canonical type, which is looking through typedefs. I<br>don't think that's what we want to do. What sort of segfaults did you get<br>when using the decayed type?<br><br>Jordan<br><br><br>On Mar 5, 2014, at 13:19 , Zach Davis <<a href="mailto:zdavkeos@gmail.com">zdavkeos@gmail.com</a>> wrote:<br><br>Thanks for the comments.<br><br>I have made the style changes and re-arranged the checks so wide<br>char types are also included.<br><br>However, when I changed<br><br>+ QualType QT = Ctx.getCanonicalParamType(ArgQT)<br><br>to getDecayedType, I got a number of seg faults. So I have left<br>it for now as GetCanonicalParamType. Along this same line, the<br>patch does cause an existing fixit test to fail, namely:<br><br>size_t size_t_var;<br>scanf("%f", size_t_var);<br> ^-<br> %ul<br><br>The test expects "%zu", not "%ul". This seems to be related to<br>the type of decay we do, but I don't know where to go from there.<br><br>Zach<br><br>On Wed, Mar 5, 2014 at 11:50 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:<br><br>Good idea! I have a few comments on the patch itself, but the general<br>implementation seems sound.<br><br>+ QualType QT = Ctx.getCanonicalParamType(ArgQT);<br><br>This probably doesn't need to be canonical; just "getDecayedType" should be<br>good enough.<br><br><br>+ else {<br>+ // if we can determine the array length,<br>+ // we should include it as a field width<br>+ const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(ArgQT);<br>+ if (CAT && CAT->getSizeModifier() == ArrayType::Normal)<br><br>Three notes:<br><br>- This would more idiomatically be spelled "else if (const ConstantArrayType<br>*CAT = ...)".<br>- Please use complete sentences and punctuation for comments in the LLVM<br>code.<br>- Is there any reason the same logic won't work with wide string buffers?<br><br><br>+ bool success = fixedFS.fixType(Ex->IgnoreImpCasts()->getType(),<br>S.getLangOpts(),<br><br>Please reflow the arguments to stay within 80 columns.<br><br>Thanks for working on this!<br>Jordan<br><br><br>On Mar 4, 2014, at 21:07 , Zach Davis <<a href="mailto:zdavkeos@gmail.com">zdavkeos@gmail.com</a>> wrote:<br><br>Background: Bug 18412 suggests that the compiler should issue a<br>security warning when a scanf %s format specifier does not include a<br>field width. This is the second of 3 patches working toward this<br>(first was r202114).<br><br>This patch updates the fixit system to suggest a field width for %s<br>specifiers when the length of the target array is a know fixed size.<br><br>Example:<br><br>char a[10];<br>scanf("%s", a);<br> ^-<br> %9s<br><br>In order to determine the array length, the fixType function needs to<br>know the complete type of the argument, otherwise it is just the raw<br>pointer type that we can't reason about.<br><scanf_fixit.patch>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits<br><br><br><scanf_fixit2.patch><br><br><br><br></blockquote><scanf_fixit_4.patch><br></blockquote><br></blockquote><span><scanf_fixit5.patch></span></blockquote></div><br></body></html>