Fix reflection access to misaligned fields in packed structs

[steveisok]

Fixes #133236

On architectures that require natural alignment for acquire/release operations, reflection access to under-aligned fields in packed structs can fault. FieldDesc::GetInstanceField and SetInstanceField use volatile scalar operations for primitive fields, which can raise SIGBUS on arm64 when the field address is misaligned.

This also affects reflection consumers such as the field-walking fallback in ValueType.Equals.

This change applies a consistent scalar-access policy across CoreCLR FieldInfo.GetValue, SetValue, GetValueDirect, and SetValueDirect, including first and cached calls, instance fields, and static fields:

• Naturally aligned primitive, enum, native-int, pointer, and function-pointer fields use the existing volatile scalar operations.
• Atomicity remains platform-dependent; this change does not add a 64-bit atomicity guarantee on 32-bit systems.
• Misaligned non-GC scalar fields use memcpyNoGCRefs and intentionally do not provide atomicity or volatile ordering.
• Managed cached access is used only when the required alignment can be guaranteed. Eight-byte fields remain on the native path on 32-bit.
• Reflection getters load scalar values into aligned storage before boxing them.
• GC-reference access and arbitrary value-type copying retain their existing behavior.

Testing

• Confirmed the original regression fails against the unmodified runtime with SIGBUS on macOS arm64.
• Built checked CoreCLR, libraries, and host successfully.
• Built and ran the targeted checked CoreCLR regression successfully on macOS arm64.
• Built and ran the regression successfully with NativeAOT.
• Added coverage for aligned and misaligned fields across normal and direct reflection APIs, first and cached calls, instance and static fields, and affected scalar types.
• Added an ARM32-specific case for the runtime-aligned 8-byte native path. ARM32 execution remains for CI.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical gaps remain in SetValueDirect and misaligned pointer access, and the regression test has an unreliable placement assumption.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite
Findings: 2

Open findings (2)

• SetValueDirect still uses unaligned volatile stores · New
• Misaligned pointer fields still use the cached direct load · New

What changed in this PR

Updates reflection access for misaligned packed-struct fields to avoid alignment faults while preserving aligned volatile behavior.

Changes:

• Adds alignment-aware managed and native access paths.
• Uses safe copying for misaligned scalar fields.
• Adds regression coverage for reflection and equality scenarios.

File	Summary
src/​tests/​Regressions/​coreclr/​GitHub_133236/​test133236.csproj	Registers the regression test project.
src/​tests/​Regressions/​coreclr/​GitHub_133236/​test133236.cs	Adds coverage for affected field types and access paths.
src/​libraries/​System.Private.CoreLib/​src/​System/​Reflection/​FieldAccessor.cs	Adds alignment checks for cached instance access.
src/​coreclr/​vm/​field.cpp	Adds safe handling for misaligned scalar fields.

[Copilot]

Copilot review overview

🔵 Needs a closer look

NativeAOT coverage and deterministic 32-bit regression-test placement need to be addressed.

Review tier: Lite
Findings: None

Resolved findings (2)

• Misaligned pointer fields still use the cached direct load
• SetValueDirect still uses unaligned volatile stores

Previously missed findings (1)

In code that hasn't changed since last review

Avoid requiring an ARM64 granule crossing on every architecture

src/​tests/​Regressions/​coreclr/​GitHub_133236/​test133236.cs:249

This helper is used by the unconditional TestEntryPoint, but it always searches for a 16-byte granule crossing even on 32-bit targets. CoreCLR only guarantees DATA_ALIGNMENT of 4 bytes there (src/coreclr/vm/arm/cgencpu.h:16, src/coreclr/vm/i386/cgencpu.h:25), and these packed boxes can have a fixed residue because their allocation size is repeatedly rounded to 16 bytes; after AllocationLimit attempts the test can reach line 296 and fail without exercising the runtime fix. Keep the granule-crossing predicate for arm64, and use a normal field-misalignment predicate (or another deterministic placement) on other architectures.

[dotnet-policy-service]

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

[MichalPetryka]

Does this work with EnC?

[jkotas]

This may need to be fixed in NAOT too - make sure that the test passes with NAOT on all arches.

[steveisok]

NativeAOT audit result: no NativeAOT product change is needed for this bug class.

• Normal and TypedReference primitive/enum reads and writes flow through RuntimeAugments.LoadValueTypeField / StoreValueTypeField and their value-type-target variants, then RhBox / RhUnbox. These are block copies, not volatile/acquire-release scalar operations. Non-GC values use Unsafe.CopyBlock; GC-containing values use RhBulkMoveWithWriteBarrier. On ARM32, the block-copy codegen uses integer ldr/str chunks rather than alignment-sensitive ldrd/vldr long/double operations.
• Packed unmanaged-pointer reads use Unsafe.As<byte, IntPtr>. The JIT importer marks that widening reinterpretation GTF_IND_UNALIGNED because the source is byte-aligned and the destination requires pointer alignment. Function-pointer instance reads either box through RhBox or use the same unaligned-marked direct path. None of these paths use Volatile, LDAR, or STLR.
• NativeAOT ValueType.Equals uses RhBoxAny; its hash-code fallback uses byte spans or Unsafe.As<byte, float/double>, which receives the same unaligned importer treatment. These paths do not share CoreCLR’s reflection volatile accessor.

I built and ran the full packed-field regression as NativeAOT on macOS arm64. The first attempt found that the generic reflected fields were trimmed; the follow-up commit roots public fields on the generic struct parameter. The final NativeAOT build completed with zero warnings/errors and the test passed with expected/actual exit code 100. The test placement predicate now requires a 16-byte atomic-granule crossing only on ARM64 and natural misalignment on other architectures.

Note

This comment was generated with GitHub Copilot.

[Copilot]

Copilot review overview

🟡 Changes recommended

The ARM32 regression test’s allocation predicate can exhaust pinned boxes without exercising the intended reflection path.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite
Findings: 1

Open findings (1)

• 32-bit ARM test cannot find the requested misaligned box · New

[MichalPetryka]

Handle this in VolatileLoad? Do we have a test checking this on 32bit?

[jkotas]

I think this should be more like C# volatile that does not guarantee atomicity for 64-bit values on 32-bit systems.

This Volatile.Read quirk is not without issues. It requires the memory to be writeable. I think it was a mistake to implement Volatile.Read like this.

[jkotas]

Ah, C# does not even allow you to define volatile long.

[MichalPetryka]

This Volatile.Read quirk is not without issues. It requires the memory to be writeable. I think it was a mistake to implement Volatile.Read like this.

I don't think reflection can ever happen to access readonly pages so I'd assume that to not matter here?

[jkotas]

using System.Runtime.InteropServices;

unsafe
{
    MyStruct* ms = ... get a pointer to a readonly memory from somewhere...;
    TypedReference tr = __makeref(*ms);
    Console.WriteLine(typeof(MyStruct).GetField("MyField").GetValueDirect(tr));
}

struct MyStruct
{
    public double MyField;
}

[steveisok]

Agreed. The example with a TypedReference over read-only memory is why the interlocked read is not appropriate.

Fixed in 4691b84380a: GetValueDirect now uses the existing native VolatileLoad for aligned fields and memcpyNoGCRefs for misaligned fields. Neither path writes to or requires writable source memory. The corresponding stores use VolatileStore or the misaligned copy fallback.

This also avoids adding a 64-bit atomicity guarantee on 32-bit systems. Managed cached 8-byte access is disabled on 32-bit so those calls remain on the same native policy.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Low-level runtime changes remain insufficiently covered for direct 8-byte reflection access and warrant final human review.

Review tier: Lite
Findings: None

Resolved findings (1)

• 32-bit ARM test cannot find the requested misaligned box

[steveisok]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[steveisok]

@jkotas none of the CI failures appear related to the change. Is this the right shape?

[jkotas]

Field.cpp/.h is part of type system. Reflection-specific behaviors do not belong here.

Can we move these helpers that implement reflection-specific behaviors to invokeutil.cpp?

It may be best to delete the methods that try to read the value (like GetValue32/SetValue32) in field.h/field.cpp and just have methods that return the field address like GetInstanceAddress. Leave it to the caller to do the actual reference with the right volatile or unaligned treatment.

[jkotas]

NativeAOT audit result: no NativeAOT product change is needed for this bug class

I think this analysis missed some cases.

For example,

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

Line 331 in d4b2951

IntPtr ptrValue = Unsafe.As<byte, IntPtr>(ref Unsafe.Add<byte>(ref typedReference.Value, fieldOffset));

may be doing misaligned read if the TypedReference pointed at misaligned valuetype.

[jkotas]

Instead of introducing this specialized low-level helper, can we deal with the alignment and atomicity in the reflection layer, the same way it is done in regular CoreCLR?