ODFstudio/odfstudio
2
0
Fork 0
Code Issues Packages Activity

new loplugin:weakobject

Browse Source 
find classes with more than one copy of OWeakObject in their inheritance
hierarchy, which is dodgy

Change-Id: I4e31bd6db03d25d934b736bd6a9c1b665f976ee2
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/118855
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
This commit is contained in:
Noel Grandin
2021-07-14 14:39:39 +02:00
parent 03cdb7392b
commit 81697ca63d
3 changed files with 89 additions and 140 deletions
Download Patch File Download Diff File Expand all files Collapse all files
compilerplugins/clang/test/weakobject.cxx
+31
View File
@@ -0,0 +1,31 @@
/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
/*
* This file is part of the LibreOffice project.
*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/
#include "config_clang.h"
namespace cppu
{
class OWeakObject
{
};
}
class Foo1 : public cppu::OWeakObject
{
};
class Foo2 : public cppu::OWeakObject
{
};
// expected-error@+1 {{more than one copy of cppu::OWeakObject inherited [loplugin:weakobject]}}
class Foo3 : public Foo1, public Foo2
{
};
/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
compilerplugins/clang/weakobject.cxx
+57 -140
View File
@@ -2,163 +2,80 @@
/*
* This file is part of the LibreOffice project.
*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
* Based on LLVM/Clang.
*
* This file is distributed under the University of Illinois Open Source
* License. See LICENSE.TXT for details.
*
*/
#ifndef LO_CLANG_SHARED_PLUGINS
#include "check.hxx"
#include <cassert>
#include <string>
#include <iostream>
#include <fstream>
#include <set>
#include <unordered_set>
#include "plugin.hxx"
#include "check.hxx"
/* OWeakObject::release() disposes weak references. If that isn't done
* because a sub-class improperly overrides release() then
* OWeakConnectionPoint::m_pObject continues to point to the deleted object
* and that could result in use-after-free.
*/
/*
Check for places where we end up with more than one copy of cppu::OweakObject in a class, which
really should not happen - we should be using one of the AggImplInheritanceHelper classes then
to inherit.
*/
namespace {
class WeakObject
: public loplugin::FilteringPlugin<WeakObject>
namespace
{
class WeakObject : public loplugin::FilteringPlugin<WeakObject>
{
public:
explicit WeakObject(loplugin::InstantiationData const& rData): FilteringPlugin(rData)
{}
virtual bool preRun() override {
return compiler.getLangOpts().CPlusPlus; // no OWeakObject in C
}
void run() override {
if( preRun()) {
TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
}
}
bool isDerivedFromOWeakObject(CXXMethodDecl const*const pMethodDecl)
explicit WeakObject(loplugin::InstantiationData const& data)
: FilteringPlugin(data)
{
QualType const pClass(pMethodDecl->getParent()->getTypeForDecl(), 0);
if (loplugin::TypeCheck(pClass).Class("OWeakObject").Namespace("cppu"))
{
return true;
}
// hopefully it's faster to recurse overridden methods than the
// thicket of WeakImplHelper32 but that is purely speculation
for (auto it = pMethodDecl->begin_overridden_methods();
it != pMethodDecl->end_overridden_methods(); ++it)
{
if (isDerivedFromOWeakObject(*it))
{
return true;
}
}
return false;
}
bool VisitCXXMethodDecl(CXXMethodDecl const*const pMethodDecl)
virtual bool preRun() override
{
if (ignoreLocation(pMethodDecl)) {
return true;
}
if (!pMethodDecl->isThisDeclarationADefinition()
|| pMethodDecl->isLateTemplateParsed())
{
return true;
}
if (!pMethodDecl->isInstance()) {
return true;
}
// this is too "simple", if a NamedDecl class has a getName() member expecting it to actually work would clearly be unreasonable if (pMethodDecl->getName() != "release") {
if (auto const i = pMethodDecl->getIdentifier()) {
if (i != nullptr && !i->isStr("release")) {
return true;
}
}
if (pMethodDecl->getNumParams() != 0) {
return true;
}
if (loplugin::TypeCheck(QualType(pMethodDecl->getParent()->getTypeForDecl(), 0)).Class("OWeakObject").Namespace("cppu"))
{
return true;
}
CXXMethodDecl const* pOverridden(nullptr);
for (auto it = pMethodDecl->begin_overridden_methods();
it != pMethodDecl->end_overridden_methods(); ++it)
{
if (auto const i = (*it)->getIdentifier()) {
if (i != nullptr && i->isStr("release")) {
pOverridden = *it;
break;
}
}
}
if (pOverridden == nullptr)
{
return true;
}
if (!isDerivedFromOWeakObject(pOverridden))
{
return true;
}
CompoundStmt const*const pCompoundStatement(
dyn_cast<CompoundStmt>(pMethodDecl->getBody()));
for (auto i = pCompoundStatement->body_begin();
i != pCompoundStatement->body_end(); ++i)
{
// note: this is not a CXXMemberCallExpr
CallExpr const*const pCallExpr(dyn_cast<CallExpr>(*i));
if (pCallExpr)
{
// note: this is only sometimes a CXXMethodDecl
FunctionDecl const*const pCalled(pCallExpr->getDirectCallee());
if (pCalled->getName() == "release")
{
//this never works && pCalled == pOverridden
if (pCalled->getParent() == pOverridden->getParent())
{
return true;
}
// Allow this convenient shortcut:
auto td = dyn_cast<TypeDecl>(pCalled->getParent());
if (td != nullptr
&& (loplugin::TypeCheck(td).Class("OWeakObject").Namespace("cppu").GlobalNamespace()
|| loplugin::TypeCheck(td).Class("OWeakAggObject").Namespace("cppu").GlobalNamespace()))
{
return true;
}
}
}
}
// allowlist
auto tc = loplugin::TypeCheck(pMethodDecl->getParent());
if ( tc.Class("OWeakAggObject").Namespace("cppu").GlobalNamespace() // conditional call
|| tc.Class("WeakComponentImplHelperBase").Namespace("cppu").GlobalNamespace() // extra magic
|| tc.Class("WeakAggComponentImplHelperBase").Namespace("cppu").GlobalNamespace() // extra magic
|| tc.Class("CDOMImplementation").Namespace("DOM").GlobalNamespace() // a static oddity
|| tc.Class("SwXTextFrame").GlobalNamespace() // ambiguous, 3 parents
|| tc.Class("SwXTextDocument").GlobalNamespace() // ambiguous, ~4 parents
|| tc.Class("SdStyleSheet").GlobalNamespace() // same extra magic as WeakComponentImplHelperBase
|| tc.Class("SdXImpressDocument").GlobalNamespace() // same extra magic as WeakComponentImplHelperBase
)
{
return true;
}
report(DiagnosticsEngine::Warning,
"override of OWeakObject::release() does not call superclass release()",
pMethodDecl->getLocation())
<< pMethodDecl->getSourceRange();
return true;
}
virtual void run() override
{
if (preRun())
TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
}
bool VisitCXXRecordDecl( const CXXRecordDecl* decl);
};
loplugin::Plugin::Registration<WeakObject> weakobject("weakobject");
bool WeakObject::VisitCXXRecordDecl(const CXXRecordDecl* decl)
{
if (ignoreLocation(decl))
return true;
if (!decl->hasDefinition())
return true;
if (decl->hasAnyDependentBases())
return true;
int cnt = 0;
decl->forallBases(
[&cnt] (const CXXRecordDecl *BaseDefinition) -> bool
{
if (loplugin::DeclCheck(BaseDefinition).Class("OWeakObject").Namespace("cppu").GlobalNamespace())
++cnt;
return true;
});
if (cnt < 2)
return true;
report(DiagnosticsEngine::Warning, "more than one copy of cppu::OWeakObject inherited",
compat::getBeginLoc(decl))
<< decl->getSourceRange();
return true;
}
loplugin::Plugin::Registration<WeakObject> weakobject("weakobject", false);
} // namespace
solenv/CompilerTest_compilerplugins_clang.mk
+1
View File
@@ -113,6 +113,7 @@ $(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
compilerplugins/clang/test/useuniqueptr \
compilerplugins/clang/test/vclwidgets \
compilerplugins/clang/test/weakbase \
compilerplugins/clang/test/weakobject \
compilerplugins/clang/test/writeonlyvars \
compilerplugins/clang/test/xmlimport \
))