Merge lp:~widelands-dev/widelands/bug-1807156-heap-use-after-free into lp:widelands

[GunChleoc]

Ensure that interactive base still exists when toggling building help window

This will potentially fix a heap-use-after-free

I could not reproduce the original bug, so this fix is just my best guess.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4517. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/497694161.
Appveyor build 4304. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1807156_heap_use_after_free-4304.

[Klaus Halfmann (klaus-halfmann)]

Mhh, in case (building_in_lambda == nullptr) you return;
In case (parent_ != nullptr) you fall through.

This is in create_capsbuttons() but I dont see this in the stacktrace?

Any idea how to test this?

[GunChleoc (gunchleoc)]

The reported line is:

./src/wui/buildingwindow.cc:359

which is

Widelands::Building* building_in_lambda = building_.get(parent_->egbase());

and my guess is that parent_ has become nullptr for some reason.

No idea how to test his - I did not manage to trigger the bug, but maybe somebody else will have more creative ideas.

[Klaus Halfmann (klaus-halfmann)]

OK merged trunk. I get some

../src/wlapplication.cc:1476:30: warning: unused exception parameter 'e'
[-Wunused-exception-parameter]
        } catch (const FileError& e) {

will now use a debugger to poke into that code.

[GunChleoc (gunchleoc)]

Please do not report unrelated issues in merge requests - it makes the merge request noisy with unrelated discussions. Create a bug for it ;)

I have fixed this one directly in trunk, thanks for reporting!

[Klaus Halfmann (klaus-halfmann)]

OK, his is te code that will (new in R20) add the Helpbutton
so the correct helpwindow for the building in progress will be shown.

_parent is the same as igbase() which is used at the very start of the function,
so I have no idea how this should be come null?

Teh origiinal bug is about some heap-use-after-free, in this case the prt
would be != null but be freed before?

I see no operator() (e.g. from unique Pointer) involved?

So my bues guess is that this _may_ be a help window for some consntruction side,
that is open longer then the original building window().

Ill use this branch to try to provoke such conditions.

review: Needs Information

[Klaus Halfmann (klaus-halfmann)]

I quick game for some 40 minutes, found nothing,
will try to produce this in some coop game perhaps.

[GunChleoc (gunchleoc)]

We decided in the bug to merge this, since it doesn't do any harm

@bunnybot merge

=== modified file 'src/wui/buildingwindow.cc'
--- src/wui/buildingwindow.cc	2019-02-23 11:00:49 +0000
+++ src/wui/buildingwindow.cc	2019-03-05 09:06:17 +0000
@@ -356,12 +356,14 @@
 		UI::UniqueWindow::Registry& registry =
 		   igbase()->unique_windows().get_registry(building_descr_for_help_.name() + "_help");
 		registry.open_window = [this, &registry] {
-			Widelands::Building* building_in_lambda = building_.get(parent_->egbase());
-			if (building_in_lambda == nullptr) {
-				return;
+			if (parent_ != nullptr) {
+				Widelands::Building* building_in_lambda = building_.get(parent_->egbase());
+				if (building_in_lambda == nullptr) {
+					return;
+				}
+				new UI::BuildingHelpWindow(igbase(), registry, building_descr_for_help_,
+										   building_in_lambda->owner().tribe(), &parent_->egbase().lua());
 			}
-			new UI::BuildingHelpWindow(igbase(), registry, building_descr_for_help_,
-			                           building_in_lambda->owner().tribe(), &parent_->egbase().lua());
 		};
 
 		helpbtn->sigclicked.connect(