Crash can result if townhall is located at top of map and its road is removed

[RandomJerry]

Kept getting a persistent crash in a save of mine in pak64 across a couple of versions of Simutrans (including 124.1). I noticed in the Windows event viewer that the instruction pointer was identical between crashes (indicates that the crash is happening at the same place). I have since investigated further and have managed to find a bug that can result in a crash if the following conditions are true:
1. A townhall is located at the very top of the map (ie, y = 0)
2. The road underneath the townhall is removed

When the townhall is upgraded (if ever), check_bau_townhall() appears to be executed. The function has the following code (line 2501 to 2518 or simcity.cc):

Code Select Expand

if (umziehen) {
// we need to built a new road, thus we will use the old as a starting point (if found)
if (welt->lookup_kartenboden(townhall_road)  &&  welt->lookup_kartenboden(townhall_road)->hat_weg(road_wt)) {
alte_str = townhall_road;
}
else {
koord k = pos + (old_layout==0 ? koord(0, desc_old->get_y()) : koord(desc_old->get_x(),0) );
if (welt->lookup_kartenboden(k)->hat_weg(road_wt)) {
alte_str = k;
}
else {
k = pos - (old_layout==0 ? koord(0, desc_old->get_y()) : koord(desc_old->get_x(),0) );
if (welt->lookup_kartenboden(k)->hat_weg(road_wt)) {
alte_str = k;
}
}
}
}


All nested if statements will evaluate to false except the last which ends up crashing. The last if statement crashes if the townhall is located at the top of the map because:
1. pos.y is dependant on the townhall position and will be zero if the townhall is located at the top of the map
2. Subtracting anything from zero results in a negative number
3. Negative map coordinates are not valid
4. lookup_kartenboden() returns NULL if supplied with invalid map coordinates
5. There is no bounds check to ensure that k is not invalid before calling lookup_kartenboden()
6. There is also no NULL checks outside of lookup_kartenboden()
7. hat_weg() assumes that the this pointer is not NULL and will dereference any NULL pointer

Ultimately, the chain of events looks something like:
1. k is evaluated to invalid map coordinates
2. lookup_kartenboden() returns a NULL pointer after being supplied with k
3. hat_weg() dereferences the NULL pointer and crashes

The code is looking for a road which does not exist. However, it does not appear that check_bau_townhall() expects the missing road to exist and this is reinforced by the comment in the code:
we need to built a new road, thus we will use the old as a starting point (if found)

Instead of "old", read "original road of the townhall" or "existing townhall road". Took me multiple days to realise that.

A simple bounds check (or even a null check, but I think a bounds check would look better) will fix it permanently:

Code Select Expand

// Fix applied (the bounds checks) for the edge case where a townhall is at the very top of the map and the townhall road is demolished
// Previously, (pos - whatever) could result in negative (or invalid) map coordinates given that pos can be zero and given that zero is a valid pos in the world
if (umziehen) {
// we need to built a new road, thus we will use the old as a starting point (if found)
if (welt->lookup_kartenboden(townhall_road)  &&  welt->lookup_kartenboden(townhall_road)->hat_weg(road_wt)) {
alte_str = townhall_road;
}
else {
koord k = pos + (old_layout==0 ? koord(0, desc_old->get_y()) : koord(desc_old->get_x(),0) );
if (welt->is_within_limits(k) && welt->lookup_kartenboden(k)->hat_weg(road_wt)) {
alte_str = k;
}
else {
k = pos - (old_layout==0 ? koord(0, desc_old->get_y()) : koord(desc_old->get_x(),0) );
if (welt->is_within_limits(k) && welt->lookup_kartenboden(k)->hat_weg(road_wt)) {
alte_str = k;
}
}
}
}

I decided to add two bounds checks given that there are two if statements making assumptions (townhall not being at the top or bottom of the map).

[prissi]

A very good find. Not sure how many townhalls were in such a corner as it is often on a slope. Incorporated in r11296