[passenger-generation] tricky segfault caused by hausbauer.cc

[Philip]

In trying to make the hausbauer code behave and build up/down according to our preliminary consensus, I've run into a strange bug in gebaeude.cc, which I understand but haven't fully worked out how to fix. The problem is the gebaeude_t::check_road_tiles code, which marks and unmarks connected streets based on their 3D position; if the position changes in between the two calls, data corruption and a segfault might occur.

This is relevant in hausbauer_t::baue, which creates a gebaeude at one position (calling the gebaeude_t constructor and thus making the first call to check_road_tiles), then builds a fundament and changes gb's position by calling gb->set_pos(). Ideally, gb->set_pos() would call check_road_tiles again, but it's not a virtual method (nor does it need to be to override set_pos() for cases where it isn't called as a ding_t method).

The good news is hausbauer_t::baue appears to be the only place where gb->set_pos is called, and we can avoid that call by reordering some of the code:

Code Select Expand


diff --git a/bauer/hausbauer.cc b/bauer/hausbauer.cc
index 9779b8b..2b6a429 100644
--- a/bauer/hausbauer.cc
+++ b/bauer/hausbauer.cc
@@ -451,19 +451,6 @@ gebaeude_t* hausbauer_t::baue(karte_t* welt, spieler_t* sp, koord3d pos, int org
DBG_MESSAGE("hausbauer_t::baue()","get_tile() empty at %i,%i",k.x,k.y);
continue;
}
- gebaeude_t *gb = new gebaeude_t(welt, pos + k, sp, tile);
- if (first_building == NULL) {
- first_building = gb;
- }
-
- if(besch->ist_fabrik()) {
- gb->set_fab((fabrik_t *)param);
- }
- // try to fake old building
- else if(welt->get_zeit_ms() < 2) {
- // Hajo: after staring a new map, build fake old buildings
- gb->add_alter(10000ll);
- }
grund_t *gr;
/*if(besch->get_allow_underground() == 1)
{
@@ -475,15 +462,14 @@ gebaeude_t* hausbauer_t::baue(karte_t* welt, spieler_t* sp, koord3d pos, int org
{*/
gr = welt->lookup_kartenboden(pos.get_2d() + k);
//}
+
+ leitung_t *lt = NULL;
if(gr->ist_wasser()) {
- gr->obj_add(gb);
} else if (besch->get_utyp() == haus_besch_t::hafen) {
// it's a dock!
- gr->obj_add(gb);
}
else {
// very likely remove all
- leitung_t *lt = NULL;
if(!gr->hat_wege()) {
lt = gr->find<leitung_t>();
if(lt) {
@@ -570,17 +563,30 @@ gebaeude_t* hausbauer_t::baue(karte_t* welt, spieler_t* sp, koord3d pos, int org
welt->access(gr->get_pos().get_2d())->boden_ersetzen(gr, gr2);
gr = gr2;
//DBG_DEBUG("hausbauer_t::baue()","ground count now %i",gr->obj_count());
- gr->obj_add( gb );
- if(lt) {
- gr->obj_add( lt );
- }
- if(needs_ground_recalc  &&  welt->is_within_limits(pos.get_2d()+k+koord(1,1))  &&  (k.y+1==dim.y  ||  k.x+1==dim.x)) {
- welt->lookup_kartenboden(pos.get_2d()+k+koord(1,0))->calc_bild();
- welt->lookup_kartenboden(pos.get_2d()+k+koord(0,1))->calc_bild();
- welt->lookup_kartenboden(pos.get_2d()+k+koord(1,1))->calc_bild();
- }
}
- gb->set_pos( gr->get_pos() );
+
+ gebaeude_t *gb = new gebaeude_t(welt, gr->get_pos(), sp, tile);
+ if (first_building == NULL) {
+ first_building = gb;
+ }
+
+ if(besch->ist_fabrik()) {
+ gb->set_fab((fabrik_t *)param);
+ }
+ // try to fake old building
+ else if(welt->get_zeit_ms() < 2) {
+ // Hajo: after staring a new map, build fake old buildings
+ gb->add_alter(10000ll);
+ }
+ gr->obj_add( gb );
+ if(lt) {
+ gr->obj_add( lt );
+ }
+ if(needs_ground_recalc  &&  welt->is_within_limits(pos.get_2d()+k+koord(1,1))  &&  (k.y+1==dim.y  ||  k.x+1==dim.x)) {
+ welt->lookup_kartenboden(pos.get_2d()+k+koord(1,0))->calc_bild();
+ welt->lookup_kartenboden(pos.get_2d()+k+koord(0,1))->calc_bild();
+ welt->lookup_kartenboden(pos.get_2d()+k+koord(1,1))->calc_bild();
+ }
if(besch->ist_ausflugsziel()) {
welt->add_ausflugsziel( gb );
}



The other problem is that buildings won't be connected if built in the wrong spot, if all roads facing them are at different levels. That doesn't appear to happen with the old code...

(I'm in the process of setting up a git repository, but haven't quite figured out git yet.)

[jamespetts]

Thank you for that. I have now added this to my passenger-generation branch. However, I wonder whether this might cause trouble: I get an assertion failure a number of times over in simhalt.cc line 3775 on loading the Pak128.Britain-Ex demo.sve.

As to the second issue, the solution is probably not to have buildings surrounded entirely by tiles of a different level: this does not, after all, happen in reality.

[Philip]

I don't know about that—here's an example with a house that would now be unreachable, I think. Note this could happen well above sea level, too, and it could even affect diagonal neighbours, as in the second image.

[jamespetts]

The interesting thing is in those examples is that one edge of the road is on the correct tile. I wonder whether there is a way to check for that?

[Philip]

I've run into a very similar bug to this one: when rebuilding townhalls, check_road_tiles(true) gets called on each tile, but some of them might already have been deleted, so it doesn't deregister roads connecting to previously-deleted tiles.

Another case is not the building changing height, but the connecting road: that can happen when a player (or the city construction code) raises or lowers ground with an end-piece of road on it. To make things more vexing still, it's also possible to connect to roads in tunnels (should it be?), and those can change height, too.

I've attached a patch that should work as a work-around. (pull req at https://github.com/jamespetts/simutrans-experimental/pull/12 )

[jamespetts]

Thank you - that is most helpful.  Now incorporated. As to tunnels, I do not think that these should count as connecting a building. Does your patch cause them so to count? If so, we need to consider changing that. Thank you for spotting that issue - that is helpful.