Fwd: r198063 - Warn on mismatched parentheses in memcmp and friends.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 18 10:53:41 PDT 2016
On Tue, Oct 18, 2016 at 9:31 AM, Nico Weber <thakis at chromium.org> wrote:
> 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.
>
I found this when implementing core issue 1512, under which the suggested
"fixed" code doesn't compile any more.
> On Oct 17, 2016 1:44 PM, "Richard Smith" <richard at metafoo.co.uk> wrote:
>
>> [Re-send to correct addresses.]
>>
>> On Thu, Dec 26, 2013 at 3:38 PM, Nico Weber <nicolasweber at gmx.de> wrote:
>>
>>> Author: nico
>>> Date: Thu Dec 26 17:38:39 2013
>>> New Revision: 198063
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=198063&view=rev
>>> Log:
>>> Warn on mismatched parentheses in memcmp and friends.
>>>
>>> Thisadds a new warning that warns on code like this:
>>>
>>> if (memcmp(a, b, sizeof(a) != 0))
>>>
>>> The warning looks like:
>>>
>>> test4.cc:5:30: warning: size argument in 'memcmp' call is a comparison
>>> [-Wmemsize-comparison]
>>> if (memcmp(a, b, sizeof(a) != 0))
>>> ~~~~~~~~~~^~~~
>>> test4.cc:5:7: note: did you mean to compare the result of 'memcmp'
>>> instead?
>>> if (memcmp(a, b, sizeof(a) != 0))
>>> ^ ~
>>> )
>>> test4.cc:5:20: note: explicitly cast the argument to size_t to silence
>>> this warning
>>> if (memcmp(a, b, sizeof(a) != 0))
>>> ^
>>> (size_t)( )
>>> 1 warning generated.
>>>
>>> This found 2 bugs in chromium and has 0 false positives on both chromium
>>> and
>>> llvm.
>>>
>>> The idea of triggering this warning on a binop in the size argument is
>>> due to
>>> rnk.
>>>
>>>
>>> Added:
>>> cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp
>>> Modified:
>>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>>
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/
>>> Basic/DiagnosticSemaKinds.td?rev=198063&r1=198062&r2=198063&view=diff
>>> ============================================================
>>> ==================
>>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Dec 26
>>> 17:38:39 2013
>>> @@ -390,11 +390,18 @@ def warn_sizeof_pointer_type_memaccess :
>>> "%select{destination|source}2; expected %3 or an explicit length">,
>>> InGroup<SizeofPointerMemaccess>;
>>> def warn_strlcpycat_wrong_size : Warning<
>>> - "size argument in %0 call appears to be size of the source; expected
>>> the size of "
>>> - "the destination">,
>>> + "size argument in %0 call appears to be size of the source; "
>>> + "expected the size of the destination">,
>>> InGroup<DiagGroup<"strlcpy-strlcat-size">>;
>>> def note_strlcpycat_wrong_size : Note<
>>> "change size argument to be the size of the destination">;
>>> +def warn_memsize_comparison : Warning<
>>> + "size argument in %0 call is a comparison">,
>>> + InGroup<DiagGroup<"memsize-comparison">>;
>>> +def warn_memsize_comparison_paren_note : Note<
>>> + "did you mean to compare the result of %0 instead?">;
>>> +def warn_memsize_comparison_cast_note : Note<
>>> + "explicitly cast the argument to size_t to silence this warning">;
>>>
>>> def warn_strncat_large_size : Warning<
>>> "the value of the size argument in 'strncat' is too large, might lead
>>> to a "
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaC
>>> hecking.cpp?rev=198063&r1=198062&r2=198063&view=diff
>>> ============================================================
>>> ==================
>>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Dec 26 17:38:39 2013
>>> @@ -622,7 +622,7 @@ bool Sema::CheckARMBuiltinFunctionCall(u
>>> RHS.get(), AA_Assigning))
>>> return true;
>>> }
>>> -
>>> +
>>> // For NEON intrinsics which take an immediate value as part of the
>>> // instruction, range check them here.
>>> unsigned i = 0, l = 0, u = 0;
>>> @@ -3560,6 +3560,40 @@ void Sema::CheckFormatString(const Strin
>>>
>>> //===--- CHECK: Standard memory functions ------------------------------
>>> ---===//
>>>
>>> +/// \brief Takes the expression passed to the size_t parameter of
>>> functions
>>> +/// such as memcmp, strncat, etc and warns if it's a comparison.
>>> +///
>>> +/// This is to catch typos like `if (memcmp(&a, &b, sizeof(a) > 0))`.
>>> +static bool CheckMemorySizeofForComparison(Sema &S, const Expr *E,
>>> + IdentifierInfo *FnName,
>>> + SourceLocation FnLoc,
>>> + SourceLocation RParenLoc) {
>>> + const BinaryOperator *Size = dyn_cast<BinaryOperator>(E);
>>> + if (!Size)
>>> + return false;
>>> +
>>> + // if E is binop and op is >, <, >=, <=, ==, &&, ||:
>>> + if (!Size->isComparisonOp() && !Size->isEqualityOp() &&
>>> !Size->isLogicalOp())
>>> + return false;
>>> +
>>> + Preprocessor &PP = S.getPreprocessor();
>>> + SourceRange SizeRange = Size->getSourceRange();
>>> + S.Diag(Size->getOperatorLoc(), diag::warn_memsize_comparison)
>>> + << SizeRange << FnName;
>>> + S.Diag(FnLoc, diag::warn_memsize_comparison_paren_note)
>>> + << FnName
>>> + << FixItHint::CreateInsertion(
>>> + PP.getLocForEndOfToken(Size->getLHS()->getLocEnd()),
>>> + ")")
>>> + << FixItHint::CreateRemoval(RParenLoc);
>>> + S.Diag(SizeRange.getBegin(), diag::warn_memsize_comparison_cast_note)
>>> + << FixItHint::CreateInsertion(SizeRange.getBegin(), "(size_t)(")
>>> + << FixItHint::CreateInsertion(
>>> + PP.getLocForEndOfToken(SizeRange.getEnd()), ")");
>>> +
>>> + return true;
>>> +}
>>> +
>>> /// \brief Determine whether the given type is a dynamic class type
>>> (e.g.,
>>> /// whether it has a vtable).
>>> static bool isDynamicClassType(QualType T) {
>>> @@ -3615,6 +3649,10 @@ void Sema::CheckMemaccessArguments(const
>>> unsigned LenArg = (BId == Builtin::BIstrndup ? 1 : 2);
>>> const Expr *LenExpr = Call->getArg(LenArg)->IgnoreParenImpCasts();
>>>
>>> + if (CheckMemorySizeofForComparison(*this, LenExpr, FnName,
>>> + Call->getLocStart(),
>>> Call->getRParenLoc()))
>>> + return;
>>> +
>>> // We have special checking when the length is a sizeof expression.
>>> QualType SizeOfArgTy = getSizeOfArgType(LenExpr);
>>> const Expr *SizeOfArg = getSizeOfExprArg(LenExpr);
>>> @@ -3798,6 +3836,10 @@ void Sema::CheckStrlcpycatArguments(cons
>>> const Expr *SrcArg = ignoreLiteralAdditions(Call->getArg(1),
>>> Context);
>>> const Expr *SizeArg = ignoreLiteralAdditions(Call->getArg(2),
>>> Context);
>>> const Expr *CompareWithSrc = NULL;
>>> +
>>> + if (CheckMemorySizeofForComparison(*this, SizeArg, FnName,
>>> + Call->getLocStart(),
>>> Call->getRParenLoc()))
>>> + return;
>>>
>>> // Look for 'strlcpy(dst, x, sizeof(x))'
>>> if (const Expr *Ex = getSizeOfExprArg(SizeArg))
>>> @@ -3880,6 +3922,10 @@ void Sema::CheckStrncatArguments(const C
>>> const Expr *SrcArg = CE->getArg(1)->IgnoreParenCasts();
>>> const Expr *LenArg = CE->getArg(2)->IgnoreParenCasts();
>>>
>>> + if (CheckMemorySizeofForComparison(*this, LenArg, FnName,
>>> CE->getLocStart(),
>>> + CE->getRParenLoc()))
>>> + return;
>>> +
>>> // Identify common expressions, which are wrongly used as the size
>>> argument
>>> // to strncat and may lead to buffer overflows.
>>> unsigned PatternType = 0;
>>> @@ -5235,7 +5281,7 @@ void CheckImplicitConversion(Sema &S, Ex
>>> if (Target->isSpecificBuiltinType(BuiltinType::Bool)) {
>>> if (isa<StringLiteral>(E))
>>> // Warn on string literal to bool. Checks for string literals in
>>> logical
>>> - // expressions, for instances, assert(0 && "error here"), is
>>> prevented
>>> + // expressions, for instances, assert(0 && "error here"), are
>>> prevented
>>> // by a check in AnalyzeImplicitConversions().
>>> return DiagnoseImpCast(S, E, T, CC,
>>> diag::warn_impcast_string_lite
>>> ral_to_bool);
>>>
>>> Added: cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/w
>>> arn-memsize-comparison.cpp?rev=198063&view=auto
>>> ============================================================
>>> ==================
>>> --- cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp (added)
>>> +++ cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp Thu Dec 26
>>> 17:38:39 2013
>>> @@ -0,0 +1,93 @@
>>> +// RUN: %clang_cc1 -fsyntax-only -verify %s
>>> +//
>>> +
>>> +typedef __SIZE_TYPE__ size_t;
>>> +extern "C" void *memset(void *, int, size_t);
>>> +extern "C" void *memmove(void *s1, const void *s2, size_t n);
>>> +extern "C" void *memcpy(void *s1, const void *s2, size_t n);
>>> +extern "C" void *memcmp(void *s1, const void *s2, size_t n);
>>> +extern "C" int strncmp(const char *s1, const char *s2, size_t n);
>>> +extern "C" int strncasecmp(const char *s1, const char *s2, size_t n);
>>> +extern "C" char *strncpy(char *dst, const char *src, size_t n);
>>> +extern "C" char *strncat(char *dst, const char *src, size_t n);
>>> +extern "C" char *strndup(const char *src, size_t n);
>>> +extern "C" size_t strlcpy(char *dst, const char *src, size_t size);
>>> +extern "C" size_t strlcat(char *dst, const char *src, size_t size);
>>> +
>>> +void f() {
>>> + char b1[80], b2[80];
>>> + if (memset(b1, 0, sizeof(b1) != 0)) {} // \
>>> + expected-warning{{size argument in 'memset' call is a comparison}} \
>>> + expected-note {{did you mean to compare}} \
>>> + expected-note {{explicitly cast the argument}}
>>> + if (memset(b1, 0, sizeof(b1)) != 0) {}
>>> +
>>> + if (memmove(b1, b2, sizeof(b1) == 0)) {} // \
>>> + expected-warning{{size argument in 'memmove' call is a comparison}}
>>> \
>>> + expected-note {{did you mean to compare}} \
>>> + expected-note {{explicitly cast the argument}}
>>> + if (memmove(b1, b2, sizeof(b1)) == 0) {}
>>> +
>>> + if (memcpy(b1, b2, sizeof(b1) < 0)) {} // \
>>> + expected-warning{{size argument in 'memcpy' call is a comparison}} \
>>> + expected-note {{did you mean to compare}} \
>>>
>>
>> This note makes no sense in this case. memcpy returns a pointer;
>> comparing it 'less than 0' is not meaningful...
>>
>>
>>> + expected-note {{explicitly cast the argument}}
>>> + if (memcpy(b1, b2, sizeof(b1)) < 0) {}
>>>
>>
>> ... and this 'corrected' form is ill-formed under C++ DR1512.
>>
>> Do you think we should suppress the note in this case, or the entire
>> warning?
>>
>> +
>>> + if (memcmp(b1, b2, sizeof(b1) <= 0)) {} // \
>>> + expected-warning{{size argument in 'memcmp' call is a comparison}} \
>>> + expected-note {{did you mean to compare}} \
>>> + expected-note {{explicitly cast the argument}}
>>> + if (memcmp(b1, b2, sizeof(b1)) <= 0) {}
>>> +
>>> + if (strncmp(b1, b2, sizeof(b1) > 0)) {} // \
>>> + expected-warning{{size argument in 'strncmp' call is a comparison}}
>>> \
>>> + expected-note {{did you mean to compare}} \
>>> + expected-note {{explicitly cast the argument}}
>>> + if (strncmp(b1, b2, sizeof(b1)) > 0) {}
>>> +
>>> + if (strncasecmp(b1, b2, sizeof(b1) >= 0)) {} // \
>>> + expected-warning{{size argument in 'strncasecmp' call is a
>>> comparison}} \
>>> + expected-note {{did you mean to compare}} \
>>> + expected-note {{explicitly cast the argument}}
>>> + if (strncasecmp(b1, b2, sizeof(b1)) >= 0) {}
>>> +
>>> + if (strncpy(b1, b2, sizeof(b1) == 0 || true)) {} // \
>>> + expected-warning{{size argument in 'strncpy' call is a comparison}}
>>> \
>>> + expected-note {{did you mean to compare}} \
>>> + expected-note {{explicitly cast the argument}}
>>> + if (strncpy(b1, b2, sizeof(b1)) == 0 || true) {}
>>> +
>>> + if (strncat(b1, b2, sizeof(b1) - 1 >= 0 && true)) {} // \
>>> + expected-warning{{size argument in 'strncat' call is a comparison}}
>>> \
>>> + expected-note {{did you mean to compare}} \
>>>
>>
>> Likewise.
>>
>>
>>> + expected-note {{explicitly cast the argument}}
>>> + if (strncat(b1, b2, sizeof(b1) - 1) >= 0 && true) {}
>>> +
>>> + if (strndup(b1, sizeof(b1) != 0)) {} // \
>>> + expected-warning{{size argument in 'strndup' call is a comparison}}
>>> \
>>> + expected-note {{did you mean to compare}} \
>>> + expected-note {{explicitly cast the argument}}
>>> + if (strndup(b1, sizeof(b1)) != 0) {}
>>> +
>>> + if (strlcpy(b1, b2, sizeof(b1) != 0)) {} // \
>>> + expected-warning{{size argument in 'strlcpy' call is a comparison}}
>>> \
>>> + expected-note {{did you mean to compare}} \
>>> + expected-note {{explicitly cast the argument}}
>>> + if (strlcpy(b1, b2, sizeof(b1)) != 0) {}
>>> +
>>> + if (strlcat(b1, b2, sizeof(b1) != 0)) {} // \
>>> + expected-warning{{size argument in 'strlcat' call is a comparison}}
>>> \
>>> + expected-note {{did you mean to compare}} \
>>> + expected-note {{explicitly cast the argument}}
>>> + if (strlcat(b1, b2, sizeof(b1)) != 0) {}
>>> +
>>> + if (memset(b1, 0, sizeof(b1) / 2)) {}
>>> + if (memset(b1, 0, sizeof(b1) >> 2)) {}
>>> + if (memset(b1, 0, 4 << 2)) {}
>>> + if (memset(b1, 0, 4 + 2)) {}
>>> + if (memset(b1, 0, 4 - 2)) {}
>>> + if (memset(b1, 0, 4 * 2)) {}
>>> +
>>> + if (memset(b1, 0, (size_t)(sizeof(b1) != 0))) {}
>>> +}
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161018/35e418eb/attachment-0001.html>
More information about the cfe-commits
mailing list