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>
>
>
>
Index: include/clang/Analysis/Analyses/FormatString.h
===================================================================
--- include/clang/Analysis/Analyses/FormatString.h (revision 204199)
+++ include/clang/Analysis/Analyses/FormatString.h (working copy)
@@ -572,7 +572,8 @@
ArgType getArgType(ASTContext &Ctx) const;
- bool fixType(QualType QT, const LangOptions &LangOpt, ASTContext &Ctx);
+ bool fixType(QualType QT, QualType RawQT, const LangOptions &LangOpt,
+ ASTContext &Ctx);
void toString(raw_ostream &os) const;
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp (revision 204199)
+++ lib/Sema/SemaChecking.cpp (working copy)
@@ -3518,8 +3518,9 @@
const analyze_format_string::ArgType &AT = FS.getArgType(S.Context);
if (AT.isValid() && !AT.matchesType(S.Context, Ex->getType())) {
ScanfSpecifier fixedFS = FS;
- bool success = fixedFS.fixType(Ex->getType(), S.getLangOpts(),
- S.Context);
+ bool success = fixedFS.fixType(Ex->getType(),
+ Ex->IgnoreImpCasts()->getType(),
+ S.getLangOpts(), S.Context);
if (success) {
// Get the fix string from the fixed format specifier.
Index: lib/Analysis/ScanfFormatString.cpp
===================================================================
--- lib/Analysis/ScanfFormatString.cpp (revision 204199)
+++ lib/Analysis/ScanfFormatString.cpp (working copy)
@@ -379,21 +379,22 @@
return ArgType();
}
-bool ScanfSpecifier::fixType(QualType QT, const LangOptions &LangOpt,
+bool ScanfSpecifier::fixType(QualType QT, QualType RawQT,
+ const LangOptions &LangOpt,
ASTContext &Ctx) {
- if (!QT->isPointerType())
- return false;
// %n is different from other conversion specifiers; don't try to fix it.
if (CS.getKind() == ConversionSpecifier::nArg)
return false;
- QualType PT = QT->getPointeeType();
+ if (!QT->isPointerType())
+ return false;
// If it's an enum, get its underlying type.
if (const EnumType *ETy = QT->getAs<EnumType>())
QT = ETy->getDecl()->getIntegerType();
-
+
+ QualType PT = QT->getPointeeType();
const BuiltinType *BT = PT->getAs<BuiltinType>();
if (!BT)
return false;
@@ -405,6 +406,15 @@
LM.setKind(LengthModifier::AsWideChar);
else
LM.setKind(LengthModifier::None);
+
+ // If we know the target array length, we can use it as a field width.
+ if(const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(RawQT)) {
+ if (CAT->getSizeModifier() == ArrayType::Normal)
+ FieldWidth = OptionalAmount(OptionalAmount::Constant,
+ CAT->getSize().getZExtValue() - 1,
+ "", 0, false);
+
+ }
return true;
}
Index: test/Sema/format-strings-fixit.c
===================================================================
--- test/Sema/format-strings-fixit.c (revision 204199)
+++ test/Sema/format-strings-fixit.c (working copy)
@@ -206,7 +206,7 @@
// CHECK: printf("%La", (long double) 42);
// CHECK: printf("%LA", (long double) 42);
-// CHECK: scanf("%s", str);
+// CHECK: scanf("%99s", str);
// CHECK: scanf("%hd", &shortVar);
// CHECK: scanf("%hu", &uShortVar);
// CHECK: scanf("%d", &intVar);
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
