Opened 2 years ago

Closed 21 months ago

Last modified 21 months ago

#14563 closed defect (fixed)

Crash when Disconnect()ing event handler without specifying function

Reported by: cwalther Owned by:
Priority: normal Milestone: 2.9.5
Component: base Version: 2.9.4
Keywords: wxEvtHandler Disconnect wxEventConnectionRef Cc:
Blocked By: Blocking:
Patch: yes

Description

After this sequence

source->Connect(wxEVT_SOMETHING, wxSomethingEventHandler(MySink::OnSomething), NULL, sink);
source->Disconnect(wxID_ANY, wxEVT_SOMETHING));
delete source;

destruction of sink crashes because wxEventConnectionRef::OnObjectDestroy tries to call a method on the already destroyed source.

The cause is that calling Disconnect() without specifying the function (letting it figure it out on its own, taking the first match for wxEVT_SOMETHING) fails to call DecRef() on the wxEventConnectionRef connecting sink and source, so that it erroneously still exists at the time sink is destroyed.

I realize this is a somewhat exotic situation, as the problem can be easily worked around by passing the function to Disconnect() too, and Connect/Disconnect is supposed to be replaced by Bind/Unbind now, but as far as I understand the documentation, this should work. I just ran into this problem while porting an application to wxWidgets 2.9 that worked fine in 2.6.

To reproduce: apply the attached disconnectcrash-unittest.patch to the events/evthandler unit test and run test EvtHandlerTestCase. Expected result: tests should pass. Actual result: EvtHandlerTestCase::DisconnectWildcard() crashes on line evthandler.cpp:265 (at least when run on a C library that overwrites freed memory, such as in a debug build on Windows (MSVC10) - it didn’t crash for me on Linux, but even there stepping through with a debugger clearly shows that a method is being called on a destroyed object).

It seems to me this could be fixed by the attached disconnectcrash.patch, but I am completely unfamiliar with wxEventConnectionRef etc., so I’d appreciate a thorough review. In addition to fixing the above crash, it also makes sure that wxEventConnectionRef::DecRef() is not called when the loop over m_dynamicEvents doesn’t find anything to disconnect (Disconnect() will return false). Otherwise, such unmatching Disconnect() calls could cause OnSinkDestroyed not to be called. I added a test case for that too (EvtHandlerTestCase::AutoDisconnect).

Attachments (2)

disconnectcrash-unittest.patch download (1.9 KB) - added by cwalther 2 years ago.
unit tests that currently fail/crash
disconnectcrash.patch download (1.3 KB) - added by cwalther 2 years ago.
proposed fix

Download all attachments as: .zip

Change History (7)

Changed 2 years ago by cwalther

unit tests that currently fail/crash

Changed 2 years ago by cwalther

proposed fix

comment:1 Changed 2 years ago by vadz

  • Milestone set to 2.9.5

Thanks for the patches, I don't have time to look at them right now but we definitely should do something about this and I'll try to do it before 2.9.5.

If anybody else can review this, it would be very welcome too, of course!

comment:2 Changed 21 months ago by vadz

  • Status changed from new to confirmed

The trouble is that I still get bug reports from valgrind even after applying the patch:

% valgrind ./test EvtHandler
==11042== Memcheck, a memory error detector
==11042== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==11042== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info
==11042== Command: ./test EvtHandler
==11042==
Test program for wxWidgets non-GUI features
build: 2.9.5 (wchar_t,compiler with C++ ABI 1002,wx containers,compatible with 2.8)
...==11042== Invalid read of size 8
==11042==    at 0x577C962: wxListBase::GetFirst() const (list.h:551)
==11042==    by 0x577CA3B: wxObjectList::GetFirst() const (list.h:1194)
==11042==    by 0x58A1BD5: wxEvtHandler::OnSinkDestroyed(wxEvtHandler*) (event.cpp:1799)
==11042==    by 0x58A2BFB: wxEventConnectionRef::OnObjectDestroy() (event.h:3675)
==11042==    by 0x58A227C: wxTrackable::~wxTrackable() (tracker.h:84)
==11042==    by 0x589FC8F: wxEvtHandler::~wxEvtHandler() (event.cpp:1152)
==11042==    by 0x4F7C54: (anonymous namespace)::MyHandler::~MyHandler() (evthandler.cpp:101)
==11042==    by 0x4F8795: EvtHandlerTestCase::DisconnectWildcard() (evthandler.cpp:262)
==11042==    by 0x501753: CppUnit::TestCaller<EvtHandlerTestCase>::runTest() (TestCaller.h:166)
==11042==    by 0x4E54406: CppUnit::TestCaseMethodFunctor::operator()() const (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4277EF: wxUnitTestProtector::protect(CppUnit::Functor const&, CppUnit::ProtectorContext const&) (test.cpp:202)
==11042==    by 0x4E50278: CppUnit::ProtectorChain::ProtectFunctor::operator()() const (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==  Address 0x6d24328 is 24 bytes inside a block of size 48 free'd
==11042==    at 0x4C23E0F: operator delete(void*) (vg_replace_malloc.c:387)
==11042==    by 0x50A8A2C: wxList::~wxList() (list.h:1208)
==11042==    by 0x589FEA2: wxEvtHandler::~wxEvtHandler() (event.cpp:1140)
==11042==    by 0x4F8746: EvtHandlerTestCase::DisconnectWildcard() (evthandler.cpp:262)
==11042==    by 0x501753: CppUnit::TestCaller<EvtHandlerTestCase>::runTest() (TestCaller.h:166)
==11042==    by 0x4E54406: CppUnit::TestCaseMethodFunctor::operator()() const (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4277EF: wxUnitTestProtector::protect(CppUnit::Functor const&, CppUnit::ProtectorContext const&) (test.cpp:202)
==11042==    by 0x4E50278: CppUnit::ProtectorChain::ProtectFunctor::operator()() const (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4E467D3: CppUnit::DefaultProtector::protect(CppUnit::Functor const&, CppUnit::ProtectorContext const&) (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4E50278: CppUnit::ProtectorChain::ProtectFunctor::operator()() const (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4E4FFBB: CppUnit::ProtectorChain::protect(CppUnit::Functor const&, CppUnit::ProtectorContext const&) (in /usr/lib/libcppunit-1.12.so.1.0.0)
==11042==    by 0x4E5BD9F: CppUnit::TestResult::protect(CppUnit::Functor const&, CppUnit::Test*, std::string const&) (in /usr/lib/libcppunit-1.12.so.1.0.0)

I didn't yet have time to look at this though, if you have any idea about why am I still getting this, it would be more than welcome.

TIA!

comment:3 Changed 21 months ago by vadz

I have no idea what happened but I can't reproduce the problem under valgrind any more. I also ran it under Intel Inspector and it didn't find anything wrong neither so it looks like I had done something wrong the last time, so I'll just commit your patches now.

Thanks again for fixing the bug and providing the unit tests for it!

comment:4 Changed 21 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [72943]) Fix crashes after using "wildcard" wxEvtHandler::Disconnect().

When not specifying the function to disconnect, the associated event sink was
destroyed too early resulting in crashes later. Fix this and add unit tests
verifying that things work as expected and at least don't crash.

Closes #14563.

comment:5 Changed 21 months ago by cwalther

Thanks! As you may have guessed from my inactivity, I haven’t gotten around to looking into your valgrind error. If it was just a mistake on your part, all the better. My guess would be that you used a build of the library that didn’t have the patch applied.

Note: See TracTickets for help on using tickets.