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.

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

Prevent crash when lastworld.json is corrupted or empty. #3100

Merged
merged 2 commits into from
Oct 28, 2023

Conversation

KheirFerrum
Copy link
Collaborator

@KheirFerrum KheirFerrum commented Aug 30, 2023

Summary

SUMMARY: Bugfixes "Prevent crash when lastworld.json is corrupted or empty."

Purpose of change

Had an issue today where someone's computer didn't end the game appropriately, corrupting lastworld.json, this caused a crash that would consistently prevent the game from starting with no warning except crash log messages.

Describe the solution

Use try and catch to catch exceptions and report them when they occur loading lastworld.json

Describe alternatives you've considered

Testing

Need others to test for me, my computer is down. This should be tested via creating lastworld.json in the config folder. Either leave it empty or in various states of incomplete/wrong format.

Additional context

@github-actions github-actions bot added the src changes related to source code. label Aug 30, 2023
src/worldfactory.cpp Outdated Show resolved Hide resolved
@scarf005 scarf005 self-assigned this Aug 30, 2023
@scarf005 scarf005 self-requested a review August 30, 2023 07:30
Copy link
Member

@scarf005 scarf005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moderately corrupt JSON

{
  "world_name": Very-Empty",
  "foo": [1,,2,3]
  "character_name": "Maribel Hearn"
}  

image

array in lastworld.json:

[]

image

empty lastworld.json:

image

corrupt options.json for comparison:

[adfs
  { "info": "If enabled, when your character dies, the player is given a prompt that gives the option to cancel savefile deletion and other death effects, returning to the main menu without saving instead.", "default": "Default: False", "name": "PROMPT_ON_CHARACTER_DEATH", "value": "true" },
(continues below)

image


the changes doesn't seem to work, could you have a look?

@KheirFerrum
Copy link
Collaborator Author

Dammit, will make draft, can't really do much with a notepad, so will have to wait until I get my computer back. Weird that test_object() isn't working at all.

@KheirFerrum KheirFerrum marked this pull request as draft August 30, 2023 12:53
@scarf005
Copy link
Member

yeah strange. i think cata's JSON parser doesn't handle invalid JSONs very well in general, as seen on corrupt options.json

@olanti-p
Copy link
Contributor

test_object only checks if an object is about to start (i.e. next character is {), it doesn't validate it or even check if there's a matching }.

According to the debugger, in some cases the game crashes because get_object() throws an exception when it tries and fails to parse the object, but that exception isn't caught by anything.

Other cases were caused by missing check in Lua backtrace code, that should be fixed by #3187

@KheirFerrum
Copy link
Collaborator Author

test_object only checks if an object is about to start (i.e. next character is {), it doesn't validate it or even check if there's a matching }.

According to the debugger, in some cases the game crashes because get_object() throws an exception when it tries and fails to parse the object, but that exception isn't caught by anything.

Other cases were caused by missing check in Lua backtrace code, that should be fixed by #3187

Then is there no way to prevent this from crashing? Or just deleting the file and restarting when it would otherwise crash?

@olanti-p
Copy link
Contributor

You need to replace that if test_object { get_object } else { error } with try { get_object } catch { error }. test_object is useless here; get_object works perfectly - detects error and throws an exception, but because there's no code that catches the exception, it gets caught by the OS and the OS terminates the game.

Co-Authored-By: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
So that errors reveal the file path of the offending file.
@KheirFerrum KheirFerrum requested a review from scarf005 October 26, 2023 14:29
@KheirFerrum KheirFerrum marked this pull request as ready for review October 26, 2023 14:29
Copy link
Member

@scarf005 scarf005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 DEBUG    : Json error: config/lastworld.json:2:23: missing comma

{
  "world_name": "Test"
                      ^
  "character_name": "Maribel Hearn"
}


 FUNCTION : void worldfactory::load_last_world_info()
 FILE     : /home/scarf/repo/cata/Cataclysm/src/worldfactory.cpp
 LINE     : 609
 VERSION  : BN 440e1342128

LGTM

@scarf005 scarf005 added this pull request to the merge queue Oct 28, 2023
Merged via the queue into cataclysmbnteam:upload with commit 6389933 Oct 28, 2023
15 of 17 checks passed
olanti-p pushed a commit to olanti-p/Cataclysm-BN that referenced this pull request Oct 30, 2023
…bnteam#3100)

* Prevent crash when lastworld.json is corrupted or empty.

Co-Authored-By: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update worldfactory.cpp

So that errors reveal the file path of the offending file.

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@KheirFerrum KheirFerrum deleted the patch-1 branch November 2, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants