Fix reflection access to misaligned fields in packed structs by steveisok · Pull Request #133828 · dotnet/runtime

steveisok · 2026-09-13T17:04:17Z

On architectures that require natural alignment for acquire/release operations, reflection access to under-aligned fields in packed structs can fault. In particular, FieldDesc::GetInstanceField and SetInstanceField use VolatileLoad and VolatileStore for 1-, 2-, 4-, and 8-byte fields, which can raise SIGBUS on arm64.

This also affects APIs that use reflection internally, such as the field-walking fallback in ValueType.Equals.

This change:

Preserves the existing atomic volatile access for naturally aligned fields.
Uses memcpyNoGCRefs for misaligned non-GC scalar fields.
Routes potentially misaligned primitive, enum, native-int, pointer, and function-pointer instance fields away from the cached managed volatile accessor and through the native slow path.
Routes 8-byte instance fields through the native path on 32-bit platforms, where object alignment alone cannot guarantee 8-byte field alignment.
Adds regression coverage for FieldInfo.GetValue, cached field access, FieldInfo.SetValue, and ValueType.Equals across the affected field types.

Misaligned accesses intentionally do not provide atomic or volatile guarantees. Behavior for naturally aligned fields is unchanged.

Testing

Confirmed the regression test fails against the unmodified runtime with SIGBUS on macOS arm64.
Built checked CoreCLR successfully.
Built and ran the targeted regression test successfully with the fix.

Note

This pull request description was generated with GitHub Copilot.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

azure-pipelines · 2026-09-13T17:04:59Z

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 · 2026-09-13T17:06:02Z

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.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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 · 2026-09-13T21:26:08Z

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

MichalPetryka · 2026-09-13T21:29:24Z

Does this work with EnC?

jkotas · 2026-09-13T21:35:54Z

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

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

steveisok · 2026-09-13T22:03:22Z

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

🔵 Needs a closer look

Alignment-sensitive runtime changes span native and managed reflection paths and require final human review.

Review tier: Lite
Findings: None

steveisok · 2026-09-13T22:55:30Z

Does this work with EnC?

We'll find out

steveisok · 2026-09-13T22:56:59Z

/azp run runtime-extra-platforms

azure-pipelines · 2026-09-13T22:57:12Z

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

MichalPetryka · 2026-09-13T23:06:10Z

On ARM32, the block-copy codegen uses integer ldr/str chunks rather than alignment-sensitive ldrd/vldr long/double operations.

Does that mean that NAOT has a bug in the other way?

steveisok · 2026-09-13T23:20:29Z

On ARM32, the block-copy codegen uses integer ldr/str chunks rather than alignment-sensitive ldrd/vldr long/double operations.

Does that mean that NAOT has a bug in the other way?

Possibly. Could be a separate pre-existing parity issue. Copilot was a little too quick to comment.

jkotas · 2026-09-14T00:55:23Z

+            // Object data is pointer-aligned. On 32-bit platforms, 8-byte
+            // alignment cannot be established from the field offset alone.
+            return alignment <= IntPtr.Size &&
+                (fieldOffset.ToInt64() & (alignment - 1)) == 0;


Suggested change

(fieldOffset.ToInt64() & (alignment - 1)) == 0;

(fieldOffset & (alignment - 1)) == 0;

This should not be necessary

jkotas · 2026-09-14T00:58:48Z

+                FieldAccessorType.InstanceValueTypeSize2 => sizeof(short),
+                FieldAccessorType.InstanceValueTypeSize4 => sizeof(int),
+                FieldAccessorType.InstanceValueTypeSize8 => sizeof(long),


Suggested change

FieldAccessorType.InstanceValueTypeSize2 => sizeof(short),

FieldAccessorType.InstanceValueTypeSize4 => sizeof(int),

FieldAccessorType.InstanceValueTypeSize8 => sizeof(long),

FieldAccessorType.InstanceValueTypeSize2 => 2,

FieldAccessorType.InstanceValueTypeSize4 => 4,

FieldAccessorType.InstanceValueTypeSize8 => 8,

The enum names use 2/4/8 - it is more natural to map them to 2/4/8

jkotas · 2026-09-14T01:07:57Z

Looks like reflection guarantees atomic access to primitive fields on some paths, but not on other paths. We should fix that to be consistent.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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

+        RunWithPinnedBoxAtMisalignedAddress(
+            new PackedInt64AtOffsetZero { Value = InitialValue },
+            fieldOffset,
+            sizeof(long),


MichalPetryka · 2026-09-14T03:17:55Z

+#ifdef TARGET_64BIT
+            *reinterpret_cast<INT64*>(pOutVal) = VolatileLoad(reinterpret_cast<INT64*>(pAddress));
+#else
+            // Match managed Volatile.Read, which guarantees atomic 64-bit access on 32-bit platforms.
+            *reinterpret_cast<INT64*>(pOutVal) =
+                InterlockedCompareExchange64(reinterpret_cast<LONGLONG volatile*>(pAddress), 0, 0);
+#endif


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

Fix reflection access to misaligned fields

db90580

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI lite review requested due to automatic review settings September 13, 2026 17:04

github-actions Bot added the area-VM-coreclr label Sep 13, 2026

Copilot started reviewing on behalf of steveisok September 13, 2026 17:04 View session

dotnet-policy-service Bot assigned steveisok Sep 13, 2026

steveisok requested a review from a team September 13, 2026 17:12

Copilot AI reviewed Sep 13, 2026

View reviewed changes

Comment thread src/coreclr/vm/field.cpp Outdated

Comment thread src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs

Handle misaligned direct reflection access

8d240e0

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 13, 2026 18:51

Copilot started reviewing on behalf of steveisok September 13, 2026 18:51 View session

Copilot AI reviewed Sep 13, 2026

View reviewed changes

jkotas added area-System.Reflection and removed area-VM-coreclr labels Sep 13, 2026

jkotas reviewed Sep 13, 2026

View reviewed changes

Comment thread src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs

Refine packed field alignment handling

630846c

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 13, 2026 22:03

Copilot started reviewing on behalf of steveisok September 13, 2026 22:03 View session

build-analysis Bot mentioned this pull request Sep 13, 2026

github60486 failed with NRE at Program.ValidateRecursiveCurrentMethod(Int32 depth) #133798

Open

Copilot AI reviewed Sep 13, 2026

View reviewed changes

jkotas reviewed Sep 14, 2026

View reviewed changes

Make primitive reflection access consistent

9192c64

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 14, 2026 02:59

Copilot started reviewing on behalf of steveisok September 14, 2026 03:00 View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

Comment thread src/tests/Regressions/coreclr/GitHub_133236/test133236.cs

Comment on lines +102 to +105

RunWithPinnedBoxAtMisalignedAddress(

new PackedInt64AtOffsetZero { Value = InitialValue },

fieldOffset,

sizeof(long),

MichalPetryka reviewed Sep 14, 2026

View reviewed changes

	(fieldOffset.ToInt64() & (alignment - 1)) == 0;
	(fieldOffset & (alignment - 1)) == 0;

Uh oh!

Conversation

steveisok commented Sep 13, 2026

Testing

Uh oh!

azure-pipelines Bot commented Sep 13, 2026

Uh oh!

dotnet-policy-service Bot commented Sep 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

dotnet-policy-service Bot commented Sep 13, 2026

Uh oh!

MichalPetryka commented Sep 13, 2026

Uh oh!

Uh oh!

jkotas commented Sep 13, 2026

Uh oh!

steveisok commented Sep 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

steveisok commented Sep 13, 2026

Uh oh!

steveisok commented Sep 13, 2026

Uh oh!

azure-pipelines Bot commented Sep 13, 2026

Uh oh!

MichalPetryka commented Sep 13, 2026

Uh oh!

steveisok commented Sep 13, 2026

Uh oh!

jkotas Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

MichalPetryka Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants