Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,9 @@ Improvements to Clang's diagnostics
comparison operators when mixed with bitwise operators in enum value initializers.
This can be locally disabled by explicitly casting the initializer value.

- Clang now emits ``-Wsizeof-pointer-memaccess`` when snprintf/vsnprintf use the sizeof
the destination buffer(dynamically allocated) in the len parameter(#GH162366)

Improvements to Clang's time-trace
----------------------------------

Expand Down
4 changes: 4 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -895,6 +895,10 @@ def warn_sizeof_pointer_type_memaccess : Warning<
"argument to 'sizeof' in %0 call is the same pointer type %1 as the "
"%select{destination|source}2; expected %3 or an explicit length">,
InGroup<SizeofPointerMemaccess>;
def warn_sizeof_pointer_dest_type_memacess : Warning<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should reuse warn_sizeof_pointer_type_memaccess and the associated note.

"argument to 'sizeof' in %0 call is the same expression as the "
"destination; did you mean to put 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">,
Expand Down
73 changes: 42 additions & 31 deletions clang/lib/Sema/SemaChecking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1139,6 +1139,37 @@ static bool ProcessFormatStringLiteral(const Expr *FormatExpr,
return false;
}

static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid this large code movement, we could forward declare these functions instead, but I think a better approach be to move the actual diagnosis logic into a separate function.

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 = 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 = getAsSizeOfExpr(E))
return SizeOf->getTypeOfArgument();
return QualType();
}

/// Check if two expressions refer to the same declaration.
static bool referToTheSameDecl(const Expr *E1, const Expr *E2) {
if (const DeclRefExpr *D1 = dyn_cast_or_null<DeclRefExpr>(E1))
if (const DeclRefExpr *D2 = dyn_cast_or_null<DeclRefExpr>(E2))
return D1->getDecl() == D2->getDecl();
return false;
}

void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
CallExpr *TheCall) {
if (TheCall->isValueDependent() || TheCall->isTypeDependent() ||
Expand Down Expand Up @@ -1449,6 +1480,17 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
}
}
DestinationSize = ComputeSizeArgument(0);
const Expr *SizeOfArg = TheCall->getArg(1)->IgnoreParenImpCasts();
const Expr *Dest = TheCall->getArg(0)->IgnoreParenImpCasts();
const Expr *SizeOfArgExpr = getSizeOfExprArg(SizeOfArg);
const QualType SizeOfArgType = getSizeOfArgType(SizeOfArg);
const Type *ExprType = SizeOfArgType.getTypePtrOrNull();
if (ExprType && ExprType->isPointerType() &&
referToTheSameDecl(SizeOfArgExpr, Dest)) {
DiagRuntimeBehavior(SizeOfArg->getExprLoc(), Dest,
PDiag(diag::warn_sizeof_pointer_dest_type_memacess)
<< FD->getNameInfo().getName());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than a reimplementation of similar semantics we should consider refactoring the existing checks so they can be used here instead.

}
}

Expand Down Expand Up @@ -9979,29 +10021,6 @@ static const CXXRecordDecl *getContainedDynamicClass(QualType T,
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 = 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 = getAsSizeOfExpr(E))
return SizeOf->getTypeOfArgument();
return QualType();
}

namespace {

struct SearchNonTrivialToInitializeField
Expand Down Expand Up @@ -10499,14 +10518,6 @@ void Sema::CheckStrlcpycatArguments(const CallExpr *Call,
OS.str());
}

/// Check if two expressions refer to the same declaration.
static bool referToTheSameDecl(const Expr *E1, const Expr *E2) {
if (const DeclRefExpr *D1 = dyn_cast_or_null<DeclRefExpr>(E1))
if (const DeclRefExpr *D2 = dyn_cast_or_null<DeclRefExpr>(E2))
return D1->getDecl() == D2->getDecl();
return false;
}

static const Expr *getStrlenExprArg(const Expr *E) {
if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
const FunctionDecl *FD = CE->getDirectCallee();
Expand Down
9 changes: 9 additions & 0 deletions clang/test/SemaCXX/warn-memset-bad-sizeof.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -188,3 +188,12 @@ void strcpy_and_friends() {
strndup(FOO, sizeof(FOO)); // \
// expected-warning {{'strndup' call operates on objects of type 'const char' while the size is based on a different type 'const char *'}} expected-note{{did you mean to provide an explicit length?}}
}

extern "C" int snprintf(char* buffer, __SIZE_TYPE__ buf_size, const char* format, ...);
extern "C" void* malloc(unsigned size);

void check_prints(){
char* a = (char*) malloc(20);
const char* b = "Hello World";
snprintf(a, sizeof(a), "%s", b); // expected-warning{{argument to 'sizeof' in 'snprintf' call is the same expression as the destination; did you mean to put an explicit length?}}
}
Loading