RuntimeHelpers.Equals incorrectly compares an extra four bytes on x64

[jnm2]

Description

Discovered in .NET Framework 4.8 and still present in .NET 8.0. (Please fix in .NET Framework as well.)

sizeof(int) (4) is subtracted instead 8 for x64, causing an extra four bytes past the end of the boxed struct value to be compared:

runtime/src/coreclr/classlibnative/bcltype/objectnative.cpp

Lines 160 to 167 in 00874d6

	// Compare the contents (size - vtable - sync block index).
	DWORD dwBaseSize = pThisMT->GetBaseSize();
	if(pThisMT == g_pStringClass)
	dwBaseSize -= sizeof(WCHAR);
	BOOL ret = memcmp(
	(void *) (pThisRef+1),
	(void *) (pCompareRef+1),
	dwBaseSize - sizeof(Object) - sizeof(int)) == 0;

That sizeof(int) line was migrated as-is when this Git history started.

Reproduction Steps

For .NET Framework, where the bug was found, I'm not how to set up a repro. Four bytes past the end of most boxed values is often going to go into the object header of another object, and the object header of another object on x64 is four zero bytes for padding followed by four sync block bytes. Those four padding bytes are probably why repros are so hard to come by. There must have been a larger object that was freed, and this boxed object was allocated over memory that had not been zeroed.

My team members and I have been seeing this bug in RuntimeHelpers.Equals every month or two where it incorrectly returns false and triggers one of our debug assertions. When it incorrectly returns false, I verified that the boxed values being compared are identical in raw memory, but the four bytes following the boxed object do differ from zero in the cases where RuntimeHelpers.Equals is incorrectly returning false.

For .NET 6 and 8, this repro works instantly (thanks @EgorBo):

using System.Runtime.CompilerServices;

while (true)
{
    if (!RuntimeHelpers.Equals(default(Guid), default(Guid)))
        throw new Exception("!");
}

Expected behavior

RuntimeHelpers.Equals should compare only the instance data of the boxed values.

Actual behavior

RuntimeHelpers.Equals appears to compare an additional four bytes past the end of the instance data of the boxed values.

Regression?

This is broken in .NET Framework 4.8 as well.

Known Workarounds

No workarounds known since comparing memory inside boxed structs is not safe without pausing GCs.

Configuration

No response

Other information

No response

[EgorBo]

Great find! Object's header (8 bytes) should never contain any garbage in its upper 4 bytes, but, presumably, garbage values could be in the padding between objects (due to 8-byte alignment) since GC supports allocations on unitialized memory (e.g. GC.AllocateUnitialized). I wonder if we should deprecate this API. Users can use plain unsafe + SequenceEqual if they want to compare structs via memcmp.

[EgorBo]

#97174 could be related

[jnm2]

@EgorBo I really want this API to stay. It's essential for comparing boxed structs that contain references without deferring to how the struct defines Equals. (I've had bugs due to Equals being a looser comparison, e.g. with System.Decimal.)

RuntimeHelpers.Equals(x, y) answers the question "could any difference be seen if I assign y over top of x."

One example of where I use it is in INotifyPropertyChanged comparisons, for which it's the perfect answer to loose Equals overrides causing bugs, again due to the nature of how it answers "could a difference be seen if this assignment were to be done." It's also useful in other scenarios.

The fix looks as straightforward as they come, so I'm very hopeful that it gets fixed in .NET Framework and .NET 6+.

[EgorBo]

Minimal repro (net8.0, Release, Windows-11 x64):

using System.Runtime.CompilerServices;

long i = 0;
while (true)
{
    i++;
    Guid g1 = default;
    Guid g2 = default;
    if (!RuntimeHelpers.Equals(g1, g2))
        throw new Exception($" failed, i={i}");
}

[huoyaoyuan]

I'm touching it in #97641
@jkotas mentioned it was already broken since really long time ago in #19379.

[ghost]

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

Issue Details

---

Description

Discovered in .NET Framework 4.8 and still present in .NET 8.0. (Please fix in .NET Framework as well.)

sizeof(int) (4) is subtracted instead 8 for x64, causing an extra four bytes past the end of the boxed struct value to be compared:

runtime/src/coreclr/classlibnative/bcltype/objectnative.cpp

Lines 160 to 167 in 00874d6

	// Compare the contents (size - vtable - sync block index).
	DWORD dwBaseSize = pThisMT->GetBaseSize();
	if(pThisMT == g_pStringClass)
	dwBaseSize -= sizeof(WCHAR);
	BOOL ret = memcmp(
	(void *) (pThisRef+1),
	(void *) (pCompareRef+1),
	dwBaseSize - sizeof(Object) - sizeof(int)) == 0;

That sizeof(int) line was migrated as-is when this Git history started.

Reproduction Steps

For .NET Framework, where the bug was found, I'm not how to set up a repro. Four bytes past the end of most boxed values is often going to go into the object header of another object, and the object header of another object on x64 is four zero bytes for padding followed by four sync block bytes. Those four padding bytes are probably why repros are so hard to come by. There must have been a larger object that was freed, and this boxed object was allocated over memory that had not been zeroed.

My team members and I have been seeing this bug in RuntimeHelpers.Equals every month or two where it incorrectly returns false and triggers one of our debug assertions. When it incorrectly returns false, I verified that the boxed values being compared are identical in raw memory, but the four bytes following the boxed object do differ from zero in the cases where RuntimeHelpers.Equals is incorrectly returning false.

For .NET 6 and 8, this repro works instantly (thanks @EgorBo):

using System.Runtime.CompilerServices;

while (true)
{
    if (!RuntimeHelpers.Equals(default(Guid), default(Guid)))
        throw new Exception("!");
}

Expected behavior

RuntimeHelpers.Equals should compare only the instance data of the boxed values.

Actual behavior

RuntimeHelpers.Equals appears to compare an additional four bytes past the end of the instance data of the boxed values.

Regression?

This is broken in .NET Framework 4.8 as well.

Known Workarounds

No workarounds known since comparing memory inside boxed structs is not safe without pausing GCs.

Configuration

No response

Other information

No response

Author:	jnm2
Assignees:	-
Labels:	area-System.Runtime.CompilerServices, untriaged, needs-area-label
Milestone:	-

[EgorBo]

Closed via #97641 in Main

[jnm2]

Requested fix in .NET Framework here: https://developercommunity.visualstudio.com/t/RuntimeHelpersEquals-compares-invalid-m/10602858

[EgorBo]

Requested fix in .NET Framework here

The bar for servicing fixes is high (especially for .NET Framework). We don't expect this API to be popular (in fact, I only found a few uses) + we consider it as a quite dangerous API since it may produce unexpected/random results when structs have paddings/gaps or floating point fields. So IMO it does not meet that bar, unless other folks have a different opinion

[jnm2]

Right now, it's the only API that "sees through" boxed copies of struct values without calling customized Equals logic. Boxing happens as part of data binding, e.g. PropertyDescriptor.GetValue, and during my usage of RuntimeHelpers.Equals, there have been no issues with struct field gaps thus far. I understand the concerns, but it's also providing a valuable feature that has no alternative. For instance, there's a need to distinguish whether an assignment will change 0.1m to 0.10m and thus whether to raise a PropertyChanged event, where neither Equals nor == provide the desired behavior.

In general, the needed API says: "could an assignment of y over x have an effect?" RuntimeHelpers.Equals currently serves this purpose for any possible objects passed. object.Equals cannot serve this purpose because it can be overridden in structs, and object.ReferenceEquals cannot serve this purpose because it returns false for separate boxes of the same value. For a fully general API, it also needs to be able to pause GC when there are object references in the structs.

Is it possible to reproduce an issue where two different boxed copies of the same struct value have different contents in gaps?