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

[metadata-update] Make a copy of the DIL when applying updates #52344

Merged
merged 1 commit into from
May 6, 2021

Conversation

lambdageek
Copy link
Member

Otherwise we may point into memory that is cleaned up by the GC

@lambdageek lambdageek added the area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc label May 5, 2021
@ghost
Copy link

ghost commented May 5, 2021

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

Otherwise we may point into memory that is cleaned up by the GC

Author: lambdageek
Assignees: -
Labels:

area-VM-meta-mono

Milestone: -

@lambdageek
Copy link
Member Author

Would like to backport this to 6.0-preview4

@lambdageek
Copy link
Member Author

/cc @marek-safar

Copy link
Member

@thaystg thaystg left a comment

Choose a reason for hiding this comment

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

LGTM

Otherwise we may point into memory that is cleaned up by the GC
@lambdageek lambdageek force-pushed the fix-dil-bytes-copy branch from 2020c07 to a4d2966 Compare May 6, 2021 00:03
Comment on lines +429 to +430
/* TODO: find a better memory manager. But this way we at least won't lose the IL data. */
MonoMemoryManager *mem_manager = (MonoMemoryManager *)mono_alc_get_default ()->memory_manager;
Copy link
Member Author

Choose a reason for hiding this comment

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

I know I could use m_image_get_memory_manager (base_image) here, but that function looks different in 6.0-preview4, so for the sake of the backport, I'd rather dump the delta IL into the default memory manager for now.

@lambdageek
Copy link
Member Author

Verified via manual testing that we no longer crash if dil_data is corrupted (or ends up pointing at reclaimed memory) by changing

LoadMetadataUpdate (assm, dmeta_data, dil_data, dpdb_data);

to

            LoadMetadataUpdate (assm, dmeta_data, dil_data, dpdb_data);
            for (int i = 0; i < dil_data.Length; ++i)
                    dil_data[i] = (byte)255;

@lambdageek
Copy link
Member Author

/backport to release/6.0-preview4

@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2021

Started backporting to release/6.0-preview4: https://github.com/dotnet/runtime/actions/runs/815283039

@lambdageek lambdageek merged commit 222cef5 into dotnet:main May 6, 2021
@lambdageek lambdageek mentioned this pull request May 6, 2021
51 tasks
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
@lambdageek lambdageek deleted the fix-dil-bytes-copy branch March 19, 2022 16:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants