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

[release/7.0] Fix WXORX issue in EEClass::Destruct #79703

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Dec 15, 2022

Backport of #79696 to release/7.0

/cc @janvorli

Customer Impact

.NET runtime with W^X enabled crashes with access violation when unloading code that uses delegates set to instance methods that require return buffer for their return value.

Testing

coreclr tests executed in unloadable assembly load context.

Risk

Low, the location that the fix modifies would always crash with AV with W^X and the change is effectively no-op with W^X disabled.

While investigating failures of some coreclr tests when running in an
unloadable context, I've hit AV in EEClass::Destruct in one of the
tests. The reason is that we are missing ExecutableWriterHolder when
updating refCount on pDelegateEEClass->m_pInstRetBuffCallStub.

This change fixes it.
@janvorli janvorli added this to the 7.0.x milestone Dec 15, 2022
@janvorli janvorli self-assigned this Dec 15, 2022
@janvorli janvorli requested a review from jkotas December 15, 2022 14:25
@janvorli janvorli added the Servicing-consider Issue for next servicing release review label Dec 15, 2022
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 7.0.x

@rbhanda rbhanda modified the milestones: 7.0.x, 7.0.3 Jan 5, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 5, 2023
@carlossanlop
Copy link
Member

@janvorli @jkotas can you please take a look at the CI failures?

@jkotas
Copy link
Member

jkotas commented Jan 5, 2023

This is CoreCLR-specific change. All CI failures are in Mono and thus unrelated.

@carlossanlop
Copy link
Member

Approved by Tactics (7.0.3).
Signed off by area owners.
No OOB changes needed.
CI failures unrelated.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit c79f847 into release/7.0 Jan 5, 2023
@carlossanlop carlossanlop deleted the backport/pr-79696-to-release/7.0 branch January 5, 2023 19:44
@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants