Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Crash: loading save #6564

Closed
DorpsGek opened this issue May 25, 2017 · 5 comments
Closed

Crash: loading save #6564

DorpsGek opened this issue May 25, 2017 · 5 comments
Labels
bug Something isn't working flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) needs triage This issue needs further investigation before it becomes actionable

Comments

@DorpsGek
Copy link
Member

Brumi opened the ticket and wrote:

The crash files are attached.
I let a game running with AIs, and by the time I came back, it had already crashed. It did manage to create an emergency save, and loading the save causes OpenTTD to crash again.

Operating system: Windows 7 SP1 x64
OpenTTD version: 1.7.0

Attachments

Reported version: 1.7.0
Operating system: All

This issue was imported from FlySpray: https://bugs.openttd.org/task/6564
@DorpsGek
Copy link
Member Author

andythenorth wrote:

Reproduced, crashes for me. FWIW, the game doesn't crash on load if Australian_Industries_Set_AuzInd-6.tar is missing from my local filesystem. The game crashes if I download that grf from bananas.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6564#comment14520

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) labels Apr 7, 2018
@TrueBrain TrueBrain added needs triage This issue needs further investigation before it becomes actionable bug Something isn't working and removed bug from FlySpray labels Apr 12, 2018
@frosch123 frosch123 removed the Core label Apr 14, 2018
@andythenorth andythenorth added the stale Stale issues label Jan 5, 2019
@andythenorth andythenorth removed the stale Stale issues label Jan 24, 2019
@stale
Copy link

stale bot commented Mar 25, 2019

This issue has been automatically marked as stale because it has not had any activity in the last two months.
If you believe the issue is still relevant, please test on the latest nightly and report back.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale Stale issues label Mar 25, 2019
@LordAro
Copy link
Member

LordAro commented Mar 25, 2019

Crash still reproduced with master. Invalid unicode in sign, or memory/data corruption?

Curiously, I can confirm andy's find as well - deleting Australian_Industries_Set_AuzInd-6.tar allows to save to load

(gdb) bt
#0  Utf8Decode (c=c@entry=0x996dedc, s=0x0)
    at C:/msys64/home/LordAro/OpenTTD/src/string.cpp:452
#1  0x00000000005ed9f5 in Utf8Consume (s=0x31be2d80)
    at C:/msys64/home/LordAro/OpenTTD/src/string_func.h:90
#2  0x00000000005f0100 in FormatString (buff=<optimized out>,
    str_arg=<optimized out>, args=args@entry=0x996e620,
    last=last@entry=0x996f2ff "", case_index=case_index@entry=0,
    game_script=game_script@entry=false, dry_run=dry_run@entry=false)
    at C:/msys64/home/LordAro/OpenTTD/src/strings.cpp:916
#3  0x00000000005f06be in FormatString (buff=<optimized out>,
    buff@entry=0x996eb00 "Velas ▒\201▒Music Programme - 'g Plant? ",
    str_arg=<optimized out>, args=args@entry=0x996e620,
    last=last@entry=0x996f2ff "", case_index=case_index@entry=0,
    game_script=false, dry_run=false)
    at C:/msys64/home/LordAro/OpenTTD/src/strings.cpp:1006
#4  0x00000000005f2fdd in GetStringWithArgs (
    buffr=0x996eb00 "Velas ▒\201▒Music Programme - 'g Plant? ",
    string=<optimized out>, args=0x996e620, last=0x996f2ff "", case_index=0,
    game_script=false) at C:/msys64/home/LordAro/OpenTTD/src/strings.cpp:242
#5  0x00000000005f259b in FormatString (buff=<optimized out>,
    str_arg=<optimized out>,
    args=args@entry=0x1dd4700 <_global_string_params>,
    last=last@entry=0x996f2ff "", case_index=case_index@entry=0,
    game_script=game_script@entry=false, dry_run=dry_run@entry=true)
    at C:/msys64/home/LordAro/OpenTTD/src/strings.cpp:1419
#6  0x00000000005f0c52 in FormatString (buff=<optimized out>,
    buff@entry=0x996eb00 "Velas ▒\201▒Music Programme - 'g Plant? ",
    str_arg=<optimized out>,
    args=args@entry=0x1dd4700 <_global_string_params>,
    last=last@entry=0x996f2ff "", case_index=case_index@entry=0,
    game_script=false, dry_run=false)
    at C:/msys64/home/LordAro/OpenTTD/src/strings.cpp:784
#7  0x00000000005f2fdd in GetStringWithArgs (
    buffr=0x996eb00 "Velas ▒\201▒Music Programme - 'g Plant? ",
    string=<optimized out>, args=0x1dd4700 <_global_string_params>,
    last=0x996f2ff "", case_index=0, game_script=false)
    at C:/msys64/home/LordAro/OpenTTD/src/strings.cpp:242
#8  0x00000000005f30b9 in GetString (
    buffr=buffr@entry=0x996eb00 "Velas ▒\201▒Music Programme - 'g Plant? ", string=34845, last=last@entry=0x996f2ff "")
    at C:/msys64/home/LordAro/OpenTTD/src/strings.cpp:268
#9  0x000000000063c5ff in ViewportSign::UpdatePosition (
    this=this@entry=0x309a9650, center=center@entry=2176,
    top=<optimized out>, top@entry=62848, str=<optimized out>,
    str@entry=34845, str_small=str_small@entry=0)
    at C:/msys64/home/LordAro/OpenTTD/src/viewport.cpp:1348
#10 0x00000000005d8f39 in Station::UpdateVirtCoord (this=0x309a9640)
    at C:/msys64/home/LordAro/OpenTTD/src/station_cmd.cpp:427
#11 0x00000000005d9e56 in UpdateAllStationVirtCoords ()
    at C:/msys64/home/LordAro/OpenTTD/src/station_cmd.cpp:455
#12 0x0000000000571109 in UpdateAllVirtCoords ()
    at C:/msys64/home/LordAro/OpenTTD/src/saveload/afterload.cpp:222
#13 0x00000000005714bb in InitializeWindowsAndCaches ()
    at C:/msys64/home/LordAro/OpenTTD/src/saveload/afterload.cpp:244
#14 0x0000000000576ad9 in AfterLoadGame ()
    at C:/msys64/home/LordAro/OpenTTD/src/saveload/afterload.cpp:3138
#15 0x0000000000587708 in DoLoad (reader=reader@entry=0xdc9f430,
    load_check=load_check@entry=false)
    at C:/msys64/home/LordAro/OpenTTD/src/saveload/saveload.cpp:2685
#16 0x00000000005885a3 in SaveOrLoad (
    filename=filename@entry=0x1dcbaac <_file_to_saveload+12> "C:\\Users\\LordAro\\Downloads\\crash.sav", fop=fop@entry=SLO_LOAD,
    dft=dft@entry=DFT_GAME_FILE, sb=sb@entry=NO_DIRECTORY,
    threaded=threaded@entry=true)
    at C:/msys64/home/LordAro/OpenTTD/src/saveload/saveload.cpp:2792
#17 0x000000000052df1c in SafeLoad (
    filename=filename@entry=0x1dcbaac <_file_to_saveload+12> "C:\\Users\\LordAro\\Downloads\\crash.sav", fop=SLO_LOAD, dft=DFT_GAME_FILE,
    newgm=newgm@entry=GM_NORMAL, subdir=subdir@entry=NO_DIRECTORY,
    lf=lf@entry=0x0) at C:/msys64/home/LordAro/OpenTTD/src/openttd.cpp:996
#18 0x000000000052e11c in SwitchToMode (new_mode=<optimized out>)
    at C:/msys64/home/LordAro/OpenTTD/src/openttd.cpp:1080
#19 0x000000000052f325 in GameLoop ()
    at C:/msys64/home/LordAro/OpenTTD/src/openttd.cpp:1440
#20 0x0000000000637c25 in VideoDriver_Win32::MainLoop (this=<optimized out>)
    at C:/msys64/home/LordAro/OpenTTD/src/video/win32_v.cpp:1274
#21 0x000000000052f971 in openttd_main (argc=argc@entry=1,
    argv=argv@entry=0x996fc30)
    at C:/msys64/home/LordAro/OpenTTD/src/openttd.cpp:847
#22 0x000000000053d99b in WinMain (hInstance=<optimized out>,
    hPrevInstance=hPrevInstance@entry=0x0, lpCmdLine=<optimized out>,
    nCmdShow=<optimized out>)
    at C:/msys64/home/LordAro/OpenTTD/src/os/windows/win32.cpp:442
#23 0x000000000095b232 in main (flags=<optimized out>,
    cmdline=<optimized out>, inst=<optimized out>)
    at C:/repo/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/crt0_c.c:18
#24 0x00000000004013a5 in __tmainCRTStartup ()
    at C:/repo/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/crtexe.c:339
#25 0x00000000004014db in WinMainCRTStartup ()
    at C:/repo/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/crtexe.c:195

@stale stale bot removed the stale Stale issues label Mar 25, 2019
@PeterN
Copy link
Member

PeterN commented Mar 26, 2019

This is a bug in the NewGRF, however we should probably not crash.

Sorry, something went wrong.

@PeterN
Copy link
Member

PeterN commented Mar 26, 2019

So the issue is the content Australian_Industries_Set_AuzInd-6.tar has an invalid string for nearby station name for an industry which includes two \80 control codes instead of just one. This control code consumes an argument and, because it treats that argument as a pointer to a string, leads to undefined behaviour because it's meant to be a station index.

I have a patch that tests NewGRF strings for correct control codes, but it's a bit unwieldy, and relies on knowledge of which control codes are permitted in different properties. Quite a lot of effort.

TL;DR it's a effectively just a format string bug (but using our string system formatting)

Sorry, something went wrong.

@PeterN PeterN mentioned this issue Mar 26, 2019
Closed
glx22 added a commit to glx22/OpenTTD that referenced this issue Mar 26, 2019
@glx22
Fix OpenTTD#6564: enforce types of arguments for station name strings
85880f5
glx22 added a commit to glx22/OpenTTD that referenced this issue Mar 26, 2019
@glx22
Fix OpenTTD#6564: enforce types of arguments for station name strings
1fa267b
glx22 added a commit to glx22/OpenTTD that referenced this issue Mar 26, 2019
@glx22
Fix OpenTTD#6564: enforce types of arguments for station name strings
bdf0958
@glx22 glx22 closed this as completed in 427d9d4 Mar 26, 2019
PeterN pushed a commit to PeterN/OpenTTD that referenced this issue Apr 8, 2019
@glx22 @PeterN
Fix OpenTTD#6564: enforce types of arguments for station name strings (…
143fbd6 
…OpenTTD#7419)
PeterN pushed a commit that referenced this issue Apr 8, 2019
@glx22 @PeterN
Fix #6564: enforce types of arguments for station name strings (#7419)
6bfec89
douiwby pushed a commit to douiwby/OpenTTD that referenced this issue Apr 16, 2020
@glx22 @douiwby
Fix OpenTTD#6564: enforce types of arguments for station name strings (…
7a3a623 
…OpenTTD#7419)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) needs triage This issue needs further investigation before it becomes actionable
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix: Validate control-codes in NewGRF strings PeterN/OpenTTD
6 participants
@PeterN @TrueBrain @andythenorth @LordAro @DorpsGek @frosch123