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

Migrate legacy wheels and fix memory corruption in vehicle deserialization #49589

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

eltank
Copy link
Contributor

@eltank eltank commented Jul 4, 2021

Summary

Bugfixes "Migrate legacy wheels and fix memory corruption in vehicle deserialization"

Purpose of change

This is the long-term fix for #49540. It reverts and replaces the short term fix #49568.
This PR migrates the deprecated steerable wheel vparts found in 0.E saves to their non-deprecated counterparts.
It also prevents memory corruption that can happen when vehicle deserialization fails, the error is silently ignored, and the game continues with a partially-initialized vehicle object.

Describe the solution

  • Add the steerable wheel vparts to the deprecated replacement map in vehicle_part::deserialize(). (I also tidied up the code a bit)
  • When deserializing the "parts" vector: catch json exceptions, report them, and skip the bad vparts.
  • When deserializing the "vehicles" vector: catch json exceptions, report them, and skip the bad vehicles.

With this change any future errors that occur when deserializing a vehicle will be prominently displayed and if the user chooses to ignore them we'll do the best we can to recover (removing bad vparts or removing the problematic vehicles).

Describe alternatives you've considered

Testing

Load one of the problematic 0.E saves from #49540. Observe no errors or mangled vehicles. Drive a problematic vehicle.
Check the debug.log for vpart migration messages. [actually they don't show up there because of severity filtering]

Save the game and search the map json files for "wheel_armor_steerable" (present in the original save), find none.

Additional context

We probably need a templatized function for "deserialize a vector of Foo from a json array one at a time and if any of them fails display an error message and skip it".

I think we also need a clang_tidy check for calls to JsonIn::read() that don't set throw_on_error to true and also don't check the return value for success. These calls silently ignore errors and may end up using partially-initialized objects, which leads to undefined behavior such as memory corruption. The vehicle deserialization call was one instance of this, but there are others (and I'm not going to fix them in this PR).

Some of the older vpart migrations from vehicle_part::deserialize can probably be removed (any of them that were present in the last 0.E release or to be extra safe just those present in the first 0.E release), because those vparts should not appear in supported save files.

Here's what a vehicle part error would look like:

part_err

@eltank
Copy link
Contributor Author

eltank commented Jul 4, 2021

I would recommend backporting this to 0.F in order to migrate the obsolete wheel vparts in save files and to safely catch any other issues with vehicle deserialization without causing memory corruption.

@eltank eltank force-pushed the fix_save_compat_2 branch from 49084d3 to ee0f261 Compare July 5, 2021 00:10
@anothersimulacrum anothersimulacrum added 0.F Backport Candidate <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` labels Jul 5, 2021
@ZhilkinSerg ZhilkinSerg merged commit 0d6814d into CleverRaven:master Jul 5, 2021
@eltank eltank deleted the fix_save_compat_2 branch July 5, 2021 20:34
@eltank
Copy link
Contributor Author

eltank commented Jul 5, 2021

@jbytheway Could you comment on the clang-tidy idea from this PR? Is it feasible? Should I open an Issue for it?

I think we also need a clang-tidy check for calls to JsonIn::read() that don't set throw_on_error to true and also don't check the return value for success. These calls silently ignore errors and may end up creating partially-initialized objects, which leads to undefined behavior such as memory corruption. The vehicle deserialization call was one instance of this, but there are others (and I'm not going to fix them in this PR).

@jbytheway
Copy link
Contributor

Yes, that's feasible, and it's something I've been wanting to do for a while. Not sure if we need to use clang-tidy, though. I had been holding out for the switch to C++17 so we could solve using [[nodiscard]].

ZhilkinSerg added a commit to ZhilkinSerg/Cataclysm-DDA that referenced this pull request Aug 12, 2021
ZhilkinSerg added a commit that referenced this pull request Aug 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants