[cfe-commits] r137980 - in /cfe/trunk: include/clang/Basic/Builtins.def include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaChecking.cpp test/Sema/warn-strlcpycat-size.c
Nico Weber
thakis at chromium.org
Fri Aug 19 21:47:12 PDT 2011
Hi Ted,
this looks like a useful warning – are you planning on adding this to -Wall?
(I tried this warning on the chromium & webkit code bases, and it
found 0 false positives and 0 bugs.)
Nico
On Thu, Aug 18, 2011 at 1:55 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Author: kremenek
> Date: Thu Aug 18 15:55:45 2011
> New Revision: 137980
>
> URL: http://llvm.org/viewvc/llvm-project?rev=137980&view=rev
> Log:
> Reapply r137903, but fix the definition of size_t in the test case to use __SIZE_TYPE__ (and hence be portable).
>
> Also, change the warning to -Wstrl-incorrect-size.
>
> Added:
> cfe/trunk/test/Sema/warn-strlcpycat-size.c
> Modified:
> cfe/trunk/include/clang/Basic/Builtins.def
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> cfe/trunk/include/clang/Sema/Sema.h
> cfe/trunk/lib/Sema/SemaChecking.cpp
>
> Modified: cfe/trunk/include/clang/Basic/Builtins.def
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Builtins.def?rev=137980&r1=137979&r2=137980&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/Builtins.def (original)
> +++ cfe/trunk/include/clang/Basic/Builtins.def Thu Aug 18 15:55:45 2011
> @@ -664,6 +664,9 @@
> // POSIX setjmp.h
> LIBBUILTIN(_longjmp, "vJi", "fr", "setjmp.h", ALL_LANGUAGES)
> LIBBUILTIN(siglongjmp, "vSJi", "fr", "setjmp.h", ALL_LANGUAGES)
> +// non-standard but very common
> +LIBBUILTIN(strlcpy, "zc*cC*z", "f", "string.h", ALL_LANGUAGES)
> +LIBBUILTIN(strlcat, "zc*cC*z", "f", "string.h", ALL_LANGUAGES)
> // id objc_msgSend(id, SEL, ...)
> LIBBUILTIN(objc_msgSend, "GGH.", "f", "objc/message.h", OBJC_LANG)
>
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=137980&r1=137979&r2=137980&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Aug 18 15:55:45 2011
> @@ -278,6 +278,13 @@
> "argument to 'sizeof' in %0 call is the same pointer type %1 as the "
> "%select{destination|source}2; expected %3 or an explicit length">,
> InGroup<DiagGroup<"sizeof-pointer-memaccess">>;
> +def warn_strlcpycat_wrong_size : Warning<
> + "size argument in %0 call appears to be size of the source; expected the size of "
> + "the destination">,
> + DefaultIgnore,
> + InGroup<DiagGroup<"strl-incorrect-size">>;
> +def note_strlcpycat_wrong_size : Note<
> + "change size argument to be the size of the destination">;
>
> /// main()
> // static/inline main() are not errors in C, just in C++.
>
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=137980&r1=137979&r2=137980&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Aug 18 15:55:45 2011
> @@ -5973,6 +5973,9 @@
> void CheckMemaccessArguments(const CallExpr *Call, CheckedMemoryFunction CMF,
> IdentifierInfo *FnName);
>
> + void CheckStrlcpycatArguments(const CallExpr *Call,
> + IdentifierInfo *FnName);
> +
> void CheckReturnStackAddr(Expr *RetValExp, QualType lhsType,
> SourceLocation ReturnLoc);
> void CheckFloatComparison(SourceLocation loc, Expr* lex, Expr* rex);
>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=137980&r1=137979&r2=137980&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Aug 18 15:55:45 2011
> @@ -319,7 +319,7 @@
> TheCall->getCallee()->getLocStart());
> }
>
> - // Memset/memcpy/memmove/memcmp handling
> + // Builtin handling
> int CMF = -1;
> switch (FDecl->getBuiltinID()) {
> case Builtin::BI__builtin_memset:
> @@ -339,6 +339,11 @@
> case Builtin::BImemmove:
> CMF = CMF_Memmove;
> break;
> +
> + case Builtin::BIstrlcpy:
> + case Builtin::BIstrlcat:
> + CheckStrlcpycatArguments(TheCall, FnInfo);
> + break;
>
> case Builtin::BI__builtin_memcmp:
> CMF = CMF_Memcmp;
> @@ -359,6 +364,7 @@
> break;
> }
>
> + // Memset/memcpy/memmove handling
> if (CMF != -1)
> CheckMemaccessArguments(TheCall, CheckedMemoryFunction(CMF), FnInfo);
>
> @@ -1990,6 +1996,95 @@
> }
> }
>
> +// A little helper routine: ignore addition and subtraction of integer literals.
> +// This intentionally does not ignore all integer constant expressions because
> +// we don't want to remove sizeof().
> +static const Expr *ignoreLiteralAdditions(const Expr *Ex, ASTContext &Ctx) {
> + Ex = Ex->IgnoreParenCasts();
> +
> + for (;;) {
> + const BinaryOperator * BO = dyn_cast<BinaryOperator>(Ex);
> + if (!BO || !BO->isAdditiveOp())
> + break;
> +
> + const Expr *RHS = BO->getRHS()->IgnoreParenCasts();
> + const Expr *LHS = BO->getLHS()->IgnoreParenCasts();
> +
> + if (isa<IntegerLiteral>(RHS))
> + Ex = LHS;
> + else if (isa<IntegerLiteral>(LHS))
> + Ex = RHS;
> + else
> + break;
> + }
> +
> + return Ex;
> +}
> +
> +// Warn if the user has made the 'size' argument to strlcpy or strlcat
> +// be the size of the source, instead of the destination.
> +void Sema::CheckStrlcpycatArguments(const CallExpr *Call,
> + IdentifierInfo *FnName) {
> +
> + // Don't crash if the user has the wrong number of arguments
> + if (Call->getNumArgs() != 3)
> + return;
> +
> + const Expr *SrcArg = ignoreLiteralAdditions(Call->getArg(1), Context);
> + const Expr *SizeArg = ignoreLiteralAdditions(Call->getArg(2), Context);
> + const Expr *CompareWithSrc = NULL;
> +
> + // Look for 'strlcpy(dst, x, sizeof(x))'
> + if (const Expr *Ex = getSizeOfExprArg(SizeArg))
> + CompareWithSrc = Ex;
> + else {
> + // Look for 'strlcpy(dst, x, strlen(x))'
> + if (const CallExpr *SizeCall = dyn_cast<CallExpr>(SizeArg)) {
> + if (SizeCall->isBuiltinCall(Context) == Builtin::BIstrlen
> + && SizeCall->getNumArgs() == 1)
> + CompareWithSrc = ignoreLiteralAdditions(SizeCall->getArg(0), Context);
> + }
> + }
> +
> + if (!CompareWithSrc)
> + return;
> +
> + // Determine if the argument to sizeof/strlen is equal to the source
> + // argument. In principle there's all kinds of things you could do
> + // here, for instance creating an == expression and evaluating it with
> + // EvaluateAsBooleanCondition, but this uses a more direct technique:
> + const DeclRefExpr *SrcArgDRE = dyn_cast<DeclRefExpr>(SrcArg);
> + if (!SrcArgDRE)
> + return;
> +
> + const DeclRefExpr *CompareWithSrcDRE = dyn_cast<DeclRefExpr>(CompareWithSrc);
> + if (!CompareWithSrcDRE ||
> + SrcArgDRE->getDecl() != CompareWithSrcDRE->getDecl())
> + return;
> +
> + const Expr *OriginalSizeArg = Call->getArg(2);
> + Diag(CompareWithSrcDRE->getLocStart(), diag::warn_strlcpycat_wrong_size)
> + << OriginalSizeArg->getSourceRange() << FnName;
> +
> + // Output a FIXIT hint if the destination is an array (rather than a
> + // pointer to an array). This could be enhanced to handle some
> + // pointers if we know the actual size, like if DstArg is 'array+2'
> + // we could say 'sizeof(array)-2'.
> + const Expr *DstArg = Call->getArg(0)->IgnoreParenImpCasts();
> +
> + if (DstArg->getType()->isArrayType()) {
> + llvm::SmallString<128> sizeString;
> + llvm::raw_svector_ostream OS(sizeString);
> + OS << "sizeof(";
> + DstArg->printPretty(OS, Context, 0, Context.PrintingPolicy);
> + OS << ")";
> +
> + Diag(OriginalSizeArg->getLocStart(), diag::note_strlcpycat_wrong_size)
> + << FixItHint::CreateReplacement(OriginalSizeArg->getSourceRange(),
> + OS.str());
> + }
> +}
> +
> //===--- CHECK: Return Address of Stack Variable --------------------------===//
>
> static Expr *EvalVal(Expr *E, SmallVectorImpl<DeclRefExpr *> &refVars);
>
> Added: cfe/trunk/test/Sema/warn-strlcpycat-size.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-strlcpycat-size.c?rev=137980&view=auto
> ==============================================================================
> --- cfe/trunk/test/Sema/warn-strlcpycat-size.c (added)
> +++ cfe/trunk/test/Sema/warn-strlcpycat-size.c Thu Aug 18 15:55:45 2011
> @@ -0,0 +1,28 @@
> +// RUN: %clang_cc1 -Wstrl-incorrect-size -verify -fsyntax-only %s
> +
> +typedef __SIZE_TYPE__ size_t;
> +size_t strlcpy (char * restrict dst, const char * restrict src, size_t size);
> +size_t strlcat (char * restrict dst, const char * restrict src, size_t size);
> +size_t strlen (const char *s);
> +
> +char s1[100];
> +char s2[200];
> +char * s3;
> +
> +struct {
> + char f1[100];
> + char f2[100][3];
> +} s4, **s5;
> +
> +int x;
> +
> +void f(void)
> +{
> + strlcpy(s1, s2, sizeof(s1)); // no warning
> + strlcpy(s1, s2, sizeof(s2)); // expected-warning {{size argument in 'strlcpy' call appears to be size of the source; expected the size of the destination}} expected-note {{change size argument to be the size of the destination}}
> + strlcpy(s1, s3, strlen(s3)+1); // expected-warning {{size argument in 'strlcpy' call appears to be size of the source; expected the size of the destination}} expected-note {{change size argument to be the size of the destination}}
> + strlcat(s2, s3, sizeof(s3)); // expected-warning {{size argument in 'strlcat' call appears to be size of the source; expected the size of the destination}} expected-note {{change size argument to be the size of the destination}}
> + strlcpy(s4.f1, s2, sizeof(s2)); // expected-warning {{size argument in 'strlcpy' call appears to be size of the source; expected the size of the destination}} expected-note {{change size argument to be the size of the destination}}
> + strlcpy((*s5)->f2[x], s2, sizeof(s2)); // expected-warning {{size argument in 'strlcpy' call appears to be size of the source; expected the size of the destination}} expected-note {{change size argument to be the size of the destination}}
> + strlcpy(s1+3, s2, sizeof(s2)); // expected-warning {{size argument in 'strlcpy' call appears to be size of the source; expected the size of the destination}}
> +}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list