<div dir="ltr"><div class="gmail_quote">[Re-send to correct addresses.]</div><div class="gmail_quote"><br><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Thu, Dec 26, 2013 at 3:38 PM, Nico Weber <span dir="ltr"><<a href="mailto:nicolasweber@gmx.de" target="_blank">nicolasweber@gmx.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: nico<br>
Date: Thu Dec 26 17:38:39 2013<br>
New Revision: 198063<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=198063&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=198063&view=rev</a><br>
Log:<br>
Warn on mismatched parentheses in memcmp and friends.<br>
<br>
Thisadds a new warning that warns on code like this:<br>
<br>
if (memcmp(a, b, sizeof(a) != 0))<br>
<br>
The warning looks like:<br>
<br>
test4.cc:5:30: warning: size argument in 'memcmp' call is a comparison [-Wmemsize-comparison]<br>
if (memcmp(a, b, sizeof(a) != 0))<br>
~~~~~~~~~~^~~~<br>
test4.cc:5:7: note: did you mean to compare the result of 'memcmp' instead?<br>
if (memcmp(a, b, sizeof(a) != 0))<br>
^ ~<br>
)<br>
test4.cc:5:20: note: explicitly cast the argument to size_t to silence this warning<br>
if (memcmp(a, b, sizeof(a) != 0))<br>
^<br>
(size_t)( )<br>
1 warning generated.<br>
<br>
This found 2 bugs in chromium and has 0 false positives on both chromium and<br>
llvm.<br>
<br>
The idea of triggering this warning on a binop in the size argument is due to<br>
rnk.<br>
<br>
<br>
Added:<br>
cfe/trunk/test/SemaCXX/warn-me<wbr>msize-comparison.cpp<br>
Modified:<br>
cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td<br>
cfe/trunk/lib/Sema/SemaCheckin<wbr>g.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=198063&r1=198062&r2=198063&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/include/clang/<wbr>Basic/DiagnosticSemaKinds.td?<wbr>rev=198063&r1=198062&r2=<wbr>198063&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td Thu Dec 26 17:38:39 2013<br>
@@ -390,11 +390,18 @@ def warn_sizeof_pointer_type_memac<wbr>cess :<br>
"%select{destination|source}<wbr>2; expected %3 or an explicit length">,<br>
InGroup<SizeofPointerMemacces<wbr>s>;<br>
def warn_strlcpycat_wrong_size : Warning<<br>
- "size argument in %0 call appears to be size of the source; expected the size of "<br>
- "the destination">,<br>
+ "size argument in %0 call appears to be size of the source; "<br>
+ "expected the size of the destination">,<br>
InGroup<DiagGroup<"strlcpy-st<wbr>rlcat-size">>;<br>
def note_strlcpycat_wrong_size : Note<<br>
"change size argument to be the size of the destination">;<br>
+def warn_memsize_comparison : Warning<<br>
+ "size argument in %0 call is a comparison">,<br>
+ InGroup<DiagGroup<"memsize-com<wbr>parison">>;<br>
+def warn_memsize_comparison_paren_<wbr>note : Note<<br>
+ "did you mean to compare the result of %0 instead?">;<br>
+def warn_memsize_comparison_cast_n<wbr>ote : Note<<br>
+ "explicitly cast the argument to size_t to silence this warning">;<br>
<br>
def warn_strncat_large_size : Warning<<br>
"the value of the size argument in 'strncat' is too large, might lead to a "<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaCheckin<wbr>g.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=198063&r1=198062&r2=198063&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/lib/Sema/SemaC<wbr>hecking.cpp?rev=198063&r1=<wbr>198062&r2=198063&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Sema/SemaCheckin<wbr>g.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaCheckin<wbr>g.cpp Thu Dec 26 17:38:39 2013<br>
@@ -622,7 +622,7 @@ bool Sema::CheckARMBuiltinFunctionC<wbr>all(u<br>
RHS.get(), AA_Assigning))<br>
return true;<br>
}<br>
-<br>
+<br>
// For NEON intrinsics which take an immediate value as part of the<br>
// instruction, range check them here.<br>
unsigned i = 0, l = 0, u = 0;<br>
@@ -3560,6 +3560,40 @@ void Sema::CheckFormatString(const Strin<br>
<br>
//===--- CHECK: Standard memory functions ------------------------------<wbr>---===//<br>
<br>
+/// \brief Takes the expression passed to the size_t parameter of functions<br>
+/// such as memcmp, strncat, etc and warns if it's a comparison.<br>
+///<br>
+/// This is to catch typos like `if (memcmp(&a, &b, sizeof(a) > 0))`.<br>
+static bool CheckMemorySizeofForComparison<wbr>(Sema &S, const Expr *E,<br>
+ IdentifierInfo *FnName,<br>
+ SourceLocation FnLoc,<br>
+ SourceLocation RParenLoc) {<br>
+ const BinaryOperator *Size = dyn_cast<BinaryOperator>(E);<br>
+ if (!Size)<br>
+ return false;<br>
+<br>
+ // if E is binop and op is >, <, >=, <=, ==, &&, ||:<br>
+ if (!Size->isComparisonOp() && !Size->isEqualityOp() && !Size->isLogicalOp())<br>
+ return false;<br>
+<br>
+ Preprocessor &PP = S.getPreprocessor();<br>
+ SourceRange SizeRange = Size->getSourceRange();<br>
+ S.Diag(Size->getOperatorLoc(), diag::warn_memsize_comparison)<br>
+ << SizeRange << FnName;<br>
+ S.Diag(FnLoc, diag::warn_memsize_comparison_<wbr>paren_note)<br>
+ << FnName<br>
+ << FixItHint::CreateInsertion(<br>
+ PP.getLocForEndOfToken(Size-><wbr>getLHS()->getLocEnd()),<br>
+ ")")<br>
+ << FixItHint::CreateRemoval(RPare<wbr>nLoc);<br>
+ S.Diag(SizeRange.getBegin(), diag::warn_memsize_comparison_<wbr>cast_note)<br>
+ << FixItHint::CreateInsertion(Siz<wbr>eRange.getBegin(), "(size_t)(")<br>
+ << FixItHint::CreateInsertion(<br>
+ PP.getLocForEndOfToken(SizeRa<wbr>nge.getEnd()), ")");<br>
+<br>
+ return true;<br>
+}<br>
+<br>
/// \brief Determine whether the given type is a dynamic class type (e.g.,<br>
/// whether it has a vtable).<br>
static bool isDynamicClassType(QualType T) {<br>
@@ -3615,6 +3649,10 @@ void Sema::CheckMemaccessArguments(<wbr>const<br>
unsigned LenArg = (BId == Builtin::BIstrndup ? 1 : 2);<br>
const Expr *LenExpr = Call->getArg(LenArg)->IgnorePa<wbr>renImpCasts();<br>
<br>
+ if (CheckMemorySizeofForCompariso<wbr>n(*this, LenExpr, FnName,<br>
+ Call->getLocStart(), Call->getRParenLoc()))<br>
+ return;<br>
+<br>
// We have special checking when the length is a sizeof expression.<br>
QualType SizeOfArgTy = getSizeOfArgType(LenExpr);<br>
const Expr *SizeOfArg = getSizeOfExprArg(LenExpr);<br>
@@ -3798,6 +3836,10 @@ void Sema::CheckStrlcpycatArguments<wbr>(cons<br>
const Expr *SrcArg = ignoreLiteralAdditions(Call->g<wbr>etArg(1), Context);<br>
const Expr *SizeArg = ignoreLiteralAdditions(Call->g<wbr>etArg(2), Context);<br>
const Expr *CompareWithSrc = NULL;<br>
+<br>
+ if (CheckMemorySizeofForCompariso<wbr>n(*this, SizeArg, FnName,<br>
+ Call->getLocStart(), Call->getRParenLoc()))<br>
+ return;<br>
<br>
// Look for 'strlcpy(dst, x, sizeof(x))'<br>
if (const Expr *Ex = getSizeOfExprArg(SizeArg))<br>
@@ -3880,6 +3922,10 @@ void Sema::CheckStrncatArguments(co<wbr>nst C<br>
const Expr *SrcArg = CE->getArg(1)->IgnoreParenCast<wbr>s();<br>
const Expr *LenArg = CE->getArg(2)->IgnoreParenCast<wbr>s();<br>
<br>
+ if (CheckMemorySizeofForCompariso<wbr>n(*this, LenArg, FnName, CE->getLocStart(),<br>
+ CE->getRParenLoc()))<br>
+ return;<br>
+<br>
// Identify common expressions, which are wrongly used as the size argument<br>
// to strncat and may lead to buffer overflows.<br>
unsigned PatternType = 0;<br>
@@ -5235,7 +5281,7 @@ void CheckImplicitConversion(Sema &S, Ex<br>
if (Target->isSpecificBuiltinType<wbr>(BuiltinType::Bool)) {<br>
if (isa<StringLiteral>(E))<br>
// Warn on string literal to bool. Checks for string literals in logical<br>
- // expressions, for instances, assert(0 && "error here"), is prevented<br>
+ // expressions, for instances, assert(0 && "error here"), are prevented<br>
// by a check in AnalyzeImplicitConversions().<br>
return DiagnoseImpCast(S, E, T, CC,<br>
diag::warn_impcast_string_lite<wbr>ral_to_bool);<br>
<br>
Added: cfe/trunk/test/SemaCXX/warn-me<wbr>msize-comparison.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp?rev=198063&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/SemaCXX/<wbr>warn-memsize-comparison.cpp?<wbr>rev=198063&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/SemaCXX/warn-me<wbr>msize-comparison.cpp (added)<br>
+++ cfe/trunk/test/SemaCXX/warn-me<wbr>msize-comparison.cpp Thu Dec 26 17:38:39 2013<br>
@@ -0,0 +1,93 @@<br>
+// RUN: %clang_cc1 -fsyntax-only -verify %s<br>
+//<br>
+<br>
+typedef __SIZE_TYPE__ size_t;<br>
+extern "C" void *memset(void *, int, size_t);<br>
+extern "C" void *memmove(void *s1, const void *s2, size_t n);<br>
+extern "C" void *memcpy(void *s1, const void *s2, size_t n);<br>
+extern "C" void *memcmp(void *s1, const void *s2, size_t n);<br>
+extern "C" int strncmp(const char *s1, const char *s2, size_t n);<br>
+extern "C" int strncasecmp(const char *s1, const char *s2, size_t n);<br>
+extern "C" char *strncpy(char *dst, const char *src, size_t n);<br>
+extern "C" char *strncat(char *dst, const char *src, size_t n);<br>
+extern "C" char *strndup(const char *src, size_t n);<br>
+extern "C" size_t strlcpy(char *dst, const char *src, size_t size);<br>
+extern "C" size_t strlcat(char *dst, const char *src, size_t size);<br>
+<br>
+void f() {<br>
+ char b1[80], b2[80];<br>
+ if (memset(b1, 0, sizeof(b1) != 0)) {} // \<br>
+ expected-warning{{size argument in 'memset' call is a comparison}} \<br>
+ expected-note {{did you mean to compare}} \<br>
+ expected-note {{explicitly cast the argument}}<br>
+ if (memset(b1, 0, sizeof(b1)) != 0) {}<br>
+<br>
+ if (memmove(b1, b2, sizeof(b1) == 0)) {} // \<br>
+ expected-warning{{size argument in 'memmove' call is a comparison}} \<br>
+ expected-note {{did you mean to compare}} \<br>
+ expected-note {{explicitly cast the argument}}<br>
+ if (memmove(b1, b2, sizeof(b1)) == 0) {}<br>
+<br>
+ if (memcpy(b1, b2, sizeof(b1) < 0)) {} // \<br>
+ expected-warning{{size argument in 'memcpy' call is a comparison}} \<br>
+ expected-note {{did you mean to compare}} \<br></blockquote><div><br></div></div></div><div>This note makes no sense in this case. memcpy returns a pointer; comparing it 'less than 0' is not meaningful...</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ expected-note {{explicitly cast the argument}}<br>
+ if (memcpy(b1, b2, sizeof(b1)) < 0) {}<br></blockquote><div><br></div></span><div>... and this 'corrected' form is ill-formed under C++ DR1512.</div><div><br></div><div>Do you think we should suppress the note in this case, or the entire warning?</div><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ if (memcmp(b1, b2, sizeof(b1) <= 0)) {} // \<br>
+ expected-warning{{size argument in 'memcmp' call is a comparison}} \<br>
+ expected-note {{did you mean to compare}} \<br>
+ expected-note {{explicitly cast the argument}}<br>
+ if (memcmp(b1, b2, sizeof(b1)) <= 0) {}<br>
+<br>
+ if (strncmp(b1, b2, sizeof(b1) > 0)) {} // \<br>
+ expected-warning{{size argument in 'strncmp' call is a comparison}} \<br>
+ expected-note {{did you mean to compare}} \<br>
+ expected-note {{explicitly cast the argument}}<br>
+ if (strncmp(b1, b2, sizeof(b1)) > 0) {}<br>
+<br>
+ if (strncasecmp(b1, b2, sizeof(b1) >= 0)) {} // \<br>
+ expected-warning{{size argument in 'strncasecmp' call is a comparison}} \<br>
+ expected-note {{did you mean to compare}} \<br>
+ expected-note {{explicitly cast the argument}}<br>
+ if (strncasecmp(b1, b2, sizeof(b1)) >= 0) {}<br>
+<br>
+ if (strncpy(b1, b2, sizeof(b1) == 0 || true)) {} // \<br>
+ expected-warning{{size argument in 'strncpy' call is a comparison}} \<br>
+ expected-note {{did you mean to compare}} \<br>
+ expected-note {{explicitly cast the argument}}<br>
+ if (strncpy(b1, b2, sizeof(b1)) == 0 || true) {}<br>
+<br>
+ if (strncat(b1, b2, sizeof(b1) - 1 >= 0 && true)) {} // \<br>
+ expected-warning{{size argument in 'strncat' call is a comparison}} \<br>
+ expected-note {{did you mean to compare}} \<br></blockquote><div><br></div></div></div><div>Likewise.</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ expected-note {{explicitly cast the argument}}<br>
+ if (strncat(b1, b2, sizeof(b1) - 1) >= 0 && true) {}<br>
+<br>
+ if (strndup(b1, sizeof(b1) != 0)) {} // \<br>
+ expected-warning{{size argument in 'strndup' call is a comparison}} \<br>
+ expected-note {{did you mean to compare}} \<br>
+ expected-note {{explicitly cast the argument}}<br>
+ if (strndup(b1, sizeof(b1)) != 0) {}<br>
+<br>
+ if (strlcpy(b1, b2, sizeof(b1) != 0)) {} // \<br>
+ expected-warning{{size argument in 'strlcpy' call is a comparison}} \<br>
+ expected-note {{did you mean to compare}} \<br>
+ expected-note {{explicitly cast the argument}}<br>
+ if (strlcpy(b1, b2, sizeof(b1)) != 0) {}<br>
+<br>
+ if (strlcat(b1, b2, sizeof(b1) != 0)) {} // \<br>
+ expected-warning{{size argument in 'strlcat' call is a comparison}} \<br>
+ expected-note {{did you mean to compare}} \<br>
+ expected-note {{explicitly cast the argument}}<br>
+ if (strlcat(b1, b2, sizeof(b1)) != 0) {}<br>
+<br>
+ if (memset(b1, 0, sizeof(b1) / 2)) {}<br>
+ if (memset(b1, 0, sizeof(b1) >> 2)) {}<br>
+ if (memset(b1, 0, 4 << 2)) {}<br>
+ if (memset(b1, 0, 4 + 2)) {}<br>
+ if (memset(b1, 0, 4 - 2)) {}<br>
+ if (memset(b1, 0, 4 * 2)) {}<br>
+<br>
+ if (memset(b1, 0, (size_t)(sizeof(b1) != 0))) {}<br>
+}<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailm<wbr>an/listinfo/cfe-commits</a><br>
</blockquote></div></div></div><br></div></div>
</div><br></div>