Skip to content

Commit d1cf276

Browse files
committedJul 19, 2018
[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 llvm-svn: 337470
1 parent b6022aa commit d1cf276

File tree

4 files changed

+176
-12
lines changed

4 files changed

+176
-12
lines changed
 

‎clang/include/clang/Basic/DiagnosticGroups.td

+7
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,13 @@ def : DiagGroup<"synth">;
443443
def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;
444444
def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
445445
def SizeofPointerMemaccess : DiagGroup<"sizeof-pointer-memaccess">;
446+
def MemsetTransposedArgs : DiagGroup<"memset-transposed-args">;
447+
def DynamicClassMemaccess : DiagGroup<"dynamic-class-memaccess">;
448+
def NonTrivialMemaccess : DiagGroup<"nontrivial-memaccess">;
449+
def SuspiciousBzero : DiagGroup<"suspicious-bzero">;
450+
def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
451+
[SizeofPointerMemaccess, DynamicClassMemaccess,
452+
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
446453
def StaticInInline : DiagGroup<"static-in-inline">;
447454
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
448455
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;

‎clang/include/clang/Basic/DiagnosticSemaKinds.td

+15-3
Original file line numberDiff line numberDiff line change
@@ -619,14 +619,14 @@ def warn_cstruct_memaccess : Warning<
619619
"%select{destination for|source of|first operand of|second operand of}0 this "
620620
"%1 call is a pointer to record %2 that is not trivial to "
621621
"%select{primitive-default-initialize|primitive-copy}3">,
622-
InGroup<DiagGroup<"nontrivial-memaccess">>;
622+
InGroup<NonTrivialMemaccess>;
623623
def note_nontrivial_field : Note<
624624
"field is non-trivial to %select{copy|default-initialize}0">;
625625
def warn_dyn_class_memaccess : Warning<
626626
"%select{destination for|source of|first operand of|second operand of}0 this "
627627
"%1 call is a pointer to %select{|class containing a }2dynamic class %3; "
628628
"vtable pointer will be %select{overwritten|copied|moved|compared}4">,
629-
InGroup<DiagGroup<"dynamic-class-memaccess">>;
629+
InGroup<DynamicClassMemaccess>;
630630
def note_bad_memaccess_silence : Note<
631631
"explicitly cast the pointer to silence this warning">;
632632
def warn_sizeof_pointer_expr_memaccess : Warning<
@@ -655,7 +655,19 @@ def note_memsize_comparison_paren : Note<
655655
"did you mean to compare the result of %0 instead?">;
656656
def note_memsize_comparison_cast_silence : Note<
657657
"explicitly cast the argument to size_t to silence this warning">;
658-
658+
def warn_suspicious_sizeof_memset : Warning<
659+
"%select{'size' argument to memset is '0'|"
660+
"setting buffer to a 'sizeof' expression}0"
661+
"; did you mean to transpose the last two arguments?">,
662+
InGroup<MemsetTransposedArgs>;
663+
def note_suspicious_sizeof_memset_silence : Note<
664+
"%select{parenthesize the third argument|"
665+
"cast the second argument to 'int'}0 to silence">;
666+
def warn_suspicious_bzero_size : Warning<"'size' argument to bzero is '0'">,
667+
InGroup<SuspiciousBzero>;
668+
def note_suspicious_bzero_size_silence : Note<
669+
"parenthesize the second argument to silence">;
670+
659671
def warn_strncat_large_size : Warning<
660672
"the value of the size argument in 'strncat' is too large, might lead to a "
661673
"buffer overflow">, InGroup<StrncatSize>;

‎clang/lib/Sema/SemaChecking.cpp

+94-9
Original file line numberDiff line numberDiff line change
@@ -8629,24 +8629,26 @@ static const CXXRecordDecl *getContainedDynamicClass(QualType T,
86298629
return nullptr;
86308630
}
86318631

8632+
static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) {
8633+
if (const auto *Unary = dyn_cast<UnaryExprOrTypeTraitExpr>(E))
8634+
if (Unary->getKind() == UETT_SizeOf)
8635+
return Unary;
8636+
return nullptr;
8637+
}
8638+
86328639
/// If E is a sizeof expression, returns its argument expression,
86338640
/// otherwise returns NULL.
86348641
static const Expr *getSizeOfExprArg(const Expr *E) {
8635-
if (const UnaryExprOrTypeTraitExpr *SizeOf =
8636-
dyn_cast<UnaryExprOrTypeTraitExpr>(E))
8637-
if (SizeOf->getKind() == UETT_SizeOf && !SizeOf->isArgumentType())
8642+
if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E))
8643+
if (!SizeOf->isArgumentType())
86388644
return SizeOf->getArgumentExpr()->IgnoreParenImpCasts();
8639-
86408645
return nullptr;
86418646
}
86428647

86438648
/// If E is a sizeof expression, returns its argument type.
86448649
static QualType getSizeOfArgType(const Expr *E) {
8645-
if (const UnaryExprOrTypeTraitExpr *SizeOf =
8646-
dyn_cast<UnaryExprOrTypeTraitExpr>(E))
8647-
if (SizeOf->getKind() == UETT_SizeOf)
8648-
return SizeOf->getTypeOfArgument();
8649-
8650+
if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E))
8651+
return SizeOf->getTypeOfArgument();
86508652
return QualType();
86518653
}
86528654

@@ -8742,6 +8744,86 @@ struct SearchNonTrivialToCopyField
87428744

87438745
}
87448746

8747+
/// Detect if \c SizeofExpr is likely to calculate the sizeof an object.
8748+
static bool doesExprLikelyComputeSize(const Expr *SizeofExpr) {
8749+
SizeofExpr = SizeofExpr->IgnoreParenImpCasts();
8750+
8751+
if (const auto *BO = dyn_cast<BinaryOperator>(SizeofExpr)) {
8752+
if (BO->getOpcode() != BO_Mul && BO->getOpcode() != BO_Add)
8753+
return false;
8754+
8755+
return doesExprLikelyComputeSize(BO->getLHS()) ||
8756+
doesExprLikelyComputeSize(BO->getRHS());
8757+
}
8758+
8759+
return getAsSizeOfExpr(SizeofExpr) != nullptr;
8760+
}
8761+
8762+
/// Check if the ArgLoc originated from a macro passed to the call at CallLoc.
8763+
///
8764+
/// \code
8765+
/// #define MACRO 0
8766+
/// foo(MACRO);
8767+
/// foo(0);
8768+
/// \endcode
8769+
///
8770+
/// This should return true for the first call to foo, but not for the second
8771+
/// (regardless of whether foo is a macro or function).
8772+
static bool isArgumentExpandedFromMacro(SourceManager &SM,
8773+
SourceLocation CallLoc,
8774+
SourceLocation ArgLoc) {
8775+
if (!CallLoc.isMacroID())
8776+
return SM.getFileID(CallLoc) != SM.getFileID(ArgLoc);
8777+
8778+
return SM.getFileID(SM.getImmediateMacroCallerLoc(CallLoc)) !=
8779+
SM.getFileID(SM.getImmediateMacroCallerLoc(ArgLoc));
8780+
}
8781+
8782+
/// Diagnose cases like 'memset(buf, sizeof(buf), 0)', which should have the
8783+
/// last two arguments transposed.
8784+
static void CheckMemaccessSize(Sema &S, unsigned BId, const CallExpr *Call) {
8785+
if (BId != Builtin::BImemset && BId != Builtin::BIbzero)
8786+
return;
8787+
8788+
const Expr *SizeArg =
8789+
Call->getArg(BId == Builtin::BImemset ? 2 : 1)->IgnoreImpCasts();
8790+
8791+
// If we're memsetting or bzeroing 0 bytes, then this is likely an error.
8792+
SourceLocation CallLoc = Call->getRParenLoc();
8793+
SourceManager &SM = S.getSourceManager();
8794+
if (isa<IntegerLiteral>(SizeArg) &&
8795+
cast<IntegerLiteral>(SizeArg)->getValue() == 0 &&
8796+
!isArgumentExpandedFromMacro(SM, CallLoc, SizeArg->getExprLoc())) {
8797+
8798+
SourceLocation DiagLoc = SizeArg->getExprLoc();
8799+
8800+
// Some platforms #define bzero to __builtin_memset. See if this is the
8801+
// case, and if so, emit a better diagnostic.
8802+
if (BId == Builtin::BIbzero ||
8803+
(CallLoc.isMacroID() && Lexer::getImmediateMacroName(
8804+
CallLoc, SM, S.getLangOpts()) == "bzero")) {
8805+
S.Diag(DiagLoc, diag::warn_suspicious_bzero_size);
8806+
S.Diag(DiagLoc, diag::note_suspicious_bzero_size_silence);
8807+
} else {
8808+
S.Diag(DiagLoc, diag::warn_suspicious_sizeof_memset) << 0;
8809+
S.Diag(DiagLoc, diag::note_suspicious_sizeof_memset_silence) << 0;
8810+
}
8811+
return;
8812+
}
8813+
8814+
// If the second argument to a memset is a sizeof expression and the third
8815+
// isn't, this is also likely an error. This should catch
8816+
// 'memset(buf, sizeof(buf), 0xff)'.
8817+
if (BId == Builtin::BImemset &&
8818+
doesExprLikelyComputeSize(Call->getArg(1)) &&
8819+
!doesExprLikelyComputeSize(Call->getArg(2))) {
8820+
SourceLocation DiagLoc = Call->getArg(1)->getExprLoc();
8821+
S.Diag(DiagLoc, diag::warn_suspicious_sizeof_memset) << 1;
8822+
S.Diag(DiagLoc, diag::note_suspicious_sizeof_memset_silence) << 1;
8823+
return;
8824+
}
8825+
}
8826+
87458827
/// Check for dangerous or invalid arguments to memset().
87468828
///
87478829
/// This issues warnings on known problematic, dangerous or unspecified
@@ -8771,6 +8853,9 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
87718853
Call->getLocStart(), Call->getRParenLoc()))
87728854
return;
87738855

8856+
// Catch cases like 'memset(buf, sizeof(buf), 0)'.
8857+
CheckMemaccessSize(*this, BId, Call);
8858+
87748859
// We have special checking when the length is a sizeof expression.
87758860
QualType SizeOfArgTy = getSizeOfArgType(LenExpr);
87768861
const Expr *SizeOfArg = getSizeOfExprArg(LenExpr);

‎clang/test/Sema/transpose-memset.c

+60
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
// RUN: %clang_cc1 -Wmemset-transposed-args -verify %s
2+
// RUN: %clang_cc1 -xc++ -Wmemset-transposed-args -verify %s
3+
4+
#define memset(...) __builtin_memset(__VA_ARGS__)
5+
#define bzero(x,y) __builtin_memset(x, 0, y)
6+
#define real_bzero(x,y) __builtin_bzero(x,y)
7+
8+
int array[10];
9+
int *ptr;
10+
11+
int main() {
12+
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}}
13+
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}}
14+
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}}
15+
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}}
16+
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}}
17+
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}}
18+
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}}
19+
memset(array, sizeof(array), sizeof(array)); // Uh... fine I guess.
20+
memset(array, 0, sizeof(array));
21+
memset(ptr, 0, sizeof(int *) * 10);
22+
memset(array, (int)sizeof(array), (0)); // no warning
23+
memset(array, (int)sizeof(array), 32); // no warning
24+
memset(array, 32, (0)); // no warning
25+
26+
bzero(ptr, 0); // expected-warning{{'size' argument to bzero is '0'}} expected-note{{parenthesize the second argument to silence}}
27+
real_bzero(ptr, 0); // expected-warning{{'size' argument to bzero is '0'}} expected-note{{parenthesize the second argument to silence}}
28+
}
29+
30+
void macros() {
31+
#define ZERO 0
32+
int array[10];
33+
memset(array, 0xff, ZERO); // no warning
34+
// Still emit a diagnostic for memsetting a sizeof expression:
35+
memset(array, sizeof(array), ZERO); // expected-warning{{'sizeof'}} expected-note{{cast}}
36+
bzero(array, ZERO); // no warning
37+
real_bzero(array, ZERO); // no warning
38+
#define NESTED_DONT_DIAG \
39+
memset(array, 0xff, ZERO); \
40+
real_bzero(array, ZERO);
41+
42+
NESTED_DONT_DIAG;
43+
44+
#define NESTED_DO_DIAG \
45+
memset(array, 0xff, 0); \
46+
real_bzero(array, 0)
47+
48+
NESTED_DO_DIAG; // expected-warning{{'size' argument to memset}} expected-warning{{'size' argument to bzero}} expected-note2{{parenthesize}}
49+
50+
#define FN_MACRO(p) \
51+
memset(array, 0xff, p)
52+
53+
FN_MACRO(ZERO);
54+
FN_MACRO(0); // FIXME: should we diagnose this?
55+
56+
__builtin_memset(array, 0, ZERO); // no warning
57+
__builtin_bzero(array, ZERO);
58+
__builtin_memset(array, 0, 0); // expected-warning{{'size' argument to memset}} // expected-note{{parenthesize}}
59+
__builtin_bzero(array, 0); // expected-warning{{'size' argument to bzero}} // expected-note{{parenthesize}}
60+
}

0 commit comments

Comments
 (0)
Please sign in to comment.