[Libreoffice-commits] core.git: Branch 'libreoffice-7-0' - sdext/source

[Michael Stahl (via logerrit)]

 sdext/source/presenter/PresenterAccessibility.cxx |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

New commits:
commit 2b779910288eece35819375fa1b00299b94fa2b1
Author:     Michael Stahl <Michael.Stahl at cib.de>
AuthorDate: Mon Oct 12 12:19:55 2020 +0200
Commit:     Caolán McNamara <caolanm at redhat.com>
CommitDate: Wed Oct 14 10:31:39 2020 +0200

    sdext: fix use-after-free on global AccessibleFocusManager
    
    The problem is that the destructor of the vector maFocusableObjects
    ends up dispose()-ing every element, which calls back into
    AccessibleFocusManager to remove the element from the vector, which
    invokes its destructor a 2nd time.
    
    Move it to the stack so it doesn't double-free itself.
    
    ERROR: AddressSanitizer: heap-use-after-free on address 0x612001571c00 at pc 0x7fc5e723ca72 bp 0x7fffbaa8d6d0 sp 0x7fffbaa8d6c8
    READ of size 1 at 0x612001571c00 thread T0
        #0 0x7fc5e723ca71 in cppu::WeakComponentImplHelperBase::release() cppuhelper/source/implbase.cxx:84:9
        #1 0x7fc595211b27 in cppu::PartialWeakComponentImplHelper<com::sun::star::accessibility::XAccessible, com::sun::star::accessibility::XAccessibleContext, com::sun::star::accessibility::XAccessibleComponent, com::sun::star::accessibility::XAccessibleEventBroadcaster, com::sun::star::awt::XWindowListener>::release() include/cppuhelper/compbase.hxx:86:36
        #2 0x7fc5952093e4 in rtl::Reference<sdext::presenter::PresenterAccessible::AccessibleObject>::~Reference() include/rtl/ref.hxx:113:22
        #3 0x7fc59522acd4 in void std::_Destroy<rtl::Reference<sdext::presenter::PresenterAccessible::AccessibleObject> >(rtl::Reference<sdext::presenter::PresenterAccessible::AccessibleObject>*) /usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../include/c++/10/bits/stl_construct.h:140:19
    
    0x612001571c00 is located 64 bytes inside of 312-byte region [0x612001571bc0,0x612001571cf8)
    freed by thread T0 here:
        #0 0x4be997 in free (instdir/program/soffice.bin+0x4be997)
        #1 0x7fc5ea2a5104 in rtl_freeMemory sal/rtl/alloc_global.cxx:51:5
        #2 0x7fc5952097f4 in cppu::WeakComponentImplHelperBase::operator delete(void*) include/cppuhelper/compbase_ex.hxx:66:11
        #3 0x7fc595211e07 in sdext::presenter::PresenterAccessible::AccessibleObject::~AccessibleObject() sdext/source/presenter/PresenterAccessibility.cxx:67:28
        #4 0x7fc5e74a11b4 in cppu::OWeakObject::release() cppuhelper/source/weak.cxx:233:9
        #5 0x7fc5e723cb05 in cppu::WeakComponentImplHelperBase::release() cppuhelper/source/implbase.cxx:86:18
        #6 0x7fc595211b27 in cppu::PartialWeakComponentImplHelper<com::sun::star::accessibility::XAccessible, com::sun::star::accessibility::XAccessibleContext, com::sun::star::accessibility::XAccessibleComponent, com::sun::star::accessibility::XAccessibleEventBroadcaster, com::sun::star::awt::XWindowListener>::release() include/cppuhelper/compbase.hxx:86:36
        #7 0x7fc5e7194115 in com::sun::star::uno::Reference<com::sun::star::uno::XInterface>::~Reference() include/com/sun/star/uno/Reference.hxx:110:22
        #8 0x7fc5e71f3944 in com::sun::star::lang::EventObject::~EventObject() workdir/UnoApiHeadersTarget/udkapi/comprehensive/com/sun/star/lang/EventObject.hdl:18:27
        #9 0x7fc5e723d395 in cppu::WeakComponentImplHelperBase::dispose() cppuhelper/source/implbase.cxx:118:5
        #10 0x7fc595211e27 in cppu::PartialWeakComponentImplHelper<com::sun::star::accessibility::XAccessible, com::sun::star::accessibility::XAccessibleContext, com::sun::star::accessibility::XAccessibleComponent, com::sun::star::accessibility::XAccessibleEventBroadcaster, com::sun::star::awt::XWindowListener>::dispose() include/cppuhelper/compbase.hxx:90:36
        #11 0x7fc5e723c6e9 in cppu::WeakComponentImplHelperBase::release() cppuhelper/source/implbase.cxx:79:13
        #12 0x7fc595211b27 in cppu::PartialWeakComponentImplHelper<com::sun::star::accessibility::XAccessible, com::sun::star::accessibility::XAccessibleContext, com::sun::star::accessibility::XAccessibleComponent, com::sun::star::accessibility::XAccessibleEventBroadcaster, com::sun::star::awt::XWindowListener>::release() include/cppuhelper/compbase.hxx:86:36
        #13 0x7fc5952093e4 in rtl::Reference<sdext::presenter::PresenterAccessible::AccessibleObject>::~Reference() include/rtl/ref.hxx:113:22
        #14 0x7fc59522acd4 in void std::_Destroy<rtl::Reference<sdext::presenter::PresenterAccessible::AccessibleObject> >(rtl::Reference<sdext::presenter::PresenterAccessible::AccessibleObject>*) /usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../include/c++/10/bits/stl_construct.h:140:19
    
    Change-Id: I95151807e9182ed5f43b63792fba86f83ee0bad8
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/104208
    Tested-by: Jenkins
    Reviewed-by: Michael Stahl <michael.stahl at cib.de>
    (cherry picked from commit 2cb54449b4fc9e908aae0a44f808d82b8e83acfc)
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/104160
    Reviewed-by: Caolán McNamara <caolanm at redhat.com>

diff --git a/sdext/source/presenter/PresenterAccessibility.cxx b/sdext/source/presenter/PresenterAccessibility.cxx
index 2c494c058a2b..4bb8e0bed6fe 100644
--- a/sdext/source/presenter/PresenterAccessibility.cxx
+++ b/sdext/source/presenter/PresenterAccessibility.cxx
@@ -451,9 +451,12 @@ public:
 
     void FocusObject (const ::rtl::Reference<PresenterAccessible::AccessibleObject>& rpObject);
 
+    ~AccessibleFocusManager();
+
 private:
     static std::shared_ptr<AccessibleFocusManager> mpInstance;
     ::std::vector<rtl::Reference<PresenterAccessible::AccessibleObject> > maFocusableObjects;
+    bool m_isInDtor = false;
 
     AccessibleFocusManager();
 };
@@ -1810,10 +1813,18 @@ std::shared_ptr<AccessibleFocusManager> const & AccessibleFocusManager::Instance
 }
 
 AccessibleFocusManager::AccessibleFocusManager()
-    : maFocusableObjects()
 {
 }
 
+AccessibleFocusManager::~AccessibleFocusManager()
+{
+    // copy member to stack, then drop it - otherwise will get use-after-free
+    // from AccessibleObject::disposing(), it will call ~Reference *twice*
+    auto const temp(std::move(maFocusableObjects));
+    (void) temp;
+    m_isInDtor = true;
+}
+
 void AccessibleFocusManager::AddFocusableObject (
     const ::rtl::Reference<PresenterAccessible::AccessibleObject>& rpObject)
 {
@@ -1833,7 +1844,7 @@ void AccessibleFocusManager::RemoveFocusableObject (
         maFocusableObjects.erase(iObject);
     else
     {
-        OSL_ASSERT(iObject!=maFocusableObjects.end());
+        OSL_ASSERT(m_isInDtor); // in dtor, was removed already
     }
 }