From 13b0857ad084ef0fcd1476b85bd1f3a7fefdcf0c Mon Sep 17 00:00:00 2001 From: Anna Zaks Date: Wed, 8 Aug 2012 21:42:23 +0000 Subject: [PATCH] Address code review comments for Wstrncat-size warning (r161440). llvm-svn: 161527 --- .../clang/Basic/DiagnosticSemaKinds.td | 6 ++- clang/lib/Sema/SemaChecking.cpp | 50 ++++++++++--------- clang/test/Sema/warn-strncat-size.c | 7 ++- 3 files changed, 36 insertions(+), 27 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 7b3cd0fb7720..357a63bce613 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -373,9 +373,11 @@ def note_strlcpycat_wrong_size : Note< def warn_strncat_large_size : Warning< "the value of the size argument in 'strncat' is too large, might lead to a " - "buffer overflow">, InGroup, DefaultWarnNoWerror; + "buffer overflow">, InGroup; def warn_strncat_src_size : Warning<"size argument in 'strncat' call appears " - "to be size of the source">, InGroup, DefaultWarnNoWerror; + "to be size of the source">, InGroup; +def warn_strncat_wrong_size : Warning< + "the value of the size argument to 'strncat' is wrong">, InGroup; def note_strncat_wrong_size : Note< "change the argument to be the free space in the destination buffer minus " "the terminating null byte">; diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 5e4309195eb3..a58e9049e48c 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -3101,6 +3101,19 @@ static const Expr *ignoreLiteralAdditions(const Expr *Ex, ASTContext &Ctx) { return Ex; } +static bool isConstantSizeArrayWithMoreThanOneElement(QualType Ty, + ASTContext &Context) { + // Only handle constant-sized or VLAs, but not flexible members. + if (const ConstantArrayType *CAT = Context.getAsConstantArrayType(Ty)) { + // Only issue the FIXIT for arrays of size > 1. + if (CAT->getSize().getSExtValue() <= 1) + return false; + } else if (!Ty->isVariableArrayType()) { + return false; + } + return true; +} + // 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, @@ -3151,16 +3164,8 @@ void Sema::CheckStrlcpycatArguments(const CallExpr *Call, // 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(); - 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()) { + if (!isConstantSizeArrayWithMoreThanOneElement(DstArg->getType(), Context)) return; - } SmallString<128> sizeString; llvm::raw_svector_ostream OS(sizeString); @@ -3242,26 +3247,23 @@ void Sema::CheckStrncatArguments(const CallExpr *CE, SM.getSpellingLoc(SR.getEnd())); } + // Check if the destination is an array (rather than a pointer to an array). + QualType DstTy = DstArg->getType(); + bool isKnownSizeArray = isConstantSizeArrayWithMoreThanOneElement(DstTy, + Context); + if (!isKnownSizeArray) { + if (PatternType == 1) + Diag(SL, diag::warn_strncat_wrong_size) << SR; + else + Diag(SL, diag::warn_strncat_src_size) << SR; + return; + } + if (PatternType == 1) Diag(SL, diag::warn_strncat_large_size) << SR; else Diag(SL, diag::warn_strncat_src_size) << SR; - // 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; - } - SmallString<128> sizeString; llvm::raw_svector_ostream OS(sizeString); OS << "sizeof("; diff --git a/clang/test/Sema/warn-strncat-size.c b/clang/test/Sema/warn-strncat-size.c index 7157edffea47..dcc3367e9499 100644 --- a/clang/test/Sema/warn-strncat-size.c +++ b/clang/test/Sema/warn-strncat-size.c @@ -59,7 +59,7 @@ 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}} + strncat(z, str, sizeof(z)); // expected-warning{{the value of the size argument to 'strncat' is wrong}} } // Support VLAs. @@ -69,3 +69,8 @@ void vlas(int size) { 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}} } + +// Non-array type gets a different error message. +void f(char* s, char* d) { + strncat(d, s, sizeof(d)); // expected-warning {{the value of the size argument to 'strncat' is wrong}} +}