Skip to content

Commit

Permalink
nullptr deref fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@Javier-varez
Javier-varez committed Jul 29, 2022
1 parent e99c347 commit 9526379
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 41 deletions.
102 changes: 63 additions & 39 deletions clang-tools-extra/clang-tidy/daedalean/DerivedClassesCheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"

#include <set>
#include <sstream>
#include <unordered_map>
#include <set>

using namespace clang::ast_matchers;

Expand All @@ -31,7 +31,10 @@ bool isInterface(const CXXRecordDecl *pDecl) {
return false;
}

for (const auto & m : pDecl->methods()) {
for (const auto *m : pDecl->methods()) {
if (m == nullptr) {
continue;
}

if (llvm::isa<CXXDestructorDecl>(m) || m->isImplicit()) {
continue;
Expand All @@ -49,10 +52,9 @@ bool isFinal(const CXXRecordDecl *pDecl) {
if (!Def)
return false;
return Def->hasAttr<FinalAttr>();

}

}
} // namespace

void DerivedClassesCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(cxxRecordDecl(hasDefinition()).bind("x"), this);
Expand All @@ -62,7 +64,9 @@ void DerivedClassesCheck::check(const MatchFinder::MatchResult &Result) {

const auto *MatchedDecl = Result.Nodes.getNodeAs<CXXRecordDecl>("x");

if (!MatchedDecl || (MatchedDecl->methods().empty() && MatchedDecl->field_empty() && MatchedDecl->getNumBases() == 0)) {
if (!MatchedDecl ||
(MatchedDecl->methods().empty() && MatchedDecl->field_empty() &&
MatchedDecl->getNumBases() == 0)) {
// Skip empty class
return;
}
Expand All @@ -75,27 +79,37 @@ void DerivedClassesCheck::check(const MatchFinder::MatchResult &Result) {
const auto dtor = MatchedDecl->getDestructor();
// 1. Each interface MUST have a virtual default destructor.
if (!dtor || (dtor->isImplicit() && !dtor->isVirtual())) {
diag(MatchedDecl->getBraceRange().getBegin(), "Interface %0 must have virtual defaulted destructor") << MatchedDecl;
diag(MatchedDecl->getBraceRange().getBegin(),
"Interface %0 must have virtual defaulted destructor")
<< MatchedDecl;
} else {
if (!dtor->isVirtual()) {
diag(dtor->getLocation(), "Interface destructor must be virtual");
diag(dtor->getLocation(), "Make destructor virtual", DiagnosticIDs::Note) << FixItHint::CreateInsertion(dtor->getLocation(), "virtual ");
diag(dtor->getLocation(), "Make destructor virtual",
DiagnosticIDs::Note)
<< FixItHint::CreateInsertion(dtor->getLocation(), "virtual ");
}
if (!dtor->isDefaulted()) {
diag(dtor->getLocation(), "Interface destructor must be defaulted");
if (const auto body = dtor->getBody(); body) {
diag(body->getBeginLoc(), "Make destructor default", DiagnosticIDs::Note) << FixItHint::CreateReplacement(body->getSourceRange(), "= default");
diag(body->getBeginLoc(), "Make destructor default",
DiagnosticIDs::Note)
<< FixItHint::CreateReplacement(body->getSourceRange(),
"= default");
}
}
}
} else {
// 7. If a class contains at least one non-pure-virtual method other than destructor it MUST be declared final.
// 7. If a class contains at least one non-pure-virtual method other than
// destructor it MUST be declared final.
if (!isFinal(MatchedDecl)) {
diag(MatchedDecl->getLocation(), "Non-interface class %0 must be final") << MatchedDecl;
diag(MatchedDecl->getLocation(), "Make class final", DiagnosticIDs::Note) << FixItHint::CreateInsertion(MatchedDecl->getLocation(), " final");
diag(MatchedDecl->getLocation(), "Non-interface class %0 must be final")
<< MatchedDecl;
diag(MatchedDecl->getLocation(), "Make class final", DiagnosticIDs::Note)
<< FixItHint::CreateInsertion(MatchedDecl->getLocation(), " final");
}

for (const auto & m : MatchedDecl->methods()) {
for (const auto &m : MatchedDecl->methods()) {
if (m->isImplicit()) {
continue;
}
Expand All @@ -109,26 +123,26 @@ void DerivedClassesCheck::check(const MatchFinder::MatchResult &Result) {
if (llvm::isa<CXXDestructorDecl>(m) && m->isDefaulted()) {
continue;
}
// 6. Virtual functions MUST contain exactly one of two specifiers: virtual for new function or final if function overrides method from base class.
// 6. Virtual functions MUST contain exactly one of two specifiers:
// virtual for new function or final if function overrides method from
// base class.
if (!m->hasAttr<FinalAttr>()) {
diag(m->getLocation(), "Implemented virtual methods must be final");
if (const auto attr = m->getAttr<OverrideAttr>(); attr) {
diag(m->getLocation(), "Make method final",
DiagnosticIDs::Note)
<< FixItHint::CreateReplacement(attr->getLocation(),
" final");
diag(m->getLocation(), "Make method final", DiagnosticIDs::Note)
<< FixItHint::CreateReplacement(attr->getLocation(), " final");
} else {
diag(m->getLocation(), "Make method final",
DiagnosticIDs::Note)
<< FixItHint::CreateInsertion(m->getLocation(),
" final");
diag(m->getLocation(), "Make method final", DiagnosticIDs::Note)
<< FixItHint::CreateInsertion(m->getLocation(), " final");
}
}
}
}

// 5. All entity names MUST be unique in all inherited interfaces. If two interfaces share the same methods they MUST extend the common base interface.
for (const auto & m : MatchedDecl->methods()) {
// 5. All entity names MUST be unique in all inherited interfaces. If two
// interfaces share the same methods they MUST extend the common base
// interface.
for (const auto &m : MatchedDecl->methods()) {
if (m->isImplicit()) {
continue;
}
Expand Down Expand Up @@ -157,11 +171,16 @@ void DerivedClassesCheck::check(const MatchFinder::MatchResult &Result) {
}

for (const auto base : MatchedDecl->bases()) {
// 2. Inheritance from base class with non-pure-virtual methods other than destructor and/or data members is forbidden.
const CXXRecordDecl * baseRecord = getRecordFromBase(base);
// 2. Inheritance from base class with non-pure-virtual methods other than
// destructor and/or data members is forbidden.
const CXXRecordDecl *baseRecord = getRecordFromBase(base);
if (baseRecord == nullptr) {
continue;
}

if (!isInterface(baseRecord)) {
diag(base.getBeginLoc(), "Inheritance from non-interface type is forbidden");
diag(base.getBeginLoc(),
"Inheritance from non-interface type is forbidden");
} else {
// 3. Public inheritance MUST be used to implement interfaces.
if (base.getAccessSpecifier() == AS_public) {
Expand All @@ -176,7 +195,7 @@ void DerivedClassesCheck::check(const MatchFinder::MatchResult &Result) {
std::unordered_map<std::string, std::set<BaseRef>> baseReferences;
walkBases(MatchedDecl, baseReferences);
// 4. Base class MUST be declared virtual if it is in diamond inheritance.
for (const auto [base, derived]: baseReferences) {
for (const auto [base, derived] : baseReferences) {
if (derived.size() <= 1) {
continue;
}
Expand All @@ -194,39 +213,45 @@ void DerivedClassesCheck::check(const MatchFinder::MatchResult &Result) {
}

for (const auto d : derived) {
diag(d.base.getBeginLoc(), "Interface " + base + " must be implemented virtually because it's extended by multiple interfaces (" + allRefs.str() + ") with root type " + MatchedDecl->getQualifiedNameAsString());
diag(d.base.getBeginLoc(), "Interface " + base +
" must be implemented virtually because "
"it's extended by multiple interfaces (" +
allRefs.str() + ") with root type " +
MatchedDecl->getQualifiedNameAsString());
diag(d.base.getBeginLoc(), "Make inheritance virtual",
DiagnosticIDs::Note)
<< FixItHint::CreateInsertion(d.base.getBeginLoc(),"virtual ");
<< FixItHint::CreateInsertion(d.base.getBeginLoc(), "virtual ");
}

}

}

const CXXRecordDecl * DerivedClassesCheck::getRecordFromBase(const CXXBaseSpecifier & base) {
if (const CXXRecordDecl * baseRecord = base.getType()->getAsCXXRecordDecl()) {
const CXXRecordDecl *
DerivedClassesCheck::getRecordFromBase(const CXXBaseSpecifier &base) {
if (const CXXRecordDecl *baseRecord = base.getType()->getAsCXXRecordDecl()) {
return baseRecord;
}

if (const auto spec = llvm::dyn_cast_or_null<TemplateSpecializationType>(base.getType().getTypePtr())) {
if (const auto spec = llvm::dyn_cast_or_null<TemplateSpecializationType>(
base.getType().getTypePtr())) {
if (const auto cls = llvm::dyn_cast_or_null<ClassTemplateDecl>(
spec->getTemplateName().getAsTemplateDecl())) {
return cls->getTemplatedDecl();
} else {
diag(base.getBeginLoc(),
"Unable to get base template spec %0 as record")
diag(base.getBeginLoc(), "Unable to get base template spec %0 as record")
<< spec->getTemplateName();
return nullptr;
}
} else {
diag(base.getBeginLoc(),
"Unable to get base of type %0 as record or template") << base.getType();
"Unable to get base of type %0 as record or template")
<< base.getType();
return nullptr;
}
}

void DerivedClassesCheck::walkBases(const CXXRecordDecl *pDecl, std::unordered_map<std::string, std::set<BaseRef>> & references) {
void DerivedClassesCheck::walkBases(
const CXXRecordDecl *pDecl,
std::unordered_map<std::string, std::set<BaseRef>> &references) {
const auto ref = pDecl->getQualifiedNameAsString();
for (const auto base : pDecl->bases()) {
const auto decl = getRecordFromBase(base);
Expand All @@ -238,7 +263,6 @@ void DerivedClassesCheck::walkBases(const CXXRecordDecl *pDecl, std::unordered_m
}
}


} // namespace daedalean
} // namespace tidy
} // namespace clang
6 changes: 4 additions & 2 deletions clang-tools-extra/clang-tidy/daedalean/PreprocessingDirectivesCheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,10 @@ class PreprocessingDirectivesPPCallbacks : public PPCallbacks {
virtual void MacroUndefined(const Token &MacroNameTok,
const MacroDefinition &MD,
const MacroDirective *Undef) override {
Check.diag(Undef->getLocation(),
"Preprocessor directives must not be used");
if (Undef) {
Check.diag(Undef->getLocation(),
"Preprocessor directives must not be used");
}
}

virtual void If(SourceLocation Loc, SourceRange ConditionRange,
Expand Down

0 comments on commit 9526379

Please sign in to comment.