From 31632c4fbba3138a4c02668c25d095c6e23b8f68 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 4 Jul 2020 15:53:36 +0200 Subject: [PATCH 1/5] Fix memory leaks in BoxSizer::IncompatibleFlags test case The sizer item allocated by wxSizer::Add() was leaked if an exception was thrown due to the use of invalid flags, resulting in tons of memory leak reports from the leak sanitizer. Fix them by using wxScopedPtr<> for this item and releasing it only if adding the item to the sizer (unexpectedly) did not throw. No real changes. --- tests/sizers/boxsizer.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/sizers/boxsizer.cpp b/tests/sizers/boxsizer.cpp index 575fdcd3b3..3bf4d6aba9 100644 --- a/tests/sizers/boxsizer.cpp +++ b/tests/sizers/boxsizer.cpp @@ -24,6 +24,8 @@ #include "asserthelper.h" +#include "wx/scopedptr.h" + // ---------------------------------------------------------------------------- // test fixture // ---------------------------------------------------------------------------- @@ -367,7 +369,9 @@ TEST_CASE_METHOD(BoxSizerTestCase, "BoxSizer::IncompatibleFlags", "[sizer]") #define ASSERT_SIZER_INVALID_FLAGS(f, msg) \ WX_ASSERT_FAILS_WITH_ASSERT_MESSAGE( \ "Expected assertion not generated for " msg, \ - sizer->Add(10, 10, 0, f) \ + wxScopedPtr item(new wxSizerItem(10, 10, 0, f)); \ + sizer->Add(item.get()); \ + item.release() \ ) #define ASSERT_SIZER_INCOMPATIBLE_FLAGS(f1, f2) \ From 187cbf1efaf9ad67a578e831c6fb90811ede1c59 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 5 Jul 2020 23:15:05 +0200 Subject: [PATCH 2/5] Use CHECK, not ASSERT, in wxSizer::Replace() if item is NULL This is a violation of function preconditions, so don't bother doing anything in this case. --- src/common/sizer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/sizer.cpp b/src/common/sizer.cpp index 33c8d39ce1..ed956e4aea 100644 --- a/src/common/sizer.cpp +++ b/src/common/sizer.cpp @@ -869,7 +869,7 @@ bool wxSizer::Replace( wxSizer *oldsz, wxSizer *newsz, bool recursive ) bool wxSizer::Replace( size_t old, wxSizerItem *newitem ) { wxCHECK_MSG( old < m_children.GetCount(), false, wxT("Replace index is out of range") ); - wxASSERT_MSG( newitem, wxT("Replacing with NULL item") ); + wxCHECK_MSG( newitem, false, wxT("Replacing with NULL item") ); wxSizerItemList::compatibility_iterator node = m_children.Item( old ); From 2f49325d4ca19c0befd66e9c037ef6e0dd2d052a Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 5 Jul 2020 23:18:43 +0200 Subject: [PATCH 3/5] Simplify check for whether wxSizerItem window is null No real changes, just write the check more compactly and without redundant IsWindow() check. --- src/common/sizer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/common/sizer.cpp b/src/common/sizer.cpp index ed956e4aea..e1fbee369a 100644 --- a/src/common/sizer.cpp +++ b/src/common/sizer.cpp @@ -878,8 +878,8 @@ bool wxSizer::Replace( size_t old, wxSizerItem *newitem ) wxSizerItem *item = node->GetData(); node->SetData(newitem); - if (item->IsWindow() && item->GetWindow()) - item->GetWindow()->SetContainingSizer(NULL); + if (wxWindow* const w = item->GetWindow()) + w->SetContainingSizer(NULL); delete item; From 957183ef476f18d972450603c994cea2016bdc07 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 5 Jul 2020 23:19:36 +0200 Subject: [PATCH 4/5] Associate the window with the sizer in wxSizer::Replace() If wxSizerItem passed to Replace() contains a window, the window must be associated with the sizer to ensure that it is uncoupled from it when it is destroyed. Add a simple test which resulted in a use-after-free before but passes now. --- src/common/sizer.cpp | 3 +++ tests/sizers/boxsizer.cpp | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/src/common/sizer.cpp b/src/common/sizer.cpp index e1fbee369a..a0b1768ea1 100644 --- a/src/common/sizer.cpp +++ b/src/common/sizer.cpp @@ -883,6 +883,9 @@ bool wxSizer::Replace( size_t old, wxSizerItem *newitem ) delete item; + if (wxWindow* const w = newitem->GetWindow()) + w->SetContainingSizer(this); + return true; } diff --git a/tests/sizers/boxsizer.cpp b/tests/sizers/boxsizer.cpp index 3bf4d6aba9..69d271b724 100644 --- a/tests/sizers/boxsizer.cpp +++ b/tests/sizers/boxsizer.cpp @@ -444,3 +444,9 @@ TEST_CASE_METHOD(BoxSizerTestCase, "BoxSizer::IncompatibleFlags", "[sizer]") #undef ASSERT_SIZER_INCOMPATIBLE_FLAGS #undef ASSERT_SIZER_INVALID_FLAGS } + +TEST_CASE_METHOD(BoxSizerTestCase, "BoxSizer::Replace", "[sizer]") +{ + m_sizer->AddSpacer(1); + m_sizer->Replace(0, new wxSizerItem(new wxWindow(m_win, wxID_ANY))); +} From 65d890f33ed4fe3ffbf26354a8565b2c502ed1b3 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 5 Jul 2020 23:28:28 +0200 Subject: [PATCH 5/5] Document important requirements of wxSizerItem::AssignWindow() Failure to call SetContainingSizer() on the window passed to it can have catastrophic consequences, so make sure to document the need to do it. --- interface/wx/sizer.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/interface/wx/sizer.h b/interface/wx/sizer.h index 30e7a277fa..41631ff941 100644 --- a/interface/wx/sizer.h +++ b/interface/wx/sizer.h @@ -1098,7 +1098,19 @@ public: /** Set the window to be tracked by this item. - The old window isn't deleted as it is now owned by the sizer item. + @note This is a low-level method which is dangerous if used + incorrectly, avoid using it if possible, i.e. if higher level + methods such as wxSizer::Replace() can be used instead. + + If the sizer item previously contained a window, it is dissociated from + the sizer containing this sizer item (if any), but this object doesn't + have the pointer to the containing sizer and so it's the caller's + responsibility to call wxWindow::SetContainingSizer() on @a window. + Failure to do this can result in memory corruption when the window is + destroyed later, so it is crucial to not forget to do it. + + Also note that the previously contained window is @e not deleted, so + it's also the callers responsibility to do it, if necessary. */ void AssignWindow(wxWindow *window);