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

Make GC Stress 4/8 work with CET #71085

Merged
merged 2 commits into from
Jun 22, 2022
Merged

Conversation

janvorli
Copy link
Member

This change makes the GC stress 4/8 work without redirection.

It also fixes a problem with missing unwinding of the shadow stack pointer
at few places that was not discovered before. I've found it while testing this
change - it has manifested itself as a shadow stack overflow.

And there is one more fix. The VSD Resolve stub was problematic to unwind
through when null reference was passed as this to it. The stub had a push rdx
as the first instruction and the dereference of this happened after that. So
in case of the null, the call stack in the vectored exception handler contained
a phantom frame caused by a problem unwinding from the stub. That
caused incorrect updating of the shadow SP. I've fixed it by moving the dereference
before the push.

This change makes the GC stress 4/8 work without redirection. It also
fixes a problem with missing unwinding of the shadow stack pointer
that was not discovered before.
@janvorli janvorli added this to the 7.0.0 milestone Jun 21, 2022
@janvorli janvorli requested review from jkotas and VSadov June 21, 2022 20:11
@janvorli janvorli self-assigned this Jun 21, 2022
LIMITED_METHOD_CONTRACT;

#ifdef USE_REDIRECT_FOR_GCSTRESS
return UseContextBasedThreadRedirection();
Copy link
Member

Choose a reason for hiding this comment

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

Can we just always return false? (and delete all the support for GC stress redirection)

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately not. I have originally just undefined the USE_REDIRECT_FOR_GCSTRESS and fixed few places, but I've hit a race problem between another thread redirection and the GC stress running from VEH updating GC references in the context passed to the VEH. I've added various stresslog loggings and found that there is a race when the thread was returning from the VEH and another thread redirected it at that point, the other thread got GetThreadContext with the old values of those GC references. And the context didn't have any of the indicators that would make us not to use it.
I was able to repro it only on the CET enabled machine that has a beta version of Windows, so it could have been caused by a bug that was introduced in that version of Windows. So I've decided to use it only when redirection is disabled (which is currently the case for CET only).


_ASSERTE(!success && (GetLastError() == ERROR_INSUFFICIENT_BUFFER));

PVOID pBuffer = _alloca(contextSize);
Copy link
Member

Choose a reason for hiding this comment

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

What is the context size we usually end up with here on current hw? I am wondering whether it would be better to allocate this on heap.

Copy link
Member

Choose a reason for hiding this comment

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

If it only adds XSAVE_CET_U_FORMAT feature , then this is probably not large.

Copy link
Member Author

Choose a reason for hiding this comment

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

The contextSize is 0x4ff.

Copy link
Member Author

Choose a reason for hiding this comment

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

The size is really just sizeof(CONTEXT) which is 0x4d8 + sizeof(XSAVE_CET_U_FORMAT) which is 16 bytes + few more bytes.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

@janvorli janvorli merged commit 305144e into dotnet:main Jun 22, 2022
@janvorli janvorli deleted the fix-cet-gcstress-2 branch June 22, 2022 08:55
@ghost ghost locked as resolved and limited conversation to collaborators Jul 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants