The International Simutrans Forum

 

Author Topic: [BUG]: Deleting markers as any other player regularly pays refunds to player 1  (Read 299 times)

0 Members and 1 Guest are viewing this topic.

Offline freddyhayward au

  • *
  • Posts: 73
  • Languages: EN
This is quite easy to reproduce: in any game, create several markers using any player other than player 1. Remove these markers and notice the refunds going to player 1 about 1/2 of the time.

After some poking around I noticed some weird logic in simtool.cc where it seems the label is deleted before finding its owner and paying the refund. I'm not familiar with C(++) so perhaps this is not how 'delete' works. Otherwise, could this be the cause?

Code: [Select]
// marker?
if (type == obj_t::label || type == obj_t::undefined) {
if (label_t* l = gr->find<label_t>()) {
msg = l-> is_deletable(player);
if(msg==NULL) {
delete l;
// Refund the cost of land if the player is deleting the marker and therefore selling it.
player_t::book_construction_costs(l->get_owner(), -welt->get_land_value(gr->get_pos()), gr->get_pos().get_2d());
return true;
}
else if(  gr->get_top()==1 || type == obj_t::label ) {
// only complain if this is the last object on this tile ...
return false;
}
msg = NULL;
// not deletable: skip it
}
}

Offline Freahk

  • *
  • Posts: 228
  • Languages: DE, EN
Yep, delete is actually what it states do be. It will delete that object.
In detail, it will call the destructor and will also deallocate the memory.
Behavior of access of a deleted object is undefined and depends on systems implementation of memory management.
Usually that memory remains in programs adress space for an undefined amount of time and won't be reset to 0.
Thus one can often still access such objects without a segfault ant it is relatively likely to get the expected value but there is no guarantee and that likelyhood strongly depends on other lightweight threads operating in the same adressspace.
I don' t need to mention tvat relying on jndefined behavior is a bad idea.
So in short, yes this could be the cause for that bug.

Offline freddyhayward au

  • *
  • Posts: 73
  • Languages: EN
I've made a pull request, though that may not mean much in James' absence.
If you have the time and means to compile could you test whether my patch works as intended?

Offline Freahk

  • *
  • Posts: 228
  • Languages: DE, EN
Didn't you test it o.O
Theres not a high change I will be able to compile and test it today due to fridays release deadline in university being quite close and I still have to do some tasks due to illness in the last week.
However, I will test it tomorrow or sunday and will also finish or at least continue my work on the long pending road removal bug.

If anybody else can test it sooner, I won't be mad if I don't need to test it ;)

Offline freddyhayward au

  • *
  • Posts: 73
  • Languages: EN
Not to worry, I finally figured out how to compile on my machine, and it all works as intended.