Segmentation fault during combat

[excruciated]

A segfault predictably happens whenever I enter combat. I just built revision 6601 today on Arch. This did not happen with the revision I built a week ago.

MilitarySite::swapSoldiers: error: Unknown player preference 0.
MilitarySite::swapSoldiers: error: Unknown player preference 0.
MilitarySite::swapSoldiers: error: Unknown player preference 0.
MilitarySite::swapSoldiers: error: Unknown player preference 0.
...
...
MilitarySite::swapSoldiers: error: Unknown player preference 0.
MilitarySite::swapSoldiers: error: Unknown player preference 0.
MilitarySite::swapSoldiers: error: Unknown player preference 0.
MilitarySite::swapSoldiers: error: Unknown player preference 0.
Cmd_EnemyFlagAction::execute player(1): flag->owner(2) number=10
MilitarySite::swapSoldiers: error: Unknown player preference 0.
MilitarySite::swapSoldiers: error: Unknown player preference 0.
MilitarySite::swapSoldiers: error: Unknown player preference 0.
Segmentation fault (core dumped)

I would think it has to do with the recent "military site soldier preference selection by Teppo" in rev. 6600.

[Johannes E. Krause (krause-f)]

In the attached savegame, when i keep attacking the other player, i get this access violation. it does not seem to happen without attacking.

the game was started with r6601

Last lines of output and backtrace:
Forcing flag at (18, 2)
Cmd_EnemyFlagAction::execute player(1): flag->owner(2) number=4

Program received signal SIGSEGV, Segmentation fault.
0x00000000007ad970 in Widelands::Soldier::get_level(Widelands::tAttribute) const ()
(gdb) bt
#0 0x00000000007ad970 in Widelands::Soldier::get_level(Widelands::tAttribute) const ()
#1 0x00000000005f9062 in Widelands::MilitarySite::update_upgrade_requirements() ()
#2 0x00000000005f913b in Widelands::MilitarySite::update_upgrade_soldier_request() ()
#3 0x00000000005f95e9 in Widelands::MilitarySite::update_soldier_request(bool) ()
#4 0x00000000005f9c6e in Widelands::MilitarySite::act(Widelands::Game&, unsigned int) ()
#5 0x000000000078c2aa in Widelands::Cmd_Act::execute(Widelands::Game&) ()
#6 0x0000000000601168 in Widelands::Cmd_Queue::run_queue(int, int&) ()
#7 0x00000000005ef02f in Widelands::Game::think() ()
#8 0x0000000000678740 in Interactive_Base::think() ()
#9 0x0000000000636a85 in Interactive_Player::think() ()
#10 0x00000000006995d9 in UI::Panel::run() ()
#11 0x00000000005f4da9 in Widelands::Game::run_load_game(std::string) ()
#12 0x000000000055a95f in WLApplication::load_game() ()
#13 0x000000000055b2bc in WLApplication::mainmenu_singleplayer() ()
#14 0x000000000055b978 in WLApplication::mainmenu() ()
#15 0x000000000055bf0b in WLApplication::run() ()
#16 0x000000000054ee12 in main ()

[Hans Joachim Desserud (hjd)]

Hi excruciated and welcome to Launchpad!

This bug seems to be triggered in battle sometimes, though I am not exactly sure why. Pure speculation: might be when one of your attackers die and hq wants to send reinforcements or something?

Anyways, it is moderately easy to reproduce on Crater. Just rush the enemy and after a few attacks back and forth it will trigger. Can't really give more detailed steps than that I'm afraid.

I've attached the backtrace I got and it seems identical with the one Johannes pasted above.

[excruciated (excruciated)]

How did you see that backtrace? Did you compile Wl as Debug? I'd like to know for the future.

[Hans Joachim Desserud (hjd)]

I ran widelands in gdb (the GNU debugger). Basically you run wl through the debugger rather than executing it directly, which gives you access to a lot more debug information than normally. You can run this on any wl build, though a debug build will give you more information.

What I did:
1. Run "gdb widelands" in a terminal and wait for it to load.
2. You should get a gdb prompt, in other words the line starts with (gdb)
3. Here you can run commands, for instance "run" which will start widelands
4. Play it as normally, and when it crashes you can run "backtrace" in the debugger, printing similar output to the one above.

There also a lot other nifty things gdb can do, but I only know the basics I'm afraid. Oh, and most commands have shortcuts, for instance r=run, bt=backtrace which saves typing.

[Teppo Mäenpää (kxq)]

I guess that this is my bad. Guess: null pointer dereferenced at militarysite.cc line 1053. Should happen only with bad timing during battle. It should be safe for the function to just return false if "Soldier * worst_guy" equals to null. I do not have the environment to touch the code next few days. Could somebody please add a null pointer check between lines 1052 and 1053 and retry? Thanks!

[cghislai (charlyghislain)]

Replacing with

Soldier * worst_guy = find_least_suited_soldier();
 if (worst_guy == NULL) {
  log("MilitarySite::swapSoldiers: error: Could not find least suited soldier");
  return false;
 }
 int32_t wg_level = worst_guy->get_level(atrTotal);

does indeed produce the error output and prevent crash.

[Teppo Mäenpää (kxq)]

Thanks for the quick debugging.

Missing null pointer check is the bug.

When a site is being successfully conquered, it is a militarysite without any soldiers present for some time. It is OK that find_least_suited_soldier() returns null at that moment. Therefore, the pointer being null is not an error itself.

I would recommend that whoever gets this bug report assigned, does the fix of Charly's, without the log() -statement. I could do that next week. Some people reading this forum might be able to make a patch to this critical bug sooner.

[SirVer (sirver)]

I pushed a fix in r6605. Quite the team effort here - thanks for the patch Charly, for the idea Teppo, for the verification hjd and the report excruciated.

[SirVer (sirver)]

I forgot johannes who provided the savegame which would have been useful if teppo didn't had a hunch. Thanks for this as well :)

[SirVer (sirver)]

Released in build-18 rc1.