News:

Use the "Forum Search"
It may help you to find anything in the forum ;).

Bug: [master] Crash when clicking 'changing price' on new brig in shipyard.

Started by Hanczar, May 30, 2018, 11:19:42 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Hanczar

I have it 100% reproducible.

version:
Desync+-6939-g0c7b97b1e
local build from master branch made today.


(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff6a75f5d in __GI_abort () at abort.c:90
#2  0x00007ffff6abe28d in __libc_message (action=action@entry=(do_abort | do_backtrace), fmt=fmt@entry=0x7ffff6be3a26 "*** %s ***: %s terminated\n") at ../sysdeps/posix/libc_fatal.c:181
#3  0x00007ffff6b6483f in __GI___fortify_fail_abort (need_backtrace=need_backtrace@entry=true, msg=msg@entry=0x7ffff6be39ad "buffer overflow detected") at fortify_fail.c:33
#4  0x00007ffff6b64861 in __GI___fortify_fail (msg=msg@entry=0x7ffff6be39ad "buffer overflow detected") at fortify_fail.c:44
#5  0x00007ffff6b62550 in __GI___chk_fail () at chk_fail.c:28
#6  0x00007ffff6b61a39 in _IO_str_chk_overflow (fp=<optimized out>, c=<optimized out>) at vsprintf_chk.c:31
#7  0x00007ffff6ac2d59 in __GI__IO_default_xsputn (f=0x7fffffffae80, data=<optimized out>, n=37) at genops.c:455
#8  0x00007ffff6a9099f in _IO_vfprintf_internal (s=s@entry=0x7fffffffae80, format=format@entry=0x555555f15bf0 "Catering income per km (full convoy):", ap=ap@entry=0x7fffffffafc0) at vfprintf.c:1328
#9  0x00007ffff6b61adb in ___vsprintf_chk (s=0x7fffffffb1a0 "Catering income per km (full co", flags=1, slen=32, format=0x555555f15bf0 "Catering income per km (full convoy):", args=args@entry=0x7fffffffafc0) at vsprintf_chk.c:82
#10 0x00007ffff6b61a0a in ___sprintf_chk (s=s@entry=0x7fffffffb1a0 "Catering income per km (full co", flags=flags@entry=1, slen=slen@entry=32, format=<optimized out>) at sprintf_chk.c:31
#11 0x000055555570e73c in sprintf (__fmt=<optimized out>, __s=0x7fffffffb1a0 "Catering income per km (full co") at /usr/include/x86_64-linux-gnu/bits/stdio2.h:34
#12 gui_class_vehicleinfo_t::draw (this=this@entry=0x555578cd0d88, offset=...) at gui/vehicle_class_manager.cc:1030
#13 0x000055555567a45c in gui_scrollpane_t::draw (this=0x555578cd0b18, pos=...) at gui/components/gui_scrollpane.cc:213
#14 0x0000555555670c65 in gui_container_t::draw (this=this@entry=0x555578cd0aa8, offset=...) at gui/components/gui_container.cc:252
#15 0x00005555556a687b in gui_frame_t::draw (this=this@entry=0x555578cd0aa0, pos=..., size=...) at gui/gui_frame.cc:202
#16 0x000055555570cb80 in vehicle_class_manager_t::draw (this=this@entry=0x555578cd0aa0, pos=..., size=...) at gui/vehicle_class_manager.cc:357
#17 0x00005555556fe5ea in display_win (win=win@entry=2) at gui/simwin.cc:924
#18 0x00005555556ff02f in display_all_win () at gui/simwin.cc:957
#19 0x0000555555701405 in win_display_flush (konto=8099.3999999999996) at gui/simwin.cc:1547
#20 0x00005555557feb00 in intr_refresh_display (dirty=<optimized out>) at simintr.cc:79
#21 0x0000555555846460 in karte_t::sync_step (this=0x555556881dc0, delta_t=<optimized out>, do_sync_step=<optimized out>, display=display@entry=true) at simworld.cc:4732
#22 0x00005555557fec0e in interrupt_check (caller_info=caller_info@entry=0x5555558dba77 "simworld.cc:10431") at simintr.cc:111
#23 0x000055555585c48d in karte_t::interactive (this=this@entry=0x555556881dc0, quit_month=quit_month@entry=2147483647) at simworld.cc:10431
#24 0x0000555555807a4c in simu_main (argc=argc@entry=1, argv=argv@entry=0x7fffffffdfc8) at simmain.cc:1362
#25 0x0000555555816872 in sysmain (argc=1, argv=0x7fffffffdfc8) at simsys.cc:825
#26 0x00007ffff6a5e1c1 in __libc_start_main (main=0x5555555a7fa0 <main(int, char**)>, argc=1, argv=0x7fffffffdfc8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffdfb8) at ../csu/libc-start.c:308
#27 0x00005555555a7fda in _start ()


Problem is quite obvious in gui/vehicle_class_manager.cc:1030 - stack buffer overflow.

sprintf(catering_service, translator::translate("catering_income_pr_km_(full_convoy):"));

catering_service  is allocated as 32 bytes buffer and above string wrote to it is longer.

My proposed fix which works for me (buffer size increase + safe way of writing strings to local buffer):

git diff ./vehicle_class_manager.cc
diff --git a/gui/vehicle_class_manager.cc b/gui/vehicle_class_manager.cc
index 0b16d7e93..f7f4ecd9e 100644
--- a/gui/vehicle_class_manager.cc
+++ b/gui/vehicle_class_manager.cc
@@ -1019,15 +1019,15 @@ void gui_class_vehicleinfo_t::draw(scr_coord offset)
                                if (v->get_desc()->get_catering_level() > 0)
                                {
                                        uint32 unit_count = 0;
-                                       char catering_service[32];
+                                       char catering_service[64];
                                        if (mail_veh)
                                        {
-                                               sprintf(catering_service, translator::translate("tpo_income_pr_km_(full_convoy):"));
+                                               snprintf(catering_service,sizeof(catering_service), "%s",translator::translate("tpo_income_pr_km_(full_convoy):"));
                                                unit_count = mail_count;
                                        }
                                        else
                                        {
-                                               sprintf(catering_service, translator::translate("catering_income_pr_km_(full_convoy):"));
+                                               snprintf(catering_service,sizeof(catering_service), "%s",translator::translate("catering_income_pr_km_(full_convoy):"));
                                                unit_count = passenger_count;
                                        }
                                        extra_y += LINESPACE;


jamespetts

Thank you very much for that - that is most helpful. I have now incorporated the first, main part of the fix.

I have not incorporated the second part of the fix, however, as I have had problems with "snprintf" compiling in Visual Studio in the past, and I use Visual Studio builds for debugging. Hopefully, the first part of the fix should solve this in any event.
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.