memory corruption issue when closing down factories

[Philip]

In the SVN version, the factory close-down code calls rem_fab from inside a loop iterating over fab_list, causing memory corruption and random crashes:

#0  karte_ab (this=0xf239be00, fab=0xe4d07418) at simworld.cc:3099
#1  0x0805ba47 in hausbauer_t::remove (welt=0xf239be00, sp=0xe7974588, gb=
    0xed0fff20) at bauer/hausbauer.cc:367
#2  0x0828e3df in fabrik_t::neuer_monat (this=0xe4d07418) at simfab.cc:2277
#3  0x0830661a in karte_t::new_month (this=0xf239be00) at simworld.cc:3732
#4  0x083080a6 in karte_t::step (this=0xf239be00) at simworld.cc:4160
#5  0x08314f31 in karte_t::interactive (this=0xf239be00, quit_month=2147483647)
    at simworld.cc:7268
#6  0x082bf5eb in simu_main (argc=1, argv=0xffffd7b4) at simmain.cc:1259
#7  0x082cd27c in sysmain (argc=1, argv=0xffffd7b4) at simsys.cc:703
#8  0x083611c2 in main (argc=1, argv=0xffffd7b4) at simsys_s.cc:723

I think the right solution is to remove the call to rem_fab from hausbauer_t::remove, and add code to rem_fab all the factories in closed_factories_this_month to karte_t::new_month; but even with that change, the industry list window causes segfaults.

[jamespetts]

Hello - welcome to the forums, and thank you very much for your report.

However, I am a little confused: you refer to the SVN version, yet Simutrans-Experimental does not have an SVN version: only Standard uses SVN. Are you referring to Github? If so, to which branch?

Also, is this reproduced when deleting factories manually as the public player, or only when they are deleted automatically?

Thank you again for your report - it is always good to see new people on the forum!

[Philip]

Sorry for the confusion, I meant to say the GIT version; the branch I am using is https://github.com/jamespetts/simutrans-experimental, but I wasn't aware there are others. That GIT version's readme.txt also refers to an svn version, which is a little confusing.

Removing the rem_fab call from hausbauer_t::remove does indeed break public player removal (which no longer removes the factory from fab_list since it goes through wkz_remover_intern), so it's a little harder to fix than I thought initially: Here's one idea:

Code Select Expand



diff --git a/simfab.cc b/simfab.cc
index 8c82191..a22340b 100644
--- a/simfab.cc
+++ b/simfab.cc
@@ -2274,7 +2274,6 @@ void fabrik_t::neuer_monat()
                        }

                        welt->closed_factories_this_month.append(this);
-                       hausbauer_t::remove(welt, welt->get_spieler(1), gb);
                }
        }
        // NOTE: No code should come after this part, as the closing down code may cause this object to be deleted.
diff --git a/simworld.cc b/simworld.cc
index 0ec2275..64d43f6 100644
--- a/simworld.cc
+++ b/simworld.cc
@@ -3735,6 +3735,15 @@ void karte_t::new_month()
                }
        }

+       FOR(vector_tpl<fabrik_t*>, const fab, closed_factories_this_month)
+       {
+               if(fab_list.is_contained(fab)) {
+                       grund_t *gr = lookup(fab->get_pos());
+                       gebaeude_t* gb = gr->find<gebaeude_t>();
+                       
+                       hausbauer_t::remove(this, get_spieler(1), gb);
+               }
+       }
        // Check to see whether more factories need to be added
        // to replace ones that have closed.
        // @author: jamespetts



We need the ugly fab_list.is_contained check because a factory might be orphaned and marked for closing at the same time, in which case the pointer is invalid by the time we reach it in closed_factories_this_month. In theory, if the factory destroy code ever were to allocate new factories (would that make sense?), one of them might end up in closed_factories_this_month by accident, and get destroyed immediately; if this is a concern, we should probably check the factory's build date to make sure it isn't the current month.

That patch seems to do the right thing, though I haven't tested the nasty corner cases (more than one factory closing down in the same month is hard to achieve).

[jamespetts]

Thank you very much - that is very helpful. I have now implemented this on the 11.x branch.