Error in Filters networks

[Yona-TYT]

Playing with network filters makes simutrans closes.
I had noticed this before, but I thought it had already been fixed.

Code Select Expand



[yona@localhost simutrans]$ gdb simutrans
GNU gdb (GDB) Fedora 7.11.1-75.fc24
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-redhat-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from simutrans...(no debugging symbols found)...done.
(gdb) run
Starting program: /home/yona/Descargas/simutrans/simutrans
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/libthread_db.so.1".
Missing separate debuginfo for /lib/libbz2.so.1.0
Try: dnf --enablerepo='*debug*' install /usr/lib/debug/.build-id/53/bc0767f9669e60c1b79ec494fac3fadf5d056f.debug
Use work dir /home/yona/Descargas/simutrans/
Reading low level config data ...
parse_simuconf() at config/simuconf.tab: Reading simuconf.tab successful!
Preparing display ...
[New Thread 0xb767cb40 (LWP 7567)]
SDL_driver=x11, hw_available=0, video_mem=0, blit_sw=0, bpp=32, bytes=4
Screen Flags: requested=10, actual=10
dr_os_open(SDL): SDL realized screen size width=704, height=560 (requested w=704, h=560)
Loading font 'font/prop.fnt'
font/prop.fnt successfully loaded as old format prop font!
Init done.
parse_simuconf() at pak/config/simuconf.tab:
Reading simuconf.tab successful!
warning: the debug information found in "/usr/lib/debug//lib/libsystemd.so.0.14.0.debug" does not match "/lib/libsystemd.so.0" (CRC mismatch).


warning: the debug information found in "/usr/lib/debug//usr/lib/libsystemd.so.0.14.0.debug" does not match "/lib/libsystemd.so.0" (CRC mismatch).


[New Thread 0xb25f9b40 (LWP 7568)]
[Thread 0xb25f9b40 (LWP 7568) exited]
[New Thread 0xb25f9b40 (LWP 7569)]
Reading compatibility sound data ...
Loaded /home/yona/Descargas/simutrans/pak/sound/bus.wav to sample 0.
Loaded /home/yona/Descargas/simutrans/pak/sound/truck.wav to sample 1.
Loaded /home/yona/Descargas/simutrans/pak/sound/diesel.wav to sample 2.
Loaded /home/yona/Descargas/simutrans/pak/sound/steam.wav to sample 3.
Loaded /home/yona/Descargas/simutrans/pak/sound/boat-horn.wav to sample 4.
Loaded /home/yona/Descargas/simutrans/pak/sound/comet-jet.wav to sample 5.
Loaded /home/yona/Descargas/simutrans/pak/sound/planelow.wav to sample 6.
Loaded /home/yona/Descargas/simutrans/pak/sound/click.wav to sample 7.
Loaded /home/yona/Descargas/simutrans/pak/sound/boing.wav to sample 8.
Loaded /home/yona/Descargas/simutrans/pak/sound/jackhammer.wav to sample 9.
Loaded /home/yona/Descargas/simutrans/pak/sound/gavel.wav to sample 10.
Loaded /home/yona/Descargas/simutrans/pak/sound/dock.wav to sample 11.
Loaded /home/yona/Descargas/simutrans/pak/sound/explosion.wav to sample 12.
Loaded /home/yona/Descargas/simutrans/pak/sound/cash.wav to sample 13.
could not load wav (Couldn't open /home/yona/Descargas/simutrans/pak/sound/beaches.wav)
Loaded /home/yona/Descargas/simutrans/pak/sound/forest.wav to sample 14.
Loaded /home/yona/Descargas/simutrans/pak/sound/Water.wav to sample 15.
Loaded /home/yona/Descargas/simutrans/pak/sound/desert.wav to sample 16.
Loaded /home/yona/Descargas/simutrans/pak/sound/tropic.wav to sample 17.
could not load wav (Couldn't open /home/yona/Descargas/simutrans/pak/sound/mediterran.wav)
could not load wav (Couldn't open /home/yona/Descargas/simutrans/pak/sound/temperate.wav)
could not load wav (Couldn't open /home/yona/Descargas/simutrans/pak/sound/tundra.wav)
could not load wav (Couldn't open /home/yona/Descargas/simutrans/pak/sound/rocky.wav)
Loaded /home/yona/Descargas/simutrans/pak/sound/arctic.wav to sample 18.
Loading BDF font 'cyr.bdf'
Reading city configuration ...
Reading speedbonus configuration ...
Reading menu configuration ...
Reading object data from pak/...
Loaded /home/yona/Descargas/simutrans/pak/sound/horse.wav to sample 19.
Loaded /home/yona/Descargas/simutrans/pak/sound/crossing.wav to sample 20.
Reading menu configuration ...
Midi disabled ...
Calculating textures ...done
[New Thread 0xad7d0b40 (LWP 7570)]
[New Thread 0xacfcfb40 (LWP 7571)]
[New Thread 0xac7ceb40 (LWP 7572)]
Creating cities ...
Creating cities: 1
Creating factories ...
Distributing 1 tourist attractions ...
Preparing startup ...
Loading BDF font 'cyr.bdf'
Show banner ...
[New Thread 0xaa41bb40 (LWP 7573)]
[New Thread 0xa9c1ab40 (LWP 7574)]
[New Thread 0xa9419b40 (LWP 7575)]
expose: x=1440, y=825
textur_resize()::screen=0x85da968
Running world, pause=0, fast forward=0 ...
World destroyed.
[New Thread 0xa8420b40 (LWP 7578)]
[Thread 0xa8420b40 (LWP 7578) exited]
World finished ...
Show banner ...
Running world, pause=0, fast forward=0 ...
set_zoom_factor() : set 4 (3/4)
set_zoom_factor() : set 5 (5/8)
set_zoom_factor() : set 6 (1/2)
set_zoom_factor() : set 7 (3/8)
set_zoom_factor() : set 8 (1/4)
set_zoom_factor() : set 9 (1/8)
set_zoom_factor() : set 8 (1/4)
set_zoom_factor() : set 7 (3/8)
set_zoom_factor() : set 6 (1/2)
set_zoom_factor() : set 5 (5/8)
set_zoom_factor() : set 6 (1/2)
set_zoom_factor() : set 7 (3/8)
set_zoom_factor() : set 8 (1/4)
set_zoom_factor() : set 9 (1/8)
set_zoom_factor() : set 8 (1/4)
set_zoom_factor() : set 7 (3/8)
set_zoom_factor() : set 6 (1/2)
set_zoom_factor() : set 5 (5/8)
set_zoom_factor() : set 4 (3/4)
set_zoom_factor() : set 3 (1/1)
set_zoom_factor() : set 2 (4/3)
set_zoom_factor() : set 1 (3/2)
set_zoom_factor() : set 0 (2/1)


Thread 1 "simutrans" received signal SIGSEGV, Segmentation fault.
0x08068288 in ?? ()
Missing separate debuginfos, use: dnf debuginfo-install systemd-libs-229-12.fc24.i686
(gdb) where
#0  0x08068288 in ?? ()
#1  0x081933c6 in ?? ()
#2  0x0818f9c0 in ?? ()
#3  0x08128dfd in ?? ()
#4  0x0811d9b6 in ?? ()
#5  0x08170a1f in ?? ()
#6  0x081a2d42 in ?? ()
#7  0x081ecc5a in ?? ()
#8  0x081eceb1 in ?? ()
#9  0x081ef328 in ?? ()
#10 0x08346cad in ?? ()
#11 0x08391618 in ?? ()
#12 0x08346e16 in ?? ()
#13 0x0839e456 in ?? ()
#14 0x083504a1 in ?? ()
#15 0x0835f64f in ?? ()
#16 0x083c311f in ?? ()
#17 0xb7b1f5a6 in __libc_start_main (main=0x83c3104, argc=1, argv=0xbffff0e4,
    init=0x83c35d0, fini=0x83c35c0, rtld_fini=0xb7feb200 <_dl_fini>,
    stack_end=0xbffff0dc) at ../csu/libc-start.c:289
#18 0x0804c3e1 in ?? ()


[Ters]

These stack traces are virtually useless unless you use debug builds. It would be more helpful to describe what exactly you do when it crashes. Did you press a button and if so, which one? Did you click on anything else, and if so, what? Did you zoom in or out? Did you move around the map? Or does it crash when you are not even touching anything?

[DrSuperGood]

Does this error happen with a specific map? Or all maps? Is there a specific requirement for the crash to occur?

[Yona-TYT]

Does this error happen with a specific map? Or all maps? Is there a specific requirement for the crash to occur?

I suspect this happens when there are many active networks, but is a bit hard to replicate I'm afraid.

Play Online with "Rivers Server" for test.


And this is useful ?.

Code Select Expand


(gdb) where
#0  0x08068a78 in ware_besch_t::get_catg_index (this=0x4)
    at bauer/../vehicle/../besch/ware_besch.h:92
#1  0x08168c84 in reliefkarte_t::is_matching_freight_catg (this=0x996f8e0,
    goods_catg_index=...) at gui/karte.cc:1795
#2  0x08164fc8 in reliefkarte_t::draw (this=0x996f8e0, pos=...)
    at gui/karte.cc:1237
#3  0x08116fd1 in gui_scrollpane_t::draw (this=0xcc04f40, pos=...)
    at gui/components/gui_scrollpane.cc:213
#4  0x0810d515 in gui_container_t::draw (this=0xcc04a2c, offset=...)
    at gui/components/gui_container.cc:252
#5  0x0814d6b2 in gui_frame_t::draw (this=0xcc04a28, pos=..., size=...)
    at gui/gui_frame.cc:151
#6  0x08173e45 in map_frame_t::draw (this=0xcc04a28, pos=..., size=...)
    at gui/map_frame.cc:771
#7  0x081ad328 in display_win (win=0) at gui/simwin.cc:926
#8  0x081ad5af in display_all_win () at gui/simwin.cc:959
#9  0x081af910 in win_display_flush (konto=263728204.88) at gui/simwin.cc:1549
#10 0x082c8c9a in intr_refresh_display (dirty=false) at simintr.cc:79
#11 0x083124b8 in karte_t::sync_step (this=0x8b584c8, delta_t=41,
    do_sync_step=true, display=true) at simworld.cc:3549
#12 0x082c8e1e in interrupt_check (caller_info=0x83869b4 "simworld.cc:6542")
    at simintr.cc:111
#13 0x0831eb6c in karte_t::interactive (this=0x8b584c8, quit_month=2147483647)
---Type <return> to continue, or q <return> to quit---
    at simworld.cc:6542
#14 0x082d16c9 in simu_main (argc=1, argv=0xbffff0f4) at simmain.cc:1312
#15 0x082e0899 in sysmain (argc=1, argv=0xbffff0f4) at simsys.cc:805
#16 0x08341c06 in main (argc=1, argv=0xbffff0f4) at simsys_s2.cc:753
(gdb)

[ucho]

Looks like out-of-bounds read in:

Code Select Expand


reliefkarte_t::get_karte()->freight_type_group_index_showed_on_map = viewable_freight_types[freight_type_c.get_selection()];

To reproduce - expand combobox, resize minimap, select something from combobox, then get_selection() returns -1.

[DrSuperGood]

I cannot recreate this crash so far but there is certainly something wrong with the pull down list logic.

As seen in the image, there are no scroll bars and it is not showing the top of the list. Scroll wheel does not work on it as well.

There also seems to be an off by 1 error with click detection allowing the selection of what seems to be nothing. This is shown in the other image where I have set all 3 pulldowns to nothing (not all). This might be the cause of the crash, however it did not crash for me possibly due to different memory layout of MSVC builds.

gui_scrolled_list_t resize logic does not correct offset. That is the first problem which was fixed fix.
The other was fixed by applying a bounds check to any selected value as long as there is at least 1 element in the list so that it is not possible to select no value. Not sure if this will fix the crash as I was not able to recreate the crash in the first place (likely due to memory layout being different).

I will commit a patch as soon as I can (this could take several weeks). However if another developer wants to get it done sooner the patch is attached bellow.

[Ters]

And this is useful ?.

In case it's not obvious from the replies: yes.

[Dwachs]

I will commit a patch as soon as I can. However if someone else wants to get it done sooner the patch is attached bellow.

Thanks for looking into this. Take your time.

[DrSuperGood]

More detail to the two problems.

The strange pulldown list offset allowing one to view past the end of list was the result of inconsistent list offset caching. For some reason the element stores its own copy of offset derived from the scroll bar. This is usually kept identical to the scroll bar offset except when the element is resized as the scroll bar is resized but the offset is not updated to the resized scroll bar's offset. Updating the offset seems to work, although that makes the assumption offset of a non-scrollable scroll bar (100% of height) is 0 which seems to be the case.

The invalid value seems to be because the pulldown list allows for no selection, specifically if one presses the far top of element 0, while the selection element is meant to always have a value if one is available. When being fed in the values from the pulldown list there is no bound check to make sure an element was selected if at least 1 element exists. Applying a bound check that forces element 0 to be selected if failed seems to have fixed this. It is assumed that invalid values should be allowed when there are no elements in the list for the logical reason that nothing can be selected. The reason the pulldown list element type was not modified to not allow for invalid selection was that it appeared to be a purposely programed feature, so might be used by other element types.

[prissi]

The patch does something which is not really intended. If one clicks on a seperator or an non-slectable element, it still returns item 0 instead invalid item -1 or (better) the previously selected item (and then not calling the callback). Also if element 0 is unselectable one probabbly does not want to return it.

The various lists were intended to handle item -1 as invalid item. Thus the error is rather in the logic around it.

[DrSuperGood]

If one clicks on a seperator or an non-slectable element, it still returns item 0 instead invalid item -1 or (better) the previously selected item (and then not calling the callback). Also if element 0 is unselectable one probabbly does not want to return it.

In which case there is a hitbox error in gui_scrolled_list_t allowing the selection of -1 by clicking the top of element 0?

Should gui_scrolled_list_t  and gui_combobox_t elements be able to return -1 if at least 1 element is selectable? I would imagine having a restriction that it always returns a selectable value if available would simplify UI design. Otherwise the solution to this would involve having to add tests for -1 every time such a value is used to map some functionality which is required.

Here is an update patch which addresses some logical errors with gui_scrolled_list_t hit detection. This alone should fix the invalid selection problem within the scope of the gui_combobox_t as with the way gui_scrolled_list_t is used it is no longer possible for an invalid selection to be made. Anything clicked within a gui_scrolled_list_t that is before or after the list will return an invalid selection since no element was selected (old behaviour was anything clicked after the list returned the last element). Some minor logic errors with list item event origin translation have also been fixed.

The main issue was that the hitboxs for the list were shifted down by 2 pixels. This meant there was a 2 pixel gap at the top of the list directly below the boarder which could be used to make an invalid selection. I am guessing the constant "2" came from the way boarders were originally implemented. When more dynamic boarders were added, the constant was accidently retained.

[Dwachs]

The main issue was that the hitboxs for the list were shifted down by 2 pixels. This meant there was a 2 pixel gap at the top of the list directly below the boarder which could be used to make an invalid selection. I am guessing the constant "2" came from the way boarders were originally implemented. When more dynamic boarders were added, the constant was accidently retained.

Clicking in this tiny space immediately crashes. The patch fixes this. Patch seems fine to me (besides boarders -> borders).

[Yona-TYT]

Maybe there's some time for this too: http://forum.simutrans.com/index.php?topic=14854.msg146608#msg146608