-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang-tidy] Add check 'bugprone-cast-to-struct' #153428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
95634ba
f2eb00d
43153e3
c14fce8
b13b366
69c37fd
dc8bc27
274fc45
72a38e4
1703f1a
3229bff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
//===--- CastToStructCheck.cpp - clang-tidy -------------------------------===// | ||
// | ||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
// See https://llvm.org/LICENSE.txt for license information. | ||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#include "CastToStructCheck.h" | ||
#include "../utils/Matchers.h" | ||
#include "../utils/OptionsUtils.h" | ||
#include "clang/ASTMatchers/ASTMatchFinder.h" | ||
|
||
using namespace clang::ast_matchers; | ||
|
||
namespace clang::tidy::bugprone { | ||
|
||
CastToStructCheck::CastToStructCheck(StringRef Name, ClangTidyContext *Context) | ||
: ClangTidyCheck(Name, Context), | ||
IgnoredFunctions( | ||
utils::options::parseStringList(Options.get("IgnoredFunctions", ""))), | ||
IgnoredFromTypes( | ||
utils::options::parseStringList(Options.get("IgnoredFromTypes", ""))), | ||
IgnoredToTypes( | ||
utils::options::parseStringList(Options.get("IgnoredToTypes", ""))) {} | ||
|
||
void CastToStructCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { | ||
Options.store(Opts, "IgnoredFunctions", | ||
utils::options::serializeStringList(IgnoredFunctions)); | ||
Options.store(Opts, "IgnoredFromTypes", | ||
utils::options::serializeStringList(IgnoredFromTypes)); | ||
Options.store(Opts, "IgnoredToTypes", | ||
utils::options::serializeStringList(IgnoredToTypes)); | ||
} | ||
|
||
void CastToStructCheck::registerMatchers(MatchFinder *Finder) { | ||
auto FromPointee = | ||
qualType(hasUnqualifiedDesugaredType(type().bind("FromType")), | ||
unless(qualType(matchers::matchesAnyListedTypeName( | ||
IgnoredFromTypes, false)))) | ||
.bind("FromPointee"); | ||
auto ToPointee = | ||
qualType(hasUnqualifiedDesugaredType(recordType().bind("ToType")), | ||
unless(qualType( | ||
matchers::matchesAnyListedTypeName(IgnoredToTypes, false)))) | ||
.bind("ToPointee"); | ||
auto FromPtrType = qualType(pointsTo(FromPointee)).bind("FromPtr"); | ||
auto ToPtrType = qualType(pointsTo(ToPointee)).bind("ToPtr"); | ||
Finder->addMatcher( | ||
cStyleCastExpr(hasSourceExpression(hasType(FromPtrType)), | ||
hasType(ToPtrType), | ||
unless(hasAncestor(functionDecl( | ||
matchers::matchesAnyListedName(IgnoredFunctions))))) | ||
.bind("CastExpr"), | ||
this); | ||
} | ||
|
||
void CastToStructCheck::check(const MatchFinder::MatchResult &Result) { | ||
const auto *const FoundCastExpr = | ||
Result.Nodes.getNodeAs<CStyleCastExpr>("CastExpr"); | ||
const auto *const FromPtr = Result.Nodes.getNodeAs<QualType>("FromPtr"); | ||
const auto *const ToPtr = Result.Nodes.getNodeAs<QualType>("ToPtr"); | ||
const auto *const FromPointee = | ||
Result.Nodes.getNodeAs<QualType>("FromPointee"); | ||
const auto *const ToPointee = Result.Nodes.getNodeAs<QualType>("ToPointee"); | ||
const auto *const FromType = Result.Nodes.getNodeAs<Type>("FromType"); | ||
const auto *const ToType = Result.Nodes.getNodeAs<RecordType>("ToType"); | ||
if (!FromPointee || !ToPointee) | ||
return; | ||
|
||
if (FromType->isVoidType() || FromType->isUnionType() || | ||
ToType->isUnionType()) | ||
|
||
return; | ||
if (FromType == ToType) | ||
return; | ||
diag(FoundCastExpr->getExprLoc(), | ||
"casting a %0 pointer to a " | ||
"%1 pointer and accessing a field can lead to memory " | ||
NagyDonat marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
"access errors or data corruption") | ||
<< *FromPtr << *ToPtr; | ||
} | ||
|
||
} // namespace clang::tidy::bugprone |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,39 @@ | ||||||
//===--- CastToStructCheck.h - clang-tidy -----------------------*- C++ -*-===// | ||||||
|
//===--- CastToStructCheck.h - clang-tidy -----------------------*- C++ -*-===// | |
//===----------------------------------------------------------------------===// |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/cast-to-struct.html | |
/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/cast-to-struct.html |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should also be active in C++ (and it should include C++ tests using C-style cast and reinterpret_cast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in C++ the problem can be handled in different way. C-style cast is discouraged in C++, so all C style casts should be converted to other kind of cast (this is job of another checker, probably it exists already). The reinterpret_cast
indicates already by syntax that it is a potentially dangerous operation and many uses of reinterpret_cast
would be likely found by the checker (if it checks for it in similar way), so a checker is not much more than find all uses of reinterpret_cast
. It is possible to check exact data layout but probably still many false positives would appear. Additionally in C++ the casting rules are more complicated at least because inheritance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we then add "Limitations" that this check does not run on C++ code. Similar to how https://clang.llvm.org/extra/clang-tidy/checks/misc/const-correctness.html states that it does not run on C code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to limit to C, probably not bad idea would be to check if Cplusplus flag is not set.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -112,6 +112,11 @@ Improvements to clang-tidy | |||||
New checks | ||||||
^^^^^^^^^^ | ||||||
|
||||||
- New :doc:`bugprone-cast-to-struct | ||||||
<clang-tidy/checks/bugprone/cast-to-struct>` check. | ||||||
|
||||||
Finds casts from pointers to struct or scalar type to pointers to struct type. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I do not want to use the word "struct" as a C keyword, instead to describe the type created with |
||||||
|
||||||
- New :doc:`bugprone-invalid-enum-default-initialization | ||||||
<clang-tidy/checks/bugprone/invalid-enum-default-initialization>` check. | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,54 @@ | ||||||
.. title:: clang-tidy - bugprone-cast-to-struct | ||||||
|
||||||
bugprone-cast-to-struct | ||||||
======================= | ||||||
|
||||||
Finds casts from pointers to struct or scalar type to pointers to struct type. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
Casts between pointers to different structs can be unsafe because it is possible | ||||||
to access uninitialized or undefined data after the cast. There may be issues | ||||||
|
||||||
with type compatibility or data alignment. Cast from a pointer to a scalar type | ||||||
(which points often to an array or memory block) to a `struct` type pointer can | ||||||
|
||||||
be unsafe for similar reasons. This check warns at casts from any non-`struct` | ||||||
type to a `struct` type. No warning is produced at cast from type `void *` (this | ||||||
is the usual way of allocating memory with `malloc`-like functions). It is | ||||||
possible to specify additional types to ignore by the check. In addition, | ||||||
`union` types are completely excluded from the check. The check does not take | ||||||
into account type compatibility or data layout, only the names of the types. | ||||||
|
||||||
.. code-block:: c | ||||||
void test1(char *p) { | ||||||
struct S1 *s; | ||||||
s = (struct S1 *)p; // warn: 'char *' is converted to 'struct S1 *' | ||||||
|
||||||
} | ||||||
void test2(struct S1 *p) { | ||||||
struct S2 *s; | ||||||
s = (struct S2 *)p; // warn: 'struct S1 *' is converted to 'struct S2 *' | ||||||
} | ||||||
void test3(void) { | ||||||
struct S1 *s; | ||||||
s = (struct S1 *)calloc(1, sizeof(struct S1)); // no warning | ||||||
} | ||||||
Options | ||||||
------- | ||||||
|
||||||
.. option:: IgnoredFromTypes | ||||||
|
||||||
Semicolon-separated list of types for which the checker should not warn if | ||||||
|
Semicolon-separated list of types for which the checker should not warn if | |
Semicolon-separated list of types for which the check should not warn if |
Same in other places.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add default value. Same for other options.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove this option, as it can be easily abused. If one needs to perform these dangerous operations (despite them being UB), they should have a big NOLINT suppression with a long comment explaining why they are choosing to deviate and why it's safe.
I feel similarly about the other two options, but not as strongly ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, other options make sense that types are used in many-many places, but functions don't tend to have duplicate names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the ignored function option to remove false positives coming from system headers like xmmintrin.h from many functions like this:
static __inline__ __m128 __DEFAULT_FN_ATTRS
_mm_loadu_ps(const float *__p)
{
struct __loadu_ps {
__m128_u __v;
} __attribute__((__packed__, __may_alias__));
return ((const struct __loadu_ps*)__p)->__v; // warning: cast from 'const float *' to 'const struct __loadu_ps *'
}
Probably the type name __loadu_ps
or similar can be enough, or the case can be detected in other way (check for specific function attributes like nodebug
which is defined in __DEFAULT_FN_ATTRS
can work for this case). I wanted to have a solution to remove similar warnings. Sometimes such casts appear in specific macros, then a similar option for macro names would be needed. Or just do not emit warnings if it comes from system header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or just do not emit warnings if it comes from system header.
Yes, we should skip warnings in system headers. You might want to try rebasing on my patch and see if you still get any (even though clang-tidy should already be filtering those out by default).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These warning must be when only when --system-headers
is enabled. Also, you could use unless(isExpansionInSystemHeader())
in matchers.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
// RUN: %check_clang_tidy -check-suffixes=FUNC %s bugprone-cast-to-struct %t -- \ | ||
// RUN: -config="{CheckOptions: {bugprone-cast-to-struct.IgnoredFunctions: 'ignored_f$'}}" | ||
// RUN: %check_clang_tidy -check-suffixes=FROM-TY %s bugprone-cast-to-struct %t -- \ | ||
// RUN: -config="{CheckOptions: {bugprone-cast-to-struct.IgnoredFromTypes: 'int'}}" | ||
// RUN: %check_clang_tidy -check-suffixes=TO-TY %s bugprone-cast-to-struct %t -- \ | ||
// RUN: -config="{CheckOptions: {bugprone-cast-to-struct.IgnoredToTypes: 'IgnoredType'}}" | ||
|
||
struct IgnoredType { | ||
int a; | ||
}; | ||
|
||
struct OtherType { | ||
int a; | ||
int b; | ||
}; | ||
|
||
void ignored_f(char *p) { | ||
struct OtherType *p1; | ||
p1 = (struct OtherType *)p; | ||
// CHECK-MESSAGES-FROM-TY: :[[@LINE-1]]:8: warning: casting a 'char *' pointer to a 'struct OtherType *' pointer and accessing a field can lead to memory access errors or data corruption | ||
// CHECK-MESSAGES-TO-TY: :[[@LINE-2]]:8: warning: casting a 'char *' pointer to a 'struct OtherType *' pointer and accessing a field can lead to memory access errors or data corruption | ||
} | ||
|
||
void ignored_from_type(int *p) { | ||
struct OtherType *p1; | ||
p1 = (struct OtherType *)p; | ||
// CHECK-MESSAGES-FUNC: :[[@LINE-1]]:8: warning: casting a 'int *' pointer to a 'struct OtherType *' pointer and accessing a field can lead to memory access errors or data corruption | ||
// CHECK-MESSAGES-TO-TY: :[[@LINE-2]]:8: warning: casting a 'int *' pointer to a 'struct OtherType *' pointer and accessing a field can lead to memory access errors or data corruption | ||
} | ||
|
||
void ignored_to_type(char *p) { | ||
struct IgnoredType *p1; | ||
p1 = (struct IgnoredType *)p; | ||
// CHECK-MESSAGES-FUNC: :[[@LINE-1]]:8: warning: casting a 'char *' pointer to a 'struct IgnoredType *' pointer and accessing a field can lead to memory access errors or data corruption | ||
// CHECK-MESSAGES-FROM-TY: :[[@LINE-2]]:8: warning: casting a 'char *' pointer to a 'struct IgnoredType *' pointer and accessing a field can lead to memory access errors or data corruption | ||
} | ||
|
||
struct OtherType *test_void_is_always_ignored(void *p) { | ||
return (struct OtherType *)p; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
// RUN: %check_clang_tidy %s bugprone-cast-to-struct %t | ||
|
||
struct S1 { | ||
int a; | ||
}; | ||
|
||
struct S2 { | ||
char a; | ||
}; | ||
|
||
union U1 { | ||
int a; | ||
char b; | ||
}; | ||
|
||
union U2 { | ||
struct S1 a; | ||
char b; | ||
}; | ||
|
||
typedef struct S1 TyS1; | ||
typedef struct S1 *TyPS1; | ||
|
||
typedef union U1 *TyPU1; | ||
|
||
typedef int int_t; | ||
typedef int * int_ptr_t; | ||
|
||
struct S1 *test_simple(char *p) { | ||
return (struct S1 *)p; | ||
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: casting a 'char *' pointer to a 'struct S1 *' pointer and accessing a field can lead to memory access errors or data corruption [bugprone-cast-to-struct] | ||
struct S1 *s; | ||
int i; | ||
s = (struct S1 *)&i; | ||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: casting a 'int *' pointer to a 'struct S1 *' pointer and accessing a field can lead to memory access errors or data corruption [bugprone-cast-to-struct] | ||
} | ||
|
||
struct S1 *test_cast_from_void(void *p) { | ||
return (struct S1 *)p; | ||
} | ||
|
||
struct S1 *test_cast_from_struct(struct S2 *p) { | ||
return (struct S1 *)p; | ||
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: casting a 'struct S2 *' pointer to a 'struct S1 *' pointer and accessing a field can lead to memory access errors or data corruption [bugprone-cast-to-struct] | ||
} | ||
|
||
TyPS1 test_cast_from_similar(struct S1 *p) { | ||
return (TyPS1)p; | ||
} | ||
|
||
void test_typedef(char *p1, int_t *p2, int_ptr_t p3) { | ||
TyS1 *a = (TyS1 *)p1; | ||
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: casting a 'char *' pointer to a 'TyS1 *' (aka 'struct S1 *') pointer and accessing a field can lead to memory access errors or data corruption [bugprone-cast-to-struct] | ||
TyPS1 b = (TyPS1)p1; | ||
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: casting a 'char *' pointer to a 'TyPS1' (aka 'struct S1 *') pointer and accessing a field can lead to memory access errors or data corruption [bugprone-cast-to-struct] | ||
struct S1 *c = (struct S1 *)p2; | ||
// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: casting a 'int_t *' (aka 'int *') pointer to a 'struct S1 *' pointer and accessing a field can lead to memory access errors or data corruption [bugprone-cast-to-struct] | ||
struct S1 *d = (struct S1 *)p3; | ||
// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: casting a 'int_ptr_t' (aka 'int *') pointer to a 'struct S1 *' pointer and accessing a field can lead to memory access errors or data corruption [bugprone-cast-to-struct] | ||
} | ||
|
||
void test_union(char *p1, union U1 *p2, TyPU1 p3) { | ||
union U1 *a = (union U1 *)p1; | ||
struct S1 *b = (struct S1 *)p2; | ||
struct S1 *c = (struct S1 *)p3; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New style: