summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMichael Stahl <mstahl@redhat.com>2016-06-20 23:38:11 +0200
committerStephan Bergmann <sbergman@redhat.com>2016-06-24 12:05:09 +0000
commit136a2fd6c08193793d546e69108765316c96668b (patch)
tree1e78b7cbb60c0726d4c13ac171b973eff6978466
parent7d76bb251e0c88ff17282a33b801a5d17a434af5 (diff)
compilerplugins: add OWeakObject::release() override check
Change-Id: I767857545d7c91615cf162790c04f0016de9fdf6 Reviewed-on: https://gerrit.libreoffice.org/26555 Reviewed-by: Stephan Bergmann <sbergman@redhat.com> Tested-by: Stephan Bergmann <sbergman@redhat.com>
-rw-r--r--basic/source/classes/sbxmod.cxx13
-rw-r--r--compilerplugins/clang/weakobject.cxx154
-rw-r--r--dbaccess/source/ui/inc/exsrcbrw.hxx2
-rw-r--r--extensions/source/propctrlr/composeduiupdate.cxx5
-rw-r--r--scripting/source/provider/BrowseNodeFactoryImpl.cxx7
-rw-r--r--sw/source/core/access/acctable.hxx4
6 files changed, 164 insertions, 21 deletions
diff --git a/basic/source/classes/sbxmod.cxx b/basic/source/classes/sbxmod.cxx
index ef6a6797c93b..7970f04b79b6 100644
--- a/basic/source/classes/sbxmod.cxx
+++ b/basic/source/classes/sbxmod.cxx
@@ -175,21 +175,14 @@ DocObjectWrapper::DocObjectWrapper( SbModule* pVar ) : m_pMod( pVar ), mName( pV
void SAL_CALL
DocObjectWrapper::acquire() throw ()
{
- osl_atomic_increment( &m_refCount );
+ OWeakObject::acquire();
SAL_INFO("basic","DocObjectWrapper::acquire("<< OUStringToOString( mName, RTL_TEXTENCODING_UTF8 ).getStr() << ") 0x" << this << " refcount is now " << m_refCount );
}
void SAL_CALL
DocObjectWrapper::release() throw ()
{
- if ( osl_atomic_decrement( &m_refCount ) == 0 )
- {
- SAL_INFO("basic","DocObjectWrapper::release("<< OUStringToOString( mName, RTL_TEXTENCODING_UTF8 ).getStr() << ") 0x" << this << " refcount is now " << m_refCount );
- delete this;
- }
- else
- {
- SAL_INFO("basic","DocObjectWrapper::release("<< OUStringToOString( mName, RTL_TEXTENCODING_UTF8 ).getStr() << ") 0x" << this << " refcount is now " << m_refCount );
- }
+ SAL_INFO("basic","DocObjectWrapper::release("<< OUStringToOString( mName, RTL_TEXTENCODING_UTF8 ).getStr() << ") 0x" << this << " decrementing refcount, was " << m_refCount );
+ OWeakObject::release();
}
DocObjectWrapper::~DocObjectWrapper()
diff --git a/compilerplugins/clang/weakobject.cxx b/compilerplugins/clang/weakobject.cxx
new file mode 100644
index 000000000000..42bdf6e16481
--- /dev/null
+++ b/compilerplugins/clang/weakobject.cxx
@@ -0,0 +1,154 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * 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 "plugin.hxx"
+#include "typecheck.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.
+ */
+
+namespace {
+
+class WeakObject
+ : public clang::RecursiveASTVisitor<WeakObject>
+ , public loplugin::Plugin
+{
+
+public:
+ explicit WeakObject(InstantiationData const& rData) : Plugin(rData) {}
+
+ void run() override {
+ if (compiler.getLangOpts().CPlusPlus) { // no OWeakObject in C
+ TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+ }
+ }
+
+ bool isDerivedFromOWeakObject(CXXMethodDecl const*const pMethodDecl)
+ {
+ 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)
+ {
+ if (ignoreLocation(pMethodDecl)) {
+ return true;
+ }
+ if (!pMethodDecl->isThisDeclarationADefinition()) {
+ 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 const pStmt : pCompoundStatement->body())
+ {
+ // note: this is not a CXXMemberCallExpr
+ CallExpr const*const pCallExpr(dyn_cast<CallExpr>(pStmt));
+ if (pCallExpr)
+ {
+ // note: this is only sometimes a CXXMethodDecl
+ FunctionDecl const*const pCalled(pCallExpr->getDirectCallee());
+ if (pCalled->getName() == "release"
+//this never works && pCalled == pOverridden
+ && (pCalled->getParent() == pOverridden->getParent()
+ // allow this convenient shortcut
+ || loplugin::TypeCheck(QualType(pMethodDecl->getParent()->getTypeForDecl(), 0)).Class("OWeakObject").Namespace("cppu")
+ || loplugin::TypeCheck(QualType(pMethodDecl->getParent()->getTypeForDecl(), 0)).Class("OWeakAggObject").Namespace("cppu")))
+ {
+ return true;
+ }
+ if (pCalled->getName() == "relase_ChildImpl") // FIXME remove this lunacy
+ {
+ return true;
+ }
+ }
+ }
+
+ // whitelist
+ auto const name(pMethodDecl->getParent()->getQualifiedNameAsString());
+ if ( name == "cppu::OWeakAggObject" // conditional call
+ || name == "cppu::WeakComponentImplHelperBase" // extra magic
+ || name == "cppu::WeakAggComponentImplHelperBase" // extra magic
+ || name == "DOM::CDOMImplementation" // a static oddity
+ || name == "SwXTextFrame" // ambiguous, 3 parents
+ || name == "SwXTextDocument" // ambiguous, ~4 parents
+ || name == "SdStyleSheet" // same extra magic as WeakComponentImplHelperBase
+ || name == "SdXImpressDocument" // 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;
+ }
+
+};
+
+loplugin::Plugin::Registration<WeakObject> X("weakobject");
+
+} // namespace
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/dbaccess/source/ui/inc/exsrcbrw.hxx b/dbaccess/source/ui/inc/exsrcbrw.hxx
index 3a533e8fadf4..53a10bb54967 100644
--- a/dbaccess/source/ui/inc/exsrcbrw.hxx
+++ b/dbaccess/source/ui/inc/exsrcbrw.hxx
@@ -50,7 +50,7 @@ namespace dbaui
SAL_CALL Create(const css::uno::Reference< css::lang::XMultiServiceFactory >&);
// UNO
- DECLARE_UNO3_DEFAULTS(SbaExternalSourceBrowser, OGenericUnoController)
+ DECLARE_UNO3_DEFAULTS(SbaExternalSourceBrowser, SbaXDataBrowserController)
virtual css::uno::Any SAL_CALL queryInterface(const css::uno::Type& _rType) throw (css::uno::RuntimeException, std::exception) override;
// virtual css::uno::Sequence< css::uno::Reference< css::reflection::XIdlClass > > getIdlClasses();
diff --git a/extensions/source/propctrlr/composeduiupdate.cxx b/extensions/source/propctrlr/composeduiupdate.cxx
index 8ad15cff1871..5fde3d39b5df 100644
--- a/extensions/source/propctrlr/composeduiupdate.cxx
+++ b/extensions/source/propctrlr/composeduiupdate.cxx
@@ -211,14 +211,13 @@ namespace pcr
void SAL_CALL CachedInspectorUI::acquire() throw()
{
- osl_atomic_increment( &m_refCount );
+ CachedInspectorUI_Base::acquire();
}
void SAL_CALL CachedInspectorUI::release() throw()
{
- if ( 0 == osl_atomic_decrement( &m_refCount ) )
- delete this;
+ CachedInspectorUI_Base::release();
}
diff --git a/scripting/source/provider/BrowseNodeFactoryImpl.cxx b/scripting/source/provider/BrowseNodeFactoryImpl.cxx
index 863860e2911e..a312ed4aa35e 100644
--- a/scripting/source/provider/BrowseNodeFactoryImpl.cxx
+++ b/scripting/source/provider/BrowseNodeFactoryImpl.cxx
@@ -500,15 +500,12 @@ public:
throw () override
{
- osl_atomic_increment( &m_refCount );
+ t_BrowseNodeBase::acquire();
}
virtual void SAL_CALL release()
throw () override
{
- if ( osl_atomic_decrement( &m_refCount ) == 0 )
- {
- delete this;
- }
+ t_BrowseNodeBase::release();
}
// XTypeProvider (implemnented by base, but needs to be overridden for
// delegating to aggregate)
diff --git a/sw/source/core/access/acctable.hxx b/sw/source/core/access/acctable.hxx
index 84930ef9f378..857636eaa19d 100644
--- a/sw/source/core/access/acctable.hxx
+++ b/sw/source/core/access/acctable.hxx
@@ -301,10 +301,10 @@ public:
throw (css::uno::RuntimeException, std::exception) override;
virtual void SAL_CALL acquire( ) throw () override
- { SwAccessibleContext::acquire(); };
+ { SwAccessibleTable::acquire(); };
virtual void SAL_CALL release( ) throw () override
- { SwAccessibleContext::release(); };
+ { SwAccessibleTable::release(); };
// XAccessibleContext