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

Fix mach hardware exception forwarding #105003

Merged
merged 3 commits into from
Jul 17, 2024

Conversation

janvorli
Copy link
Member

A recent change to enable AVX512 register state processing in the mach exception handling on macOS x64 has broken a PAL test and also a case when a hardware exception occurs in 3rd party native code. In both cases runtime hangs, another exception occurs while forwarding the exception message and that ends up happenning infinitely.

The problem is caused by the fact that the MachMessageInfo has grown, since we've changed a field containing float state to contain avx512 state instead. But the buffer that stores the message is of fixed size of 1500 bytes and it is not sufficient.

I have grown the buffer size to the necessary size, but there was another issue lurking behind the first one. The MachExceptionInfo::RestoreState was trying to restore the float state always as x86_FLOAT_STATE instead of choosing x86_FLOAT_STATE, x86_AVX_STATE or x86_AVX512_STATE depending of which of them was stored. So the thread_set_state was failing.

This change fixes both of the problems.

Close #104968

A recent change to enable AVX512 register state processing in the mach
exception handling on macOS x64 has broken a PAL test and also a case
when a hardware exception occurs in 3rd party native code.
In both cases runtime hangs, another exception occurs while forwarding
the exception message and that ends up happenning infinitely.

The problem is caused by the fact that the MachMessageInfo has grown,
since we've changed a field containing float state to contain avx512
state instead. But the buffer that stores the message is of fixed size
of 1500 bytes and it is not sufficient.

I have grown the buffer size to the necessary size, but there was another
issue lurking behind the first one. The MachExceptionInfo::RestoreState
was trying to restore the float state always as x86_FLOAT_STATE instead
of choosing x86_FLOAT_STATE, x86_AVX_STATE or x86_AVX512_STATE depending
of which of them was stored. So the thread_set_state was failing.

This change fixes both of the problems.
@janvorli janvorli added this to the 9.0.0 milestone Jul 17, 2024
@janvorli janvorli self-assigned this Jul 17, 2024
@janvorli
Copy link
Member Author

@jkotas I've simplified the state restoring. I've found that the count is stored in the state as well, so I don't need to have the switch.

@janvorli
Copy link
Member Author

/azp run runtime-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -189,7 +189,7 @@ class MachMessage
// The maximum size in bytes of any Mach message we can send or receive. Calculating an exact size for
// this is non trivial (basically because of the security trailers that Mach appends) but the current
// value has proven to be more than enough so far.
static const size_t kcbMaxMessageSize = 1500;
static const size_t kcbMaxMessageSize = 0x1500;
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to define this as sizeof(mach_message_t) plus some extra space?

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've actually found a constant in the headers representing the maximum trailer size, so we can define it as sizeof(mach_message_t) + MAX_TRAILER_SIZE and be safe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a commit with that change.

Copy link
Member

Choose a reason for hiding this comment

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

Is the MAX_TRAILER_SIZE in a mach source file header? And I am guessing it refers to the security trailers that are mentioned in the comment above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is in the Apple headers and it refers to those trailers.

Copy link
Member

@mikelle-rogers mikelle-rogers left a comment

Choose a reason for hiding this comment

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

Thanks for catching this!

@janvorli janvorli merged commit e5e56df into dotnet:main Jul 17, 2024
85 of 89 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 2024
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.

exception_handling/pal_sxs/test1/paltest_pal_sxs_test1 failure in CI
3 participants