Author Topic: Crash on placing stop on nonowned road  (Read 171 times)

0 Members and 1 Guest are viewing this topic.

Offline TurfIt

Crash on placing stop on nonowned road
« on: July 23, 2017, 11:02:35 PM »
Only with self built executable - msys2 Mingw-w64. Start a new map, place a bus stop on any city or intercity road - crash. Build on a player or public owned road, ok.

gdb works about as uselessly as usual - it even crashes with this one!
Code: [Select]
Thread 1 received signal SIGSEGV, Segmentation fault.
player_t::is_public_serivce (

and when gdb doesn't crash:
Code: [Select]
Thread 1 received signal SIGSEGV, Segmentation fault.
0x005c755e in ?? ()
(gdb) bt
#0  0x005c755e in ?? ()
#1  0x79616c70 in ?? ()
#2  0x2d207265 in ?? ()
#3  0xbaad0031 in ?? ()
#4  0xbaadf00d in ?? ()
#5  0xbaadf00d in ?? ()
#6  0xbaadf00d in ?? ()
#7  0xbaadf00d in ?? ()
#8  0xbaadf00d in ?? ()
#9  0xbaadf00d in ?? ()
#10 0xbaadf00d in ?? ()
#11 0xbaadf00d in ?? ()
#12 0xbaadf00d in ?? ()
#13 0xbaadf00d in ?? ()
#14 0xbaadf00d in ?? ()
#15 0xbaadf00d in ?? ()
#16 0xbaadf00d in ?? ()
#17 0xbaadf00d in ?? ()
#18 0xbaadf00d in ?? ()
#19 0xbaadf00d in ?? ()
#20 0xbaadf00d in ?? ()
#21 0xbaadf00d in ?? ()
#22 0xbaadf00d in ?? ()
#23 0xbaadf00d in ?? ()
#24 0xbaadf00d in ?? ()
#25 0xbaadf00d in ?? ()
#26 0xbaadf00d in ?? ()
#27 0xbaadf00d in ?? ()
#28 0xbaadf00d in ?? ()
#29 0xbaadf00d in ?? ()
#30 0xbaadf00d in ?? ()
#31 0xbaadf00d in ?? ()
#32 0xbaadf00d in ?? ()
#33 0xbaadf00d in ?? ()
#34 0xbaadf00d in ?? ()
#35 0xbaadf00d in ?? ()
#36 0xbaadf00d in ?? ()
#37 0xbaadf00d in ?? ()
#38 0xbaadf00d in ?? ()
#39 0xbaadf00d in ?? ()
#40 0xbaadf00d in ?? ()
#41 0xbaadf00d in ?? ()
#42 0xbaadf00d in ?? ()
#43 0xbaadf00d in ?? ()
#44 0xbaadf00d in ?? ()
#45 0xbaadf00d in ?? ()
#46 0xbaadf00d in ?? ()
#47 0xbaadf00d in ?? ()
#48 0xbaadf00d in ?? ()
#49 0xbaadf00d in ?? ()
#50 0xbaadf00d in ?? ()
#51 0xbaadf00d in ?? ()
#52 0xbaadf00d in ?? ()
#53 0xbaadf00d in ?? ()
#54 0xbaadf00d in ?? ()
#55 0xbaadf00d in ?? ()
#56 0xbaadf00d in ?? ()
#57 0xbaadf00d in ?? ()
#58 0xbaadf00d in ?? ()
#59 0xbaadf00d in ?? ()
#60 0xbaadf00d in ?? ()
#61 0xbaadf00d in ?? ()
#62 0xbaadf00d in ?? ()
#63 0xbaadf00d in ?? ()
#64 0xbaadf00d in ?? ()
#65 0x181d46a8 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb)

Perhaps not much to go on, what's different between placing a stop on a city road verses ones own road?
Or maybe gdb crashing on player_t::is_public_serivce (   is a clue...
Code: [Select]
bool player_t::is_public_serivce() const
 {
if (!this)
WTF?
WTFF? (doubled).

and sure enough one of the thousands of ignored warnings gcc spits out compiling Extended:
Code: [Select]
player/simplay.cc: In member function 'bool player_t::is_public_serivce() const':
player/simplay.cc:241:2: warning: nonnull argument 'this' compared to NULL [-Wnonnull-compare]
  if (!this)
  ^~

and the culprit:
Code: [Select]
const char *weg_t:: is_deletable(const player_t *player, bool allow_public)
{
if(allow_public && get_owner()->is_public_serivce())
The owner of a city road is indeed null,   why  null->is_public_serivce() doesn't crash right here.... lovely undefined behaviour...

Fix: check is there's actually an owner before checking if the owner is public player.
      also fix the same construct in bridges, crossings, halts, vehicles.
  then how about consolidate is_public_serivce() with is_public_service().
  finally get rid of the other half dozen or so  if(this) constructs.... they're just masking something broken upstream.
  and really finally maybe compile with gcc for once and exorcise  the warnings some of which do indeed point at bad things.

Offline jamespetts

  • Simitrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 15419
  • Total likes: 376
  • Helpful: 171
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Crash on placing stop on nonowned road
« Reply #1 on: July 24, 2017, 12:08:48 AM »
Thank you for your report and analysis - that is most helpful. I could not reproduce this crash in a Visual Studio build, but I have replaced the if(!this) construct in is_public_service() with a check in each of the calling methods as you suggest. However, there is only one method called is_public_service(), so I am not sure what you mean when you write of merging is_public_service() with is_public_service() - can you elaborate? (I have to say, I was not hitherto aware that if(!this) was incorrect - being self taught at C++, sometimes I miss some of the obscurer language rules; it seems like the sort of thing that ought to be possible, and does seem to work in at least some cases).

I was only able to find one other if(!this) construct (you referred to half a dozen others - did you mean six or just "at least one"?), which is as follows:

Code: [Select]
if(file->get_version() > 110005)
{
if(file->is_saving())
{
if(!self.is_bound())
{
// Something has gone a bit wrong here, as the handle to self is not bound.
if(!this)
{
// Probably superfluous, but best to be sure that this is really not a dud pointer.
dbg->error("void haltestelle_t::rdwr(loadsave_t *file)", "Handle to self not bound when saving a halt");
return;
}
if(self.get_rep() != this)
{
uint16 id = self.get_id();
self = halthandle_t(this, id);
}
}
uint16 halt_id = self.is_bound() ? self.get_id() : 0;
file->rdwr_short(halt_id);
}

This looks as though it is in a block of code that is intended never to be called unless something has already gone significantly wrong. In those circumstances, do you think it harmful for it to remain?

Thank you again for your testing.
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.

Offline TurfIt

Re: Crash on placing stop on nonowned road
« Reply #2 on: July 24, 2017, 01:05:07 AM »
However, there is only one method called is_public_service(),
and there's another one called is_public_serivce().   look close!


(I have to say, I was not hitherto aware that if(!this) was incorrect - being self taught at C++, sometimes I miss some of the obscurer language rules; it seems like the sort of thing that ought to be possible, and does seem to work in at least some cases).
You're deep into undefined territory there. MSVC is leading you down a path with a noose at the end. Standard gets the benefit of a couple more people passing the code through their choice of compilers to purge this type of thing... you'd need to try multiple compilers yourself on occasion so you don't end up with MSVC specific stuff in Extended.  Likely this type of undefined behaviour is the ultimate source of the desync problem going on with linux(gcc) and windows (msvc)...


I was only able to find one other if(!this) construct (you referred to half a dozen others - did you mean six or just "at least one"?), which is as follows:
32 nonnull compare warnings trigger during the build - I quickly scanned them and thought I seen 6 unique. There's actually 7. Attached is warning log - suggest you get rid of all 7.

Offline jamespetts

  • Simitrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 15419
  • Total likes: 376
  • Helpful: 171
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Crash on placing stop on nonowned road
« Reply #3 on: July 24, 2017, 11:52:54 PM »
and there's another one called is_public_serivce().   look close!

Ahh - there was a spelling error (which may even have come from Standard, although I am not sure): "is_public_serivce" was sometimes used instead of "is_public_service". Very odd. In any event this is now fixed: thank you for spotting that.

Quote
You're deep into undefined territory there. MSVC is leading you down a path with a noose at the end. Standard gets the benefit of a couple more people passing the code through their choice of compilers to purge this type of thing... you'd need to try multiple compilers yourself on occasion so you don't end up with MSVC specific stuff in Extended.  Likely this type of undefined behaviour is the ultimate source of the desync problem going on with linux(gcc) and windows (msvc)...

32 nonnull compare warnings trigger during the build - I quickly scanned them and thought I seen 6 unique. There's actually 7. Attached is warning log - suggest you get rid of all 7.

Thank you for letting me know about that - it is useful to know which things should not be done. I wonder why checking whether "this" is null is illegitimate? It is quite useful to be able to do that. In any case, I have now removed this construct wherever the warnings pointed to it: thank you very much for that log: that was most helpful.

I do compile with GCC, incidentally: the automatic nightly builds all use GCC, including that used by the server, albeit I do not easily then see the warnings.

As to the Linux/Windows desync, that was fixed a while ago: the problem turned out to be the use of the built-in Simutrans min/max templates with 64-bit integers.

Thank you again for your help with quality checking the code: it is very useful to have a more experienced programmer look over this: it is quite challenging doing this mostly on my own as a self-taught coder, and all help is gratefully welcomed.
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.