[cfe-commits] r149524 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/AST/Decl.cpp lib/Sema/SemaChecking.cpp test/Sema/warn-strncat-size.c test/SemaCXX/warn-memset-bad-sizeof.cpp
Anna Zaks
ganna at apple.com
Wed Feb 1 11:08:58 PST 2012
Author: zaks
Date: Wed Feb 1 13:08:57 2012
New Revision: 149524
URL: http://llvm.org/viewvc/llvm-project?rev=149524&view=rev
Log:
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.
Added:
cfe/trunk/test/Sema/warn-strncat-size.c
Modified:
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/AST/Decl.cpp
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=149524&r1=149523&r2=149524&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Wed Feb 1 13:08:57 2012
@@ -158,6 +158,7 @@
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">;
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=149524&r1=149523&r2=149524&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed Feb 1 13:08:57 2012
@@ -314,6 +314,15 @@
InGroup<DiagGroup<"strlcpy-strlcat-size">>;
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<StrncatSize>, DefaultIgnore;
+def warn_strncat_src_size : Warning<"size argument in 'strncat' call appears "
+ "to be size of the source">, InGroup<StrncatSize>, 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++.
Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=149524&r1=149523&r2=149524&view=diff
==============================================================================
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Wed Feb 1 13:08:57 2012
@@ -6347,6 +6347,9 @@
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);
Modified: cfe/trunk/lib/AST/Decl.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=149524&r1=149523&r2=149524&view=diff
==============================================================================
--- cfe/trunk/lib/AST/Decl.cpp (original)
+++ cfe/trunk/lib/AST/Decl.cpp Wed Feb 1 13:08:57 2012
@@ -2333,6 +2333,7 @@
return Builtin::BIstrncasecmp;
case Builtin::BI__builtin_strncat:
+ case Builtin::BI__builtin___strncat_chk:
case Builtin::BIstrncat:
return Builtin::BIstrncat;
@@ -2340,6 +2341,10 @@
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 @@
return Builtin::BIstrncat;
else if (FnInfo->isStr("strndup"))
return Builtin::BIstrndup;
+ else if (FnInfo->isStr("strlen"))
+ return Builtin::BIstrlen;
}
break;
}
Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=149524&r1=149523&r2=149524&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Wed Feb 1 13:08:57 2012
@@ -456,6 +456,8 @@
// 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 @@
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<DeclRefExpr>(E1))
+ if (const DeclRefExpr *D2 = dyn_cast_or_null<DeclRefExpr>(E2))
+ return D1->getDecl() == D2->getDecl();
+ return false;
+}
+
+static const Expr *getStrlenExprArg(const Expr *E) {
+ if (const CallExpr *CE = dyn_cast<CallExpr>(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<BinaryOperator>(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<DeclRefExpr *> &refVars);
@@ -3699,15 +3794,24 @@
/// 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 @@
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);
}
Added: cfe/trunk/test/Sema/warn-strncat-size.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-strncat-size.c?rev=149524&view=auto
==============================================================================
--- cfe/trunk/test/Sema/warn-strncat-size.c (added)
+++ cfe/trunk/test/Sema/warn-strncat-size.c Wed Feb 1 13:08:57 2012
@@ -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}}
+}
Modified: cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof.cpp?rev=149524&r1=149523&r2=149524&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof.cpp Wed Feb 1 13:08:57 2012
@@ -132,8 +132,6 @@
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}}
}
More information about the cfe-commits
mailing list