From 81697ca63d47decf165420c4373b2b53d7eab385 Mon Sep 17 00:00:00 2001 From: Noel Grandin Date: Tue, 13 Jul 2021 15:50:35 +0200 Subject: [PATCH] new loplugin:weakobject 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 --- compilerplugins/clang/test/weakobject.cxx | 31 +++ compilerplugins/clang/weakobject.cxx | 197 ++++++------------- solenv/CompilerTest_compilerplugins_clang.mk | 1 + 3 files changed, 89 insertions(+), 140 deletions(-) create mode 100644 compilerplugins/clang/test/weakobject.cxx diff --git a/compilerplugins/clang/test/weakobject.cxx b/compilerplugins/clang/test/weakobject.cxx new file mode 100644 index 000000000000..7c7da55664d2 --- /dev/null +++ b/compilerplugins/clang/test/weakobject.cxx @@ -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: */ diff --git a/compilerplugins/clang/weakobject.cxx b/compilerplugins/clang/weakobject.cxx index 055110ef9331..4801953cc44a 100644 --- a/compilerplugins/clang/weakobject.cxx +++ b/compilerplugins/clang/weakobject.cxx @@ -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 +#include +#include +#include +#include +#include #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 +namespace +{ +class WeakObject : public loplugin::FilteringPlugin { - 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(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(*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(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"); +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", false); } // namespace diff --git a/solenv/CompilerTest_compilerplugins_clang.mk b/solenv/CompilerTest_compilerplugins_clang.mk index d3131efd0e7e..89bb3058f4a7 100644 --- a/solenv/CompilerTest_compilerplugins_clang.mk +++ b/solenv/CompilerTest_compilerplugins_clang.mk @@ -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 \ ))