Skip to content

Capture Reverse P/Invoke frame offset in x86 GC info and disallow return hijacking of reverse P/Invokes on x86. - #49066

Merged
jkoritzinsky merged 5 commits into
dotnet:mainfrom
jkoritzinsky:x86-rev-pinvoke-gcinfo
Mar 5, 2021
Merged

Capture Reverse P/Invoke frame offset in x86 GC info and disallow return hijacking of reverse P/Invokes on x86.#49066
jkoritzinsky merged 5 commits into
dotnet:mainfrom
jkoritzinsky:x86-rev-pinvoke-gcinfo

Conversation

@jkoritzinsky

@jkoritzinsky jkoritzinsky commented Mar 3, 2021

Copy link
Copy Markdown
Member

Fixes #45326

cc: @dotnet/jit-contrib

@jkoritzinsky jkoritzinsky added this to the 6.0.0 milestone Mar 3, 2021
@jkoritzinsky
jkoritzinsky requested a review from jkotas March 3, 2021 17:36
@ghost ghost added the area-ReadyToRun label Mar 3, 2021
jkotas
jkotas reviewed Mar 3, 2021
View reviewed changes
Comment thread src/coreclr/vm/eetwain.cpp

DecodeGCHdrInfo(gcInfoToken, 0, &info);

if (info.revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET)

@jkotas jkotas Mar 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we also need to check this in EH, like GetReversePInvokeFrameStackSlot
is checked on non-x86? It is possible that it is not needed, just mentioning it.

@jkoritzinsky jkoritzinsky Mar 3, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's not needed in x86 Windows EH since we fully integrate with SEH already. However, it looks like Unix x86 needs it, so I'll update the relevant locations to use it.

jkotas
jkotas approved these changes Mar 3, 2021
View reviewed changes

@jkotas jkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@ghost

ghost commented Mar 3, 2021

Copy link
Copy Markdown

Hello @jkoritzinsky!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@jkotas

jkotas commented Mar 4, 2021

Copy link
Copy Markdown
Member

Many tests on x86 failing with:

Assert failure(PID 1332 [0x00000534], Thread: 864 [0x0360]): Assertion failed 'header->revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET' in 'System.Net.NameResolutionPal:GetAddressInfoExCallback(int,int,int)' during 'Emit GC+EH tables' (IL size 10)

    File: D:\workspace\_work\1\s\src\coreclr\jit\gcencode.cpp Line: 1631

@jkoritzinsky

jkoritzinsky commented Mar 4, 2021

Copy link
Copy Markdown
Member Author

I think the sentinel we have for the "invalid offset" value can possibly be a valid value. I'll run through one of the CoreCLR tests that's failing tomorrow.

@jkoritzinsky
jkoritzinsky merged commit 197ce0f into dotnet:main Mar 5, 2021
@jkoritzinsky
jkoritzinsky deleted the x86-rev-pinvoke-gcinfo branch March 5, 2021 21:05
@trylek

trylek commented Mar 5, 2021

Copy link
Copy Markdown
Member

Beautiful, thank you!

@jkotas jkotas mentioned this pull request Mar 5, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Apr 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotas jkotas jkotas approved these changes

Assignees

No one assigned

Labels

area-ReadyToRun

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

Test failure: readytorun/coreroot_determinism/coreroot_determinism/coreroot_determinism.sh

3 participants

@jkoritzinsky @jkotas @trylek