Author Topic: Fix segfault when clicking on depot  (Read 2238 times)

0 Members and 1 Guest are viewing this topic.

Offline ArthurDenture

Fix segfault when clicking on depot
« on: September 08, 2013, 05:46:05 AM »
I found a weird segfault that happened when opening the depot dialog box. It turned out to be caused by the following sequence:

- Buttons have a union {text, targetpos}, with buttons of type posbutton (the triangular button that jumps you to another location) using target and all others using text
- The goto line button in the depot dialog box is a posbutton
- When the depot dialog is created, a WIN_OPEN infowin event is fired
- The button infowin event handler interpreted the union as being text instead of targetpos, thus dereferencing an invalid pointer

The fix in this case was simply to get rid of the union type so that it's always safe to read from the text member variable. The patch is attached.

(I think the segfault has been around since revision 6048, https://github.com/aburch/simutrans/commit/21d8079c7a9f0e32a118e62b7281a9a97453cd8a, but that commit just triggered the existing bug, which has been around essentially forever. You wouldn't always get a segfault, depending on where the invalid pointer ended up pointing, which is likely why this bug is as old as it is. For instance, the segfault went away for me when running under gdb, which made debugging fun.)

Offline TurfIt

Re: Fix segfault when clicking on depot
« Reply #1 on: September 08, 2013, 06:07:02 PM »
The joys of unions... Thanks for finding this. If one actually calls init on the button when creating, things are set right, if bypassed, well...  Fixed by just setting no translate on posbuttons as intended.

Offline ArthurDenture

Re: Fix segfault when clicking on depot
« Reply #2 on: September 08, 2013, 10:54:13 PM »
That does fix the bug, thanks. Are you sure you don't want the part of the patch that removes the union? It's really an accident waiting to happen that if you try to read the text member variable within a posbutton, you get a garbage pointer instead of NULL.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 4812
  • Total likes: 191
  • Helpful: 108
  • Languages: EN, NO
Re: Fix segfault when clicking on depot
« Reply #3 on: September 09, 2013, 04:41:20 AM »
A NULL pointer is also an accident waiting to happen. The only solution is to not have a text member variable for posbuttons (whatever that is) at all, so that attempts to read it yields a compile time error.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 8799
  • Total likes: 320
  • Helpful: 229
  • Languages: De,EN,JP
Re: Fix segfault when clicking on depot
« Reply #4 on: September 09, 2013, 08:07:44 PM »
The GUI renovation project will surely give posbuttons their own grafik, and maybe also their own class. Those buttons with subtype was a very very old legacy of the very first simutrans GUI, which is almost completely gone (luckily). Current version was 3 and themed will be 4 ...

Offline Max-Max

Re: Fix segfault when clicking on depot
« Reply #5 on: September 12, 2013, 08:04:22 PM »
Prissi is right. The whole GUI class structure will have a thorough redesign where we will use polymorphism to reuse as much code as possible and transfer basic behaviours to a grope of similar GUI controls.

I'm in the stage where one fundamental brick is almost in place; the basic functionality of the theme manager.
When this is in place, I can start to work on the new GUI structure. There will for sure be some new GUI controls to play with as well together with a looot of theme images :)

Meanwhile, I wouldn't spend to much time to "fix" the current GUI system. Only fix what is necessarily to prevent crashes or obviously user experience errors...
- My code doesn't have bugs. It develops random features...