From c36bedc90c687caa71748480c60707ea4608b092 Mon Sep 17 00:00:00 2001 From: Anna Zaks Date: Wed, 1 Feb 2012 19:08:57 +0000 Subject: [PATCH] Add a new compiler warning, which flags anti-patterns used as the size argument in strncat. The warning is ignored by default since it needs more qualification. TODO: The warning message and the note are messy when strncat is a builtin due to the macro expansion. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@149524 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticGroups.td | 1 + include/clang/Basic/DiagnosticSemaKinds.td | 9 ++ include/clang/Sema/Sema.h | 3 + lib/AST/Decl.cpp | 7 ++ lib/Sema/SemaChecking.cpp | 113 ++++++++++++++++++++- test/Sema/warn-strncat-size.c | 60 +++++++++++ test/SemaCXX/warn-memset-bad-sizeof.cpp | 2 - 7 files changed, 189 insertions(+), 6 deletions(-) create mode 100644 test/Sema/warn-strncat-size.c diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index d5a0eea4d5..f3e2634d64 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -158,6 +158,7 @@ def : DiagGroup<"stack-protector">; def : DiagGroup<"switch-default">; def : DiagGroup<"synth">; def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">; +def StrncatSize : DiagGroup<"strncat-size">; def TautologicalCompare : DiagGroup<"tautological-compare">; def HeaderHygiene : DiagGroup<"header-hygiene">; diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 84f67cdb90..52de6ca26e 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -314,6 +314,15 @@ def warn_strlcpycat_wrong_size : Warning< InGroup>; def note_strlcpycat_wrong_size : Note< "change size argument to be the size of the destination">; + +def warn_strncat_large_size : Warning< + "the value of the size argument in 'strncat' is too large, might lead to a " + "buffer overflow">, InGroup, DefaultIgnore; +def warn_strncat_src_size : Warning<"size argument in 'strncat' call appears " + "to be size of the source">, InGroup, DefaultIgnore; +def note_strncat_wrong_size : Note< + "change the argument to be the free space in the destination buffer minus " + "the terminating null byte">; /// main() // static/inline main() are not errors in C, just in C++. diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 5b87bb98fa..609c64e0c7 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -6347,6 +6347,9 @@ private: void CheckStrlcpycatArguments(const CallExpr *Call, IdentifierInfo *FnName); + void CheckStrncatArguments(const CallExpr *Call, + IdentifierInfo *FnName); + void CheckReturnStackAddr(Expr *RetValExp, QualType lhsType, SourceLocation ReturnLoc); void CheckFloatComparison(SourceLocation Loc, Expr* LHS, Expr* RHS); diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 49227e9195..aa7bb0812a 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -2333,6 +2333,7 @@ unsigned FunctionDecl::getMemoryFunctionKind() const { return Builtin::BIstrncasecmp; case Builtin::BI__builtin_strncat: + case Builtin::BI__builtin___strncat_chk: case Builtin::BIstrncat: return Builtin::BIstrncat; @@ -2340,6 +2341,10 @@ unsigned FunctionDecl::getMemoryFunctionKind() const { case Builtin::BIstrndup: return Builtin::BIstrndup; + case Builtin::BI__builtin_strlen: + case Builtin::BIstrlen: + return Builtin::BIstrlen; + default: if (isExternC()) { if (FnInfo->isStr("memset")) @@ -2360,6 +2365,8 @@ unsigned FunctionDecl::getMemoryFunctionKind() const { return Builtin::BIstrncat; else if (FnInfo->isStr("strndup")) return Builtin::BIstrndup; + else if (FnInfo->isStr("strlen")) + return Builtin::BIstrlen; } break; } diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp index 4b507f456c..ee04463037 100644 --- a/lib/Sema/SemaChecking.cpp +++ b/lib/Sema/SemaChecking.cpp @@ -456,6 +456,8 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall) { // Handle memory setting and copying functions. if (CMId == Builtin::BIstrlcpy || CMId == Builtin::BIstrlcat) CheckStrlcpycatArguments(TheCall, FnInfo); + else if (CMId == Builtin::BIstrncat) + CheckStrncatArguments(TheCall, FnInfo); else CheckMemaccessArguments(TheCall, CMId, FnInfo); @@ -2683,6 +2685,99 @@ void Sema::CheckStrlcpycatArguments(const CallExpr *Call, OS.str()); } +/// Check if two expressions refer to the same declaration. +static bool referToTheSameDecl(const Expr *E1, const Expr *E2) { + if (const DeclRefExpr *D1 = dyn_cast_or_null(E1)) + if (const DeclRefExpr *D2 = dyn_cast_or_null(E2)) + return D1->getDecl() == D2->getDecl(); + return false; +} + +static const Expr *getStrlenExprArg(const Expr *E) { + if (const CallExpr *CE = dyn_cast(E)) { + const FunctionDecl *FD = CE->getDirectCallee(); + if (!FD || FD->getMemoryFunctionKind() != Builtin::BIstrlen) + return 0; + return CE->getArg(0)->IgnoreParenCasts(); + } + return 0; +} + +// Warn on anti-patterns as the 'size' argument to strncat. +// The correct size argument should look like following: +// strncat(dst, src, sizeof(dst) - strlen(dest) - 1); +void Sema::CheckStrncatArguments(const CallExpr *CE, + IdentifierInfo *FnName) { + // Don't crash if the user has the wrong number of arguments. + if (CE->getNumArgs() < 3) + return; + const Expr *DstArg = CE->getArg(0)->IgnoreParenCasts(); + const Expr *SrcArg = CE->getArg(1)->IgnoreParenCasts(); + const Expr *LenArg = CE->getArg(2)->IgnoreParenCasts(); + + // Identify common expressions, which are wrongly used as the size argument + // to strncat and may lead to buffer overflows. + unsigned PatternType = 0; + if (const Expr *SizeOfArg = getSizeOfExprArg(LenArg)) { + // - sizeof(dst) + if (referToTheSameDecl(SizeOfArg, DstArg)) + PatternType = 1; + // - sizeof(src) + else if (referToTheSameDecl(SizeOfArg, SrcArg)) + PatternType = 2; + } else if (const BinaryOperator *BE = dyn_cast(LenArg)) { + if (BE->getOpcode() == BO_Sub) { + const Expr *L = BE->getLHS()->IgnoreParenCasts(); + const Expr *R = BE->getRHS()->IgnoreParenCasts(); + // - sizeof(dst) - strlen(dst) + if (referToTheSameDecl(DstArg, getSizeOfExprArg(L)) && + referToTheSameDecl(DstArg, getStrlenExprArg(R))) + PatternType = 1; + // - sizeof(src) - (anything) + else if (referToTheSameDecl(SrcArg, getSizeOfExprArg(L))) + PatternType = 2; + } + } + + if (PatternType == 0) + return; + + if (PatternType == 1) + Diag(DstArg->getLocStart(), diag::warn_strncat_large_size) + << LenArg->getSourceRange(); + else + Diag(DstArg->getLocStart(), diag::warn_strncat_src_size) + << LenArg->getSourceRange(); + + // 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'. + QualType DstArgTy = DstArg->getType(); + + // Only handle constant-sized or VLAs, but not flexible members. + if (const ConstantArrayType *CAT = Context.getAsConstantArrayType(DstArgTy)) { + // Only issue the FIXIT for arrays of size > 1. + if (CAT->getSize().getSExtValue() <= 1) + return; + } else if (!DstArgTy->isVariableArrayType()) { + return; + } + + llvm::SmallString<128> sizeString; + llvm::raw_svector_ostream OS(sizeString); + OS << "sizeof("; + DstArg->printPretty(OS, Context, 0, getPrintingPolicy()); + OS << ") - "; + OS << "strlen("; + DstArg->printPretty(OS, Context, 0, getPrintingPolicy()); + OS << ") - 1"; + + Diag(LenArg->getLocStart(), diag::note_strncat_wrong_size) + << FixItHint::CreateReplacement(LenArg->getSourceRange(), + OS.str()); +} + //===--- CHECK: Return Address of Stack Variable --------------------------===// static Expr *EvalVal(Expr *E, SmallVectorImpl &refVars); @@ -3699,15 +3794,24 @@ static void AnalyzeAssignment(Sema &S, BinaryOperator *E) { /// Diagnose an implicit cast; purely a helper for CheckImplicitConversion. static void DiagnoseImpCast(Sema &S, Expr *E, QualType SourceType, QualType T, - SourceLocation CContext, unsigned diag) { + SourceLocation CContext, unsigned diag, + bool pruneControlFlow = false) { + if (pruneControlFlow) { + S.DiagRuntimeBehavior(E->getExprLoc(), E, + S.PDiag(diag) + << SourceType << T << E->getSourceRange() + << SourceRange(CContext)); + return; + } S.Diag(E->getExprLoc(), diag) << SourceType << T << E->getSourceRange() << SourceRange(CContext); } /// Diagnose an implicit cast; purely a helper for CheckImplicitConversion. static void DiagnoseImpCast(Sema &S, Expr *E, QualType T, - SourceLocation CContext, unsigned diag) { - DiagnoseImpCast(S, E, E->getType(), T, CContext, diag); + SourceLocation CContext, unsigned diag, + bool pruneControlFlow = false) { + DiagnoseImpCast(S, E, E->getType(), T, CContext, diag, pruneControlFlow); } /// Diagnose an implicit cast from a literal expression. Does not warn when the @@ -3913,7 +4017,8 @@ void CheckImplicitConversion(Sema &S, Expr *E, QualType T, return; if (SourceRange.Width == 64 && TargetRange.Width == 32) - return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_integer_64_32); + return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_integer_64_32, + /* pruneControlFlow */ true); return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_integer_precision); } diff --git a/test/Sema/warn-strncat-size.c b/test/Sema/warn-strncat-size.c new file mode 100644 index 0000000000..4233f25d5a --- /dev/null +++ b/test/Sema/warn-strncat-size.c @@ -0,0 +1,60 @@ +// RUN: %clang_cc1 -Wstrncat-size -verify -fsyntax-only %s + +typedef __SIZE_TYPE__ size_t; +char *strncat(char *, const char *, size_t); +size_t strlen (const char *s); + +struct { + char f1[100]; + char f2[100][3]; +} s4, **s5; + +char s1[100]; +char s2[200]; +int x; + +void test(char *src) { + char dest[10]; + + strncat(dest, "AAAAAAAAAAAAAAAAAAAAAAAAAAAAA", sizeof(dest) - strlen(dest) - 1); // no-warning + strncat(dest, "AAAAAAAAAAAAAAAAAAAAAAAAAAAAA", sizeof(dest) - 1); // no-warning - the code might assume that dest is empty + + strncat(dest, src, sizeof(src)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}} + + strncat(dest, src, sizeof(src) - 1); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}} + + strncat(dest, "AAAAAAAAAAAAAAAAAAAAAAAAAAA", sizeof(dest)); // expected-warning{{the value of the size argument in 'strncat' is too large, might lead to a buffer overflow}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}} + + strncat(dest, "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", sizeof(dest) - strlen(dest)); // expected-warning{{the value of the size argument in 'strncat' is too large, might lead to a buffer overflow}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}} + + strncat((*s5)->f2[x], s2, sizeof(s2)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}} + strncat(s1+3, s2, sizeof(s2)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} + strncat(s4.f1, s2, sizeof(s2)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}} +} + +// Don't issue FIXIT for flexible arrays. +struct S { + int y; + char x[]; +}; + +void flexible_arrays(struct S *s) { + char str[] = "hi"; + strncat(s->x, str, sizeof(str)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} +} + +// Don't issue FIXIT for destinations of size 1. +void size_1() { + char z[1]; + char str[] = "hi"; + + strncat(z, str, sizeof(z)); // expected-warning{{the value of the size argument in 'strncat' is too large, might lead to a buffer overflow}} +} + +// Support VLAs. +void vlas(int size) { + char z[size]; + char str[] = "hi"; + + strncat(z, str, sizeof(str)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}} +} diff --git a/test/SemaCXX/warn-memset-bad-sizeof.cpp b/test/SemaCXX/warn-memset-bad-sizeof.cpp index a018223cbd..388e362768 100644 --- a/test/SemaCXX/warn-memset-bad-sizeof.cpp +++ b/test/SemaCXX/warn-memset-bad-sizeof.cpp @@ -132,8 +132,6 @@ void strcpy_and_friends() { strncpy(buff, BAR, sizeof(BAR)); // \ // expected-warning {{argument to 'sizeof' in 'strncpy' call is the same expression as the source}} - strncat(buff, BAR, sizeof(BAR)); // \ - // expected-warning {{argument to 'sizeof' in 'strncat' call is the same expression as the source}} strndup(FOO, sizeof(FOO)); // \ // expected-warning {{argument to 'sizeof' in 'strndup' call is the same expression as the source}} } -- 2.40.0