Distinguish OOM and failure to reprotect in thunks APIs #116010

MichalStrehovsky · 2025-05-27T13:08:38Z

I wasn't sure how much cleanup to do here, the failure handling is a bit odd, I assume for historical reasons.

Cc @dotnet/ilc-contrib

Resolves dotnet#113114. I wasn't sure how much cleanup to do here, the failure handling is a bit odd, I assume for historical reasons.

MichalStrehovsky · 2025-05-27T13:09:22Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ThunkPool.cs

-            lock (this)
-            {


I was scratching my head about this - we're in a constructor. Nobody else can have this

That's obviously wrong, but also GetNewThunksBlock doesn't seem to be thread-safe, so perhaps the intention was to protect multiple simultaneous calls to that?

If we are not addressing this, should we open a follow-up issue? (or did I miss some mechanism that is making the static variable access thread safe?)

I'm not a fan of doing more within a PR than what's needed and this is orthogonal. Filed #116089.

dotnet-policy-service · 2025-05-27T13:09:40Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Copilot

Pull Request Overview

This PR enhances the thunk-allocation APIs to distinguish out‐of‐memory failures from other reprotect failures by propagating the exact exception back to managed code.

Added out Exception parameters on GetNewThunksBlock, ExpandHeap, and AllocateThunk so callers can throw the correct exception.
Updated the native RhAllocateThunksMapping signature and implementations to set an isOOM flag.
Changed managed callers (CreateThunksHeap / AllocateThunk) to remove null returns and instead throw the propagated exception.

Reviewed Changes

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

Show a summary per file

File	Description
ThunkPool.cs	Added `out Exception` to block- and thunk-allocation methods and removed null-return fallbacks
RuntimeImports.cs	Updated the `RhAllocateThunksMapping` P/Invoke to take an `int* isOOM` parameter
RuntimeAugments.cs	Switched `CreateThunksHeap` / `AllocateThunk` to the new exception-based APIs
portable.cpp	Stubbed `RhAllocateThunksMapping` to always set `isOOM = 0`
ThunksMapping.cpp	Populated `isOOM` on allocation failure

Comments suppressed due to low confidence (3)

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ThunkPool.cs:115

The new CreateThunksHeap API can now throw different exception types; add unit tests to cover both the OOM and NotSupported failure scenarios.

public static unsafe ThunksHeap CreateThunksHeap(IntPtr commonStubAddress)

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ThunkPool.cs:172

[nitpick] The out parameter exception is ambiguous; consider renaming it to something like allocationException for clarity.

public unsafe IntPtr AllocateThunk(out Exception exception)

src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/Augments/RuntimeAugments.cs:714

This change now throws NotSupportedException for reprotect failures instead of always OutOfMemoryException. Consider whether callers expect the original behavior or need to handle both exception types.

return ThunksHeap.CreateThunksHeap(commonStubAddress);

MichalPetryka · 2025-05-27T14:02:04Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ThunkPool.cs

-                    // The only option is for the caller to attempt and recycle unused thunks to be able to
-                    // find some free entries.
+                    if (isOOM == 0)
+                        exception = new NotSupportedException();


Add a custom exception message?

Do you have a suggestion for a message that would be actually useful (we might not have a full list of reasons why this could fail; we just found out about a new reason a couple months ago...). I mostly just wanted to distinguish OOM from "everything else".

"Dynamic entrypoint allocation is not supported in the current environment."?

jkotas · 2025-05-27T15:14:31Z

src/coreclr/nativeaot/Runtime/ThunksMapping.cpp

@@ -100,13 +100,18 @@ FCIMPL0(int, RhpGetThunkBlockSize)
 }
 FCIMPLEND

-EXTERN_C void* QCALLTYPE RhAllocateThunksMapping()
+EXTERN_C void* QCALLTYPE RhAllocateThunksMapping(int* isOOM)


It is unusual to return error code via out argument, and only allow that error code to be OOM.

Can we return proper error code, and return the pointer via [out] argument instead?

The error code can be an HRESULT or an enum specific to this call. (I know HRESULTs are Windows-specific, but they are part of BCL Exception so they are not going away x-plat, so there is not much value in trying hard to avoid them.)

Good idea with reusing HRESULT! I didn't like the bool either, but I didn't want to invent a dedicated enum that we need to keep in sync between native and managed.

@AaronRobinsonMSFT deleted the HRESULT definitions in #115858 so this no longer compiles.

I would prefer going back to the bool if we don't want HRESULTs anymore.

I think it is fine add back what you need to get this to compile.

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ThunkPool.cs

src/coreclr/nativeaot/Runtime/portable.cpp

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ThunkPool.cs

jkotas · 2025-05-29T06:16:39Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ThunkPool.cs

+                if (result == HResults.E_OUTOFMEMORY)
+                    throw new OutOfMemoryException();
+                else if (result != HResults.S_OK)
+                    throw new PlatformNotSupportedException();


@copilot Could you please suggest a good error message for this exception?

jkotas

Thanks

jkotas · 2025-05-29T23:33:13Z

src/coreclr/nativeaot/Runtime/CommonMacros.h

+#ifndef __GCENV_BASE_INCLUDED__
+#if !defined(_INC_WINDOWS)
+#ifdef _WIN32
+// this must exactly match the typedef used by windows.h
+typedef long HRESULT;
+#else
+typedef int32_t HRESULT;
+#endif
+
+#define S_OK  0x0
+#define E_FAIL 0x80004005
+#define E_OUTOFMEMORY 0x8007000E
+
+#endif // !defined(_INC_WINDOWS)
+#endif // __GCENV_BASE_INCLUDED__
+


Suggested change

#ifndef __GCENV_BASE_INCLUDED__

#if !defined(_INC_WINDOWS)

#ifdef _WIN32

// this must exactly match the typedef used by windows.h

typedef long HRESULT;

#else

typedef int32_t HRESULT;

#endif

#define S_OK 0x0

#define E_FAIL 0x80004005

#define E_OUTOFMEMORY 0x8007000E

#endif // !defined(_INC_WINDOWS)

#endif // __GCENV_BASE_INCLUDED__

#ifdef TARGET_UNIX

typedef int32_t HRESULT;

#define S_OK 0x0

#define E_FAIL 0x80004005

#define E_OUTOFMEMORY 0x8007000E

#endif

Could you please change this to above and move it to PalRedhawk.h right after HRESULT definition`?

This reverts commit 0248f31.

MichalStrehovsky · 2025-05-30T11:22:06Z

/ba-g the failing test was supposedly fixed in #116062, I'm not sure about the timing but we'll see if it's really fixed in the coming days...

Distinguish OOM and failure to reprotect in thunks APIs

1c15454

Resolves dotnet#113114. I wasn't sure how much cleanup to do here, the failure handling is a bit odd, I assume for historical reasons.

Copilot AI review requested due to automatic review settings May 27, 2025 13:08

github-actions bot added the area-NativeAOT-coreclr label May 27, 2025

dotnet-policy-service bot assigned MichalStrehovsky May 27, 2025

MichalStrehovsky commented May 27, 2025

View reviewed changes

Copilot AI reviewed May 27, 2025

View reviewed changes

MichalPetryka reviewed May 27, 2025

View reviewed changes

jkotas reviewed May 27, 2025

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ThunkPool.cs Outdated Show resolved Hide resolved

Rework exception throws

eda2d1d

jkotas reviewed May 28, 2025

View reviewed changes

src/coreclr/nativeaot/Runtime/portable.cpp Outdated Show resolved Hide resolved

jkotas reviewed May 28, 2025

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ThunkPool.cs Outdated Show resolved Hide resolved

jkotas reviewed May 28, 2025

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ThunkPool.cs Outdated Show resolved Hide resolved

FB

d33f7b3

jkotas reviewed May 29, 2025

View reviewed changes

jkotas approved these changes May 29, 2025

View reviewed changes

build-analysis bot mentioned this pull request May 29, 2025

Test failure: baseservices/threading/regressions/115178/115178/115178.cmd #116060

Open

MichalStrehovsky mentioned this pull request May 29, 2025

Race condition creating thunkpools #116089

Open

FB

eee3e35

This was referenced May 29, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

System.Net.Quic.Tests.MsQuicTests.WriteTests failed with "System.Net.Quic.QuicException : The connection timed out from inactivity." #105177

Open

MichalStrehovsky added 2 commits May 29, 2025 22:40

Merge branch 'main' into fix113114

b6ccfb7

Add back HRESULT

0248f31

jkotas reviewed May 29, 2025

View reviewed changes

build-analysis bot mentioned this pull request May 30, 2025

[Apple-mobile] System.Tests.StringTests.[Starts,Ends]WithNoMatch_StringComparison test failure #108832

Open

MichalStrehovsky added 2 commits May 30, 2025 07:24

Revert "Add back HRESULT"

2f1e6ff

This reverts commit 0248f31.

FB

4c20081

build-analysis bot mentioned this pull request May 30, 2025

SIGKILL System.Runtime.Serialization.Formatters.Tests #114287

Open

MichalStrehovsky merged commit 29b1e99 into dotnet:main May 30, 2025
135 of 142 checks passed

MichalStrehovsky deleted the fix113114 branch May 30, 2025 11:22

Distinguish OOM and failure to reprotect in thunks APIs #116010

Distinguish OOM and failure to reprotect in thunks APIs #116010

Uh oh!

Conversation

MichalStrehovsky commented May 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipnavara May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dotnet-policy-service bot commented May 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

jkotas May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented May 30, 2025

Uh oh!

Uh oh!

Uh oh!

filipnavara May 29, 2025 •

edited

Loading

jkotas May 29, 2025 •

edited

Loading