The International Simutrans Forum

 

Author Topic: [r9170] crash when replacing a railway signal with another type  (Read 321 times)

0 Members and 1 Guest are viewing this topic.

Offline THLeaderH

  • Coder/patcher
  • Devotee
  • *
  • Posts: 375
  • Languages: JP,EN
If you try to put a railway signal A on a tile where the railway signal B already exists, simutrans should deny the placement and warn that there are already a signal. However, in r9170, simutrans crashes when a player try to do so. This crash cannot be seen when you try to put a road sign where another type of road signs already exists.

The direct cause of the crash is an assertion failure caused by simmenu.cc:L1092. As far as I know, two_click_t::work() should not be called if two_click_tool_t::is_valid_pos() returns status 0. In this case, two_click_tool_t::is_valid_pos() correctly returns 0 because there is already another railway signal on the tile, but two_click_t::work() is called and that causes the assertion failure.

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2344
Re: [r9170] crash when replacing a railway signal with another type
« Reply #1 on: July 20, 2020, 07:21:50 PM »
Confirmed - investigating

Edit 1: bug introduced in commit 9154
« Last Edit: July 20, 2020, 07:57:21 PM by kierongreen »

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2344
Re: [r9170] crash when replacing a railway signal with another type
« Reply #2 on: July 21, 2020, 08:30:34 AM »
Committed a fix in 9171 - simply removing the assert added in 9154.

There's a distinction here between check_pos and is_valid_pos. tool_t::work should only be called if tool_t::check_pos doesn't return an error. For roadsign_t (which includes signals) there is no check_pos so two_click_tool_t::check_pos is called, which also doesn't exist so tool_t::check_pos is used:
Code: [Select]
const char *tool_t::check_pos(player_t *, koord3d pos )
{
    grund_t *gr = welt->lookup(pos);
    return (gr  &&  !gr->is_visible()) ? "" : NULL;
}
As this just checks whether there's not an invisible ground at this location this returns NULL so work goes ahead which then triggers the assertion when the more detailed checks in is_valid_pos are made.


I believe this was just an oversight when the original patch was made hence am just removing the assert.


As an aside the commit 9154 had the explanation
Code: [Select]
r9154 | dwachs | 2020-06-25 17:47:25 +0100 (Thu, 25 Jun 2020) | 1 line

FIX: correctly null error messages in case of success (link factory tool affected)

It's worth noting however that in case of link factory tool failure currently no error message is generated - possibly something like below would handle cases when it's not used on a factory, however it might be desired for an error message to be displayed when a link cannot be formed or removed as well.
Code: [Select]
===================================================================
--- simtool.cc    (revision 9170)
+++ simtool.cc    (working copy)
@@ -5705,7 +5705,7 @@
 {
     fabrik_t *fab = fabrik_t::get_fab( pos.get_2d() );
     if (fab == NULL) {
-        error = "";
+        error = "No factory";
         return 0;
     }
     return 2;

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4711
  • Languages: EN, DE, AT
Re: [r9170] crash when replacing a railway signal with another type
« Reply #3 on: July 22, 2020, 05:56:43 AM »
The assert was there for a reason: to enforce that is_valid_pos gives consistent return values. I do not see anything wrong with the code in is_valid_pos, will check.

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2344
Re: [r9170] crash when replacing a railway signal with another type
« Reply #4 on: July 22, 2020, 08:26:33 AM »
To ensure that is_valid_pos returns consistent values it needs to be either returning error == NULL and value > 0 or errror != NULL and value == 0. To enforce that you could use assert((error == NULL)  ==  (value > 0))

This would be truth table:
error == NULLerror != NULL
value == 0falsetrue
value > 0truefalse

With assert(error == NULL  ||  value > 0) we get this truth table though:
error == NULLerror != NULL
value == 0truefalse
value > 0truetrue

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4711
  • Languages: EN, DE, AT
Re: [r9170] crash when replacing a railway signal with another type
« Reply #5 on: July 22, 2020, 08:33:21 AM »
Yes, you are right. This assert was completely stupid.

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2344
Re: [r9170] crash when replacing a railway signal with another type
« Reply #6 on: July 22, 2020, 08:37:02 AM »
Was a good idea in theory!