Avoid populating shared patch bypass buffer after initial creation

[tommcdon]

Fixes #102767
This pull request changes DebuggerPatchSkip creation. Before this change we always populated the shared patch bypass buffer, which is used for patch skipping. Although the population of the patch skip buffer is done under the DebuggerController lock, as it is a shared buffer, it's possible we write to it while other threads are executing in the patch buffer which runs without the DebuggerController lock. This exposes a small race condition that can be hit with multiple threads executing on the same breakpoint location. This fix addresses the issue by effectively making the buffer read-only after it is created. There was a previous comment in the code stating that we always overwrite the buffer due to ReJIT jump stamps. Jump stamps were removed in the .NET Core 2.2 timeframe, and so it is believed that it should be safe to avoid constantly re-writing the patch buffer for each DebuggerPatchSkip creation

[Copilot]

Pull Request Overview

This PR refactors how the shared patch bypass buffer is created and accessed to eliminate a race condition by making the buffer read-only once it exists.

• Split GetOrCreateSharedPatchBypassBuffer into separate CreateSharedPatchBypassBuffer and GetSharedPatchBypassBuffer methods
• Updated DebuggerPatchSkip to only write to the buffer on initial creation and skip writes thereafter
• Renamed calls and added guards around RW versus RX buffer access

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/coreclr/debug/ee/controller.h	Renamed buffer accessor, splitting creation and retrieval into two distinct methods
src/coreclr/debug/ee/controller.cpp	Changed patch-skip logic to only populate the buffer when first created and guard subsequent writes

Comments suppressed due to low confidence (2)

src/coreclr/debug/ee/controller.h:554

• [nitpick] The comment is now misleading for CreateSharedPatchBypassBuffer. Consider updating it to describe that this method always creates a new buffer rather than just retrieving one.

    // gets a pointer to the shared buffer

src/coreclr/debug/ee/controller.cpp:4534

• The variable name SharedPatchBypassBufferWriterHolder here refers to the class, not the instance. It should be sharedPatchBypassBufferWriterHolder.AssignExecutableWriterHolder(...) to match the declared variable.

        SharedPatchBypassBufferWriterHolder.AssignExecutableWriterHolder((SharedPatchBypassBuffer*)m_pSharedPatchBypassBuffer, sizeof(SharedPatchBypassBuffer));

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[noahfalk]

A few more style/refactoring suggestions inline. LGTM!

[noahfalk]

👍

[tommcdon]

Android timeout is unrelated, merging

[tommcdon]

/ba-g failures are unrelated