<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Oct 18, 2016 at 9:31 AM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr">Did you find real-world code triggering this, or did you happen to see the implementation of the warning? I'm not sure if omitting just the note or the whole warning is more useful in practice. Intuitively I'd say that people "never" will compare the result of memcpy, so I'd guess that it doesn't matter much what we pick and omitting the warning completely is fine, but I'm not sure.</p></blockquote><div>I found this when implementing core issue 1512, under which the suggested "fixed" code doesn't compile any more.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><div class="gmail_quote">On Oct 17, 2016 1:44 PM, "Richard Smith" <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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="m_-3886657669347822752m_8122119224450443703h5">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?r<wbr>ev=198063&r1=198062&r2=198063&<wbr>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}2<wbr>; 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=1980<wbr>62&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/w<wbr>arn-memsize-comparison.cpp?rev<wbr>=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><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="m_-3886657669347822752m_8122119224450443703h5"><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="m_-3886657669347822752m_8122119224450443703h5"><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>
</blockquote></div></div>
</div></div></blockquote></div><br></div></div>