r337470 - [Sema] Add a new warning, -Wmemset-transposed-args
Erik Pilkington via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 26 13:13:22 PDT 2018
On 7/26/18 12:52 PM, Richard Smith wrote:
> Other than the (fixed) ffmpeg false positive, I see this firing in
> three places.
>
> One of them is in the libc tests for memset and bzero, where the 0
> argument is intentional.
I don't find this case very convincing, if you're literally the one
person that has to test memset then you should probably just compile
with -Wno-suspicious-memaccess.
> One of them is here:
> https://github.com/apache/xerces-c/blob/trunk/src/xercesc/util/XMLUTF16Transcoder.cpp#L114
> (note that this code is correct).
> And one of them was a real bug where the second and third arguments of
> memset were transposed.
>
> That's an extremely low false positive rate, much lower than what we'd
> expect for an enabled-by-default warning. I find this extremely
> surprising; I would have expected the ratio of true-- to
> false-positives to be much much higher. But ultimately data wins out
> over expectations.
> How does our experience compare to other people's experiences? Are our
> findings abnormal? (They may well be; our corpus is very C++-heavy.)
> If this is indeed typical, we should reconsider whether to have these
> warnings enabled by default, as they do not meet our bar for false
> positives.
I tested this internally, and it found ~50 true-positives and only one
false-positive (similar to apache bug above, where someone was
memsetting to a sizeof exression). Given those numbers, my position is
to keep it on-by-default, but I'm open to hear more. We might be able to
make this warning even more conservative in this case by checking if the
sizeof expression is actually computing a size that corresponds to the
type of the third argument. I think that would be a good tradeoff if it
meant we could keep this on by default.
> On Mon, 23 Jul 2018 at 09:28, Erik Pilkington via cfe-commits
> <cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>> wrote:
>
> Sure, that seems pretty reasonable. r337706. Thanks for the
> suggestion!
>
>
> On 7/21/18 1:40 PM, Arthur O'Dwyer wrote:
>> In this case Clang is complaining about
>>
>> memset(complicated-expr, 0, 0);
>>
>> which means that transposing the last two arguments would not
>> "fix" anything. Clang ought to not complain in this case.
>> It doesn't have anything to do with coming from a macro; it's
>> just that /both/ arguments are zero.
>> (I would also expect that `memset(complicated-expr, (unsigned
>> char)fill, (size_t)0)` shouldn't warn, either. But we had some
>> disagreement that casts to `(unsigned char)` resp. `(size_t)`
>> should be the right way to shut up the warning in the first place...)
>> Basically Clang should only be suggesting a swap (and thus
>> complaining at all) in the case that swapping the arguments would
>> actually improve the situation.
>>
>> –Arthur
>>
>> On Sat, Jul 21, 2018 at 1:33 PM, Nico Weber via cfe-commits
>> <cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>>
>> wrote:
>>
>> This has a false positive on ffmpeg:
>>
>> ../../third_party/ffmpeg/libavcodec/options.c:273:64: error:
>> 'size' argument to memset is '0'; did you mean to transpose
>> the last two arguments? [-Werror,-Wmemset-transposed-args]
>> alloc_and_copy_or_fail(intra_matrix, 64 * sizeof(int16_t), 0);
>>
>> With:
>>
>> #define alloc_and_copy_or_fail(obj, size, pad) \
>> if (src->obj && size > 0) { \
>> dest->obj = av_malloc(size + pad); \
>> if (!dest->obj) \
>> goto fail; \
>> memcpy(dest->obj, src->obj, size); \
>> if (pad) \
>> memset(((uint8_t *) dest->obj) + size, 0, pad); \
>> }
>>
>> (https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavcodec/options.c?q=alloc_and_copy_or_fail&sq=package:chromium&g=0&l=261
>> ; https://bugs.chromium.org/p/chromium/issues/detail?id=866202)
>>
>> Maybe the warning shouldn't fire if the memset is from a macro?
>>
>> On Thu, Jul 19, 2018 at 12:51 PM Erik Pilkington via
>> cfe-commits <cfe-commits at lists.llvm.org
>> <mailto:cfe-commits at lists.llvm.org>> wrote:
>>
>> Author: epilk
>> Date: Thu Jul 19 09:46:15 2018
>> New Revision: 337470
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=337470&view=rev
>> Log:
>> [Sema] Add a new warning, -Wmemset-transposed-args
>>
>> This diagnoses calls to memset that have the second and
>> third arguments
>> transposed, for example:
>>
>> memset(buf, sizeof(buf), 0);
>>
>> This is done by checking if the third argument is a
>> literal 0, or if the second
>> is a sizeof expression (and the third isn't). The first
>> check is also done for
>> calls to bzero.
>>
>> Differential revision: https://reviews.llvm.org/D49112
>>
>> Added:
>> cfe/trunk/test/Sema/transpose-memset.c
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=337470&r1=337469&r2=337470&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Thu
>> Jul 19 09:46:15 2018
>> @@ -443,6 +443,13 @@ def : DiagGroup<"synth">;
>> def SizeofArrayArgument :
>> DiagGroup<"sizeof-array-argument">;
>> def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
>> def SizeofPointerMemaccess :
>> DiagGroup<"sizeof-pointer-memaccess">;
>> +def MemsetTransposedArgs :
>> DiagGroup<"memset-transposed-args">;
>> +def DynamicClassMemaccess :
>> DiagGroup<"dynamic-class-memaccess">;
>> +def NonTrivialMemaccess : DiagGroup<"nontrivial-memaccess">;
>> +def SuspiciousBzero : DiagGroup<"suspicious-bzero">;
>> +def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
>> + [SizeofPointerMemaccess, DynamicClassMemaccess,
>> + NonTrivialMemaccess, MemsetTransposedArgs,
>> SuspiciousBzero]>;
>> def StaticInInline : DiagGroup<"static-in-inline">;
>> def StaticLocalInInline :
>> DiagGroup<"static-local-in-inline">;
>> def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
>>
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=337470&r1=337469&r2=337470&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> Thu Jul 19 09:46:15 2018
>> @@ -619,14 +619,14 @@ def warn_cstruct_memaccess : Warning<
>> "%select{destination for|source of|first operand
>> of|second operand of}0 this "
>> "%1 call is a pointer to record %2 that is not trivial
>> to "
>> "%select{primitive-default-initialize|primitive-copy}3">,
>> - InGroup<DiagGroup<"nontrivial-memaccess">>;
>> + InGroup<NonTrivialMemaccess>;
>> def note_nontrivial_field : Note<
>> "field is non-trivial to
>> %select{copy|default-initialize}0">;
>> def warn_dyn_class_memaccess : Warning<
>> "%select{destination for|source of|first operand
>> of|second operand of}0 this "
>> "%1 call is a pointer to %select{|class containing a
>> }2dynamic class %3; "
>> "vtable pointer will be
>> %select{overwritten|copied|moved|compared}4">,
>> - InGroup<DiagGroup<"dynamic-class-memaccess">>;
>> + InGroup<DynamicClassMemaccess>;
>> def note_bad_memaccess_silence : Note<
>> "explicitly cast the pointer to silence this warning">;
>> def warn_sizeof_pointer_expr_memaccess : Warning<
>> @@ -655,7 +655,19 @@ def note_memsize_comparison_paren : Note
>> "did you mean to compare the result of %0 instead?">;
>> def note_memsize_comparison_cast_silence : Note<
>> "explicitly cast the argument to size_t to silence
>> this warning">;
>> -
>> +def warn_suspicious_sizeof_memset : Warning<
>> + "%select{'size' argument to memset is '0'|"
>> + "setting buffer to a 'sizeof' expression}0"
>> + "; did you mean to transpose the last two arguments?">,
>> + InGroup<MemsetTransposedArgs>;
>> +def note_suspicious_sizeof_memset_silence : Note<
>> + "%select{parenthesize the third argument|"
>> + "cast the second argument to 'int'}0 to silence">;
>> +def warn_suspicious_bzero_size : Warning<"'size'
>> argument to bzero is '0'">,
>> + InGroup<SuspiciousBzero>;
>> +def note_suspicious_bzero_size_silence : Note<
>> + "parenthesize the second argument to silence">;
>> +
>> 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>;
>>
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=337470&r1=337469&r2=337470&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Jul 19
>> 09:46:15 2018
>> @@ -8629,24 +8629,26 @@ static const CXXRecordDecl
>> *getContained
>> return nullptr;
>> }
>>
>> +static const UnaryExprOrTypeTraitExpr
>> *getAsSizeOfExpr(const Expr *E) {
>> + if (const auto *Unary =
>> dyn_cast<UnaryExprOrTypeTraitExpr>(E))
>> + if (Unary->getKind() == UETT_SizeOf)
>> + return Unary;
>> + return nullptr;
>> +}
>> +
>> /// If E is a sizeof expression, returns its argument
>> expression,
>> /// otherwise returns NULL.
>> static const Expr *getSizeOfExprArg(const Expr *E) {
>> - if (const UnaryExprOrTypeTraitExpr *SizeOf =
>> - dyn_cast<UnaryExprOrTypeTraitExpr>(E))
>> - if (SizeOf->getKind() == UETT_SizeOf &&
>> !SizeOf->isArgumentType())
>> + if (const UnaryExprOrTypeTraitExpr *SizeOf =
>> getAsSizeOfExpr(E))
>> + if (!SizeOf->isArgumentType())
>> return
>> SizeOf->getArgumentExpr()->IgnoreParenImpCasts();
>> -
>> return nullptr;
>> }
>>
>> /// If E is a sizeof expression, returns its argument type.
>> static QualType getSizeOfArgType(const Expr *E) {
>> - if (const UnaryExprOrTypeTraitExpr *SizeOf =
>> - dyn_cast<UnaryExprOrTypeTraitExpr>(E))
>> - if (SizeOf->getKind() == UETT_SizeOf)
>> - return SizeOf->getTypeOfArgument();
>> -
>> + if (const UnaryExprOrTypeTraitExpr *SizeOf =
>> getAsSizeOfExpr(E))
>> + return SizeOf->getTypeOfArgument();
>> return QualType();
>> }
>>
>> @@ -8742,6 +8744,86 @@ struct SearchNonTrivialToCopyField
>>
>> }
>>
>> +/// Detect if \c SizeofExpr is likely to calculate the
>> sizeof an object.
>> +static bool doesExprLikelyComputeSize(const Expr
>> *SizeofExpr) {
>> + SizeofExpr = SizeofExpr->IgnoreParenImpCasts();
>> +
>> + if (const auto *BO =
>> dyn_cast<BinaryOperator>(SizeofExpr)) {
>> + if (BO->getOpcode() != BO_Mul && BO->getOpcode() !=
>> BO_Add)
>> + return false;
>> +
>> + return doesExprLikelyComputeSize(BO->getLHS()) ||
>> + doesExprLikelyComputeSize(BO->getRHS());
>> + }
>> +
>> + return getAsSizeOfExpr(SizeofExpr) != nullptr;
>> +}
>> +
>> +/// Check if the ArgLoc originated from a macro passed
>> to the call at CallLoc.
>> +///
>> +/// \code
>> +/// #define MACRO 0
>> +/// foo(MACRO);
>> +/// foo(0);
>> +/// \endcode
>> +///
>> +/// This should return true for the first call to foo,
>> but not for the second
>> +/// (regardless of whether foo is a macro or function).
>> +static bool isArgumentExpandedFromMacro(SourceManager &SM,
>> + SourceLocation CallLoc,
>> + SourceLocation ArgLoc) {
>> + if (!CallLoc.isMacroID())
>> + return SM.getFileID(CallLoc) != SM.getFileID(ArgLoc);
>> +
>> + return
>> SM.getFileID(SM.getImmediateMacroCallerLoc(CallLoc)) !=
>> + SM.getFileID(SM.getImmediateMacroCallerLoc(ArgLoc));
>> +}
>> +
>> +/// Diagnose cases like 'memset(buf, sizeof(buf), 0)',
>> which should have the
>> +/// last two arguments transposed.
>> +static void CheckMemaccessSize(Sema &S, unsigned BId,
>> const CallExpr *Call) {
>> + if (BId != Builtin::BImemset && BId != Builtin::BIbzero)
>> + return;
>> +
>> + const Expr *SizeArg =
>> + Call->getArg(BId == Builtin::BImemset ? 2 :
>> 1)->IgnoreImpCasts();
>> +
>> + // If we're memsetting or bzeroing 0 bytes, then this
>> is likely an error.
>> + SourceLocation CallLoc = Call->getRParenLoc();
>> + SourceManager &SM = S.getSourceManager();
>> + if (isa<IntegerLiteral>(SizeArg) &&
>> + cast<IntegerLiteral>(SizeArg)->getValue() == 0 &&
>> + !isArgumentExpandedFromMacro(SM, CallLoc,
>> SizeArg->getExprLoc())) {
>> +
>> + SourceLocation DiagLoc = SizeArg->getExprLoc();
>> +
>> + // Some platforms #define bzero to __builtin_memset.
>> See if this is the
>> + // case, and if so, emit a better diagnostic.
>> + if (BId == Builtin::BIbzero ||
>> + (CallLoc.isMacroID() &&
>> Lexer::getImmediateMacroName(
>> + CallLoc, SM,
>> S.getLangOpts()) == "bzero")) {
>> + S.Diag(DiagLoc, diag::warn_suspicious_bzero_size);
>> + S.Diag(DiagLoc,
>> diag::note_suspicious_bzero_size_silence);
>> + } else {
>> + S.Diag(DiagLoc,
>> diag::warn_suspicious_sizeof_memset) << 0;
>> + S.Diag(DiagLoc,
>> diag::note_suspicious_sizeof_memset_silence) << 0;
>> + }
>> + return;
>> + }
>> +
>> + // If the second argument to a memset is a sizeof
>> expression and the third
>> + // isn't, this is also likely an error. This should catch
>> + // 'memset(buf, sizeof(buf), 0xff)'.
>> + if (BId == Builtin::BImemset &&
>> + doesExprLikelyComputeSize(Call->getArg(1)) &&
>> + !doesExprLikelyComputeSize(Call->getArg(2))) {
>> + SourceLocation DiagLoc = Call->getArg(1)->getExprLoc();
>> + S.Diag(DiagLoc, diag::warn_suspicious_sizeof_memset)
>> << 1;
>> + S.Diag(DiagLoc,
>> diag::note_suspicious_sizeof_memset_silence) << 1;
>> + return;
>> + }
>> +}
>> +
>> /// Check for dangerous or invalid arguments to memset().
>> ///
>> /// This issues warnings on known problematic, dangerous
>> or unspecified
>> @@ -8771,6 +8853,9 @@ void
>> Sema::CheckMemaccessArguments(const
>> Call->getLocStart(), Call->getRParenLoc()))
>> return;
>>
>> + // Catch cases like 'memset(buf, sizeof(buf), 0)'.
>> + CheckMemaccessSize(*this, BId, Call);
>> +
>> // We have special checking when the length is a
>> sizeof expression.
>> QualType SizeOfArgTy = getSizeOfArgType(LenExpr);
>> const Expr *SizeOfArg = getSizeOfExprArg(LenExpr);
>>
>> Added: cfe/trunk/test/Sema/transpose-memset.c
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/transpose-memset.c?rev=337470&view=auto
>> ==============================================================================
>> --- cfe/trunk/test/Sema/transpose-memset.c (added)
>> +++ cfe/trunk/test/Sema/transpose-memset.c Thu Jul 19
>> 09:46:15 2018
>> @@ -0,0 +1,60 @@
>> +// RUN: %clang_cc1 -Wmemset-transposed-args -verify %s
>> +// RUN: %clang_cc1 -xc++ -Wmemset-transposed-args -verify %s
>> +
>> +#define memset(...) __builtin_memset(__VA_ARGS__)
>> +#define bzero(x,y) __builtin_memset(x, 0, y)
>> +#define real_bzero(x,y) __builtin_bzero(x,y)
>> +
>> +int array[10];
>> +int *ptr;
>> +
>> +int main() {
>> + memset(array, sizeof(array), 0); //
>> expected-warning{{'size' argument to memset is '0'; did
>> you mean to transpose the last two arguments?}}
>> expected-note{{parenthesize the third argument to silence}}
>> + memset(array, sizeof(array), 0xff); //
>> expected-warning{{setting buffer to a 'sizeof'
>> expression; did you mean to transpose the last two
>> arguments?}} expected-note{{cast the second argument to
>> 'int' to silence}}
>> + memset(ptr, sizeof(ptr), 0); //
>> expected-warning{{'size' argument to memset is '0'; did
>> you mean to transpose the last two arguments?}}
>> expected-note{{parenthesize the third argument to silence}}
>> + memset(ptr, sizeof(*ptr) * 10, 1); //
>> expected-warning{{setting buffer to a 'sizeof'
>> expression; did you mean to transpose the last two
>> arguments?}} expected-note{{cast the second argument to
>> 'int' to silence}}
>> + memset(ptr, 10 * sizeof(int *), 1); //
>> expected-warning{{setting buffer to a 'sizeof'
>> expression; did you mean to transpose the last two
>> arguments?}} expected-note{{cast the second argument to
>> 'int' to silence}}
>> + memset(ptr, 10 * sizeof(int *) + 10, 0xff); //
>> expected-warning{{setting buffer to a 'sizeof'
>> expression; did you mean to transpose the last two
>> arguments?}} expected-note{{cast the second argument to
>> 'int' to silence}}
>> + memset(ptr, sizeof(char) * sizeof(int *), 0xff); //
>> expected-warning{{setting buffer to a 'sizeof'
>> expression; did you mean to transpose the last two
>> arguments?}} expected-note{{cast the second argument to
>> 'int' to silence}}
>> + memset(array, sizeof(array), sizeof(array)); // Uh...
>> fine I guess.
>> + memset(array, 0, sizeof(array));
>> + memset(ptr, 0, sizeof(int *) * 10);
>> + memset(array, (int)sizeof(array), (0)); // no warning
>> + memset(array, (int)sizeof(array), 32); // no warning
>> + memset(array, 32, (0)); // no warning
>> +
>> + bzero(ptr, 0); // expected-warning{{'size' argument to
>> bzero is '0'}} expected-note{{parenthesize the second
>> argument to silence}}
>> + real_bzero(ptr, 0); // expected-warning{{'size'
>> argument to bzero is '0'}} expected-note{{parenthesize
>> the second argument to silence}}
>> +}
>> +
>> +void macros() {
>> +#define ZERO 0
>> + int array[10];
>> + memset(array, 0xff, ZERO); // no warning
>> + // Still emit a diagnostic for memsetting a sizeof
>> expression:
>> + memset(array, sizeof(array), ZERO); //
>> expected-warning{{'sizeof'}} expected-note{{cast}}
>> + bzero(array, ZERO); // no warning
>> + real_bzero(array, ZERO); // no warning
>> +#define NESTED_DONT_DIAG \
>> + memset(array, 0xff, ZERO); \
>> + real_bzero(array, ZERO);
>> +
>> + NESTED_DONT_DIAG;
>> +
>> +#define NESTED_DO_DIAG \
>> + memset(array, 0xff, 0); \
>> + real_bzero(array, 0)
>> +
>> + NESTED_DO_DIAG; // expected-warning{{'size' argument
>> to memset}} expected-warning{{'size' argument to bzero}}
>> expected-note2{{parenthesize}}
>> +
>> +#define FN_MACRO(p) \
>> + memset(array, 0xff, p)
>> +
>> + FN_MACRO(ZERO);
>> + FN_MACRO(0); // FIXME: should we diagnose this?
>> +
>> + __builtin_memset(array, 0, ZERO); // no warning
>> + __builtin_bzero(array, ZERO);
>> + __builtin_memset(array, 0, 0); //
>> expected-warning{{'size' argument to memset}} //
>> expected-note{{parenthesize}}
>> + __builtin_bzero(array, 0); // expected-warning{{'size'
>> argument to bzero}} // expected-note{{parenthesize}}
>> +}
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> <mailto:cfe-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180726/0f804d9a/attachment-0001.html>
More information about the cfe-commits
mailing list