[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
Ted Kremenek
kremenek at apple.com
Fri Aug 19 21:58:12 PDT 2011
My plan was do to more testing with this warning on large amounts of code before deciding whether or not to (a) put it under -Wall, (b) have it on by default, or (c) make it opt-in.
On Aug 19, 2011, at 9:47 PM, Nico Weber wrote:
> 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