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

Use a drop guard to prevent leaving a dangling reference in internals::serialize::id::run_as_context #203

Merged
merged 3 commits into from
Oct 22, 2020

Conversation

autumnontape
Copy link
Contributor

After reading about the recent improvements to Miri, I tried using it to run Legion's tests. Most of them succeed, but internals::serialize::test::serialize_bincode and internals::serialize::test::serialize_json both panic for reasons I haven't looked into, and because they run in the same thread, the second one finds a dangling pointer and triggers an error in Miri.

This pull request fixes that problem, leaving both test results as panics rather than undefined behavior. The comment suggested using catch_unwind for this, but I used the more common and probably more efficient pattern of a guard struct with a Drop implementation that cleans up. I also added a small test to make sure Miri always has the opportunity to catch this bug, since it was essentially a coincidence that it was caught this time.

@autumnontape
Copy link
Contributor Author

autumnontape commented Sep 30, 2020

Out of the other non-succeeding tests:

  • internals::systems::command::test::simple_write_test causes an ICE
  • internals::systems::schedule::test::flush causes an ICE
  • internals::systems::schedule::test::flush_thread_local causes an ICE
  • internals::world::test::pack ran out of memory on my machine

@TomGillen TomGillen merged commit d6d5d61 into amethyst:master Oct 22, 2020
@TomGillen TomGillen added the type: bug Something isn't working label Feb 10, 2021
@autumnontape autumnontape deleted the serializer-guard branch February 18, 2022 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants