News:

Simutrans.com Portal
Our Simutrans site. You can find everything about Simutrans from here.

[BUG]: Deleting markers as any other player regularly pays refunds to player 1

Started by freddyhayward, November 07, 2019, 06:58:48 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

freddyhayward

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?

// 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
}
}

Mariculous

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.

freddyhayward

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?

Mariculous

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 ;)

freddyhayward

Not to worry, I finally figured out how to compile on my machine, and it all works as intended.

jamespetts

Thank you very much for this - I have now fixed this. I should be grateful if you could re-test with the next nightly build.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.