-
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 2 commits
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), | ||
IgnoredCasts( | ||
utils::options::parseStringList(Options.get("IgnoredCasts", ""))) {} | ||
|
||
void CastToStructCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { | ||
Options.store(Opts, "IgnoredCasts", | ||
utils::options::serializeStringList(IgnoredCasts)); | ||
} | ||
|
||
void CastToStructCheck::registerMatchers(MatchFinder *Finder) { | ||
auto FromPointee = | ||
qualType(hasUnqualifiedDesugaredType(type().bind("FromType")), | ||
unless(voidType()), | ||
unless(hasDeclaration(recordDecl(isUnion())))) | ||
.bind("FromPointee"); | ||
auto ToPointee = | ||
qualType(hasUnqualifiedDesugaredType( | ||
recordType(unless(hasDeclaration(recordDecl(isUnion())))) | ||
.bind("ToType"))) | ||
.bind("ToPointee"); | ||
auto FromPtrType = qualType(pointsTo(FromPointee)).bind("FromPtr"); | ||
auto ToPtrType = qualType(pointsTo(ToPointee)).bind("ToPtr"); | ||
Finder->addMatcher(cStyleCastExpr(hasSourceExpression(hasType(FromPtrType)), | ||
hasType(ToPtrType)) | ||
.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 (FromType == ToType) | ||
return; | ||
|
||
const std::string FromName = FromPointee->getAsString(); | ||
const std::string ToName = ToPointee->getAsString(); | ||
llvm::Regex FromR; | ||
llvm::Regex ToR; | ||
for (auto [Idx, Str] : llvm::enumerate(IgnoredCasts)) { | ||
if (Idx % 2 == 0) { | ||
FromR = llvm::Regex(Str); | ||
} else { | ||
ToR = llvm::Regex(Str); | ||
if (FromR.match(FromName) && ToR.match(ToName)) | ||
return; | ||
} | ||
} | ||
steakhal marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
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,37 @@ | ||||||
//===--- 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,60 @@ | ||||||
.. 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. Cast from a | ||||||
scalar-type pointer (which points often to an array or memory block) to a | ||||||
``struct`` type pointer can be unsafe for similar reasons. This check warns at | ||||||
pointer 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). In addition, ``union`` types are excluded from the | ||||||
check. It is possible to specify additional types to ignore. 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 | ||||||
} | ||||||
Limitations | ||||||
----------- | ||||||
|
||||||
The check does run only on `C` code. | ||||||
|
||||||
|
||||||
C-style casts are discouraged in `C++` and should be converted to more type-safe | ||||||
|
C-style casts are discouraged in `C++` and should be converted to more type-safe | |
C-style casts are discouraged in C++ and should be converted to more type-safe |
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.
``;`` characters. For example ``char;Type1;char;Type2`` specifies that the | |
`;` characters. For example `char;Type1;char;Type2` specifies that the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
// RUN: %check_clang_tidy %s bugprone-cast-to-struct %t -- \ | ||
// RUN: -config="{CheckOptions: {bugprone-cast-to-struct.IgnoredCasts: 'char;S1;int;Other*'}}" | ||
steakhal marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
struct S1 { | ||
int a; | ||
}; | ||
|
||
struct S2 { | ||
char a; | ||
}; | ||
|
||
struct OtherS { | ||
int a; | ||
int b; | ||
}; | ||
|
||
void test1(char *p1, int *p2) { | ||
struct S1 *s1; | ||
s1 = (struct S1 *)p1; | ||
struct S2 *s2; | ||
s2 = (struct S2 *)p1; | ||
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: casting a 'char *' pointer to a 'struct S2 *' | ||
s2 = (struct S2 *)p2; | ||
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: casting a 'int *' pointer to a 'struct S2 *' | ||
struct OtherS *s3; | ||
s3 = (struct OtherS *)p2; | ||
s3 = (struct OtherS *)p1; | ||
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: casting a 'char *' pointer to a 'struct OtherS *' | ||
} | ||
|
||
struct S2 *test_void_is_always_ignored(void *p) { | ||
return (struct S2 *)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: