From 3c1bd85a51cb7cf44e935d73a27db692713eb509 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Tue, 1 Feb 2022 12:21:57 -0800 Subject: [PATCH 1/5] Add tests for overlap and non-aligned ref fields. --- src/coreclr/vm/methodtablebuilder.cpp | 22 +++++------ src/mono/mono/metadata/class-init.c | 4 ++ .../classloader/RefFields/InvalidCSharp.il | 37 ++++++++++++++++--- .../Loader/classloader/RefFields/Validate.cs | 2 + 4 files changed, 49 insertions(+), 16 deletions(-) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 82499f314e4f3d..b2d3921fe6df84 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -8391,7 +8391,6 @@ MethodTableBuilder::HandleExplicitLayout( { STANDARD_VM_CONTRACT; - // Instance slice size is the total size of an instance, and is calculated as // the field whose offset and size add to the greatest number. UINT instanceSliceSize = 0; @@ -8426,15 +8425,15 @@ MethodTableBuilder::HandleExplicitLayout( pFieldLayout[i] = empty; } - // go through each field and look for invalid layout + // Go through each field and look for invalid layout. // (note that we are more permissive than what Ecma allows. We only disallow the minimum set necessary to // close security holes.) // - // This is what we implment: + // This is what we implement: // - // 1. Verify that every OREF is on a valid alignment - // 2. Verify that OREFs only overlap with other OREFs. - // 3. If an OREF does overlap with another OREF, the class is marked unverifiable. + // 1. Verify that every OREF or BYREF is on a valid alignment. + // 2. Verify that OREFs or BYREFs only overlap with other OREFs or BYREFs. + // 3. If an OREF or BYREF does overlap with another OREF or BYREF, the class is marked unverifiable. // 4. If an overlap of any kind occurs, the class will be marked NotTightlyPacked (affects ValueType.Equals()). // char emptyObject[TARGET_POINTER_SIZE]; @@ -8481,7 +8480,8 @@ MethodTableBuilder::HandleExplicitLayout( // "i" indexes all fields, valueClassCacheIndex indexes non-static fields only. Don't get them confused! valueClassCacheIndex++; - if (CorTypeInfo::IsObjRef(pFD->GetFieldType())) + CorElementType type = pFD->GetFieldType(); + if (CorTypeInfo::IsObjRef(type) || CorTypeInfo::IsByRef(type)) { // Check that the ref offset is pointer aligned if ((pFD->GetOffset_NoLogging() & ((ULONG)TARGET_POINTER_SIZE - 1)) != 0) @@ -8489,13 +8489,13 @@ MethodTableBuilder::HandleExplicitLayout( badOffset = pFD->GetOffset_NoLogging(); fieldTrust.SetTrust(ExplicitFieldTrust::kNone); - // If we got here, OREF field was not pointer aligned. THROW. + // If we got here, OREF or BYREF field was not pointer aligned. THROW. break; } // check if overlaps another object if (memcmp((void *)&pFieldLayout[pFD->GetOffset_NoLogging()], (void *)isObject, sizeof(isObject)) == 0) { - // If we got here, an OREF overlapped another OREF. We permit this but mark the class unverifiable. + // If we got here, an OREF or BYREF overlapped another OREF or BYREF. We permit this but mark the class unverifiable. fieldTrust.SetTrust(ExplicitFieldTrust::kLegal); if (firstObjectOverlapOffset == ((DWORD)(-1))) @@ -8508,13 +8508,13 @@ MethodTableBuilder::HandleExplicitLayout( // check if is empty at this point if (memcmp((void *)&pFieldLayout[pFD->GetOffset_NoLogging()], (void *)emptyObject, sizeof(emptyObject)) == 0) { - // If we got here, this OREF is overlapping no other fields (yet). Record that these bytes now contain an OREF. + // If we got here, this OREF or BYREF is overlapping no other fields (yet). Record that these bytes now contain an OREF or BYREF . memset((void *)&pFieldLayout[pFD->GetOffset_NoLogging()], oref, sizeof(isObject)); fieldTrust.SetTrust(ExplicitFieldTrust::kNonOverLayed); continue; } - // If we got here, the OREF overlaps a non-OREF. THROW. + // If we got here, the OREF or BYREF overlaps a non-OREF or non-BYREF. THROW. badOffset = pFD->GetOffset_NoLogging(); fieldTrust.SetTrust(ExplicitFieldTrust::kNone); break; diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 00ab30ffbd76f2..b042f6cddec48e 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -346,6 +346,10 @@ mono_class_setup_fields (MonoClass *klass) mono_class_set_type_load_failure (klass, "Missing field layout info for %s", field->name); break; } + if (m_type_is_byref (field->type) && (offset % MONO_ABI_ALIGNOF (gpointer) != 0)) { + mono_class_set_type_load_failure (klass, "Field '%s' has an invalid offset", field->name, offset); + break; + } if (offset < -1) { /*-1 is used to encode special static fields */ mono_class_set_type_load_failure (klass, "Field '%s' has a negative offset %d", field->name, offset); break; diff --git a/src/tests/Loader/classloader/RefFields/InvalidCSharp.il b/src/tests/Loader/classloader/RefFields/InvalidCSharp.il index cb012cf23b7ecb..e832d275f78ce9 100644 --- a/src/tests/Loader/classloader/RefFields/InvalidCSharp.il +++ b/src/tests/Loader/classloader/RefFields/InvalidCSharp.il @@ -5,17 +5,44 @@ .assembly InvalidCSharp { } -.class public auto ansi sealed beforefieldinit InvalidCSharp.InvalidStructWithRefField +.class public sequential ansi sealed beforefieldinit InvalidCSharp.InvalidStructWithRefField extends [System.Runtime]System.ValueType { // Type requires IsByRefLikeAttribute to be valid. .field public string& invalid } +.class public explicit ansi sealed beforefieldinit InvalidCSharp.InvalidRefFieldAlignment + extends [System.Runtime]System.ValueType +{ + .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( + 01 00 00 00 + ) + .field [0] public int16 X + .field [2] public int32& Invalid +} + +.class public sequential ansi sealed beforefieldinit InvalidCSharp.ContainsRef + extends [System.Runtime]System.ValueType +{ + .field public object OverlapsWithX + .field public int32 Y +} + +.class public explicit ansi sealed beforefieldinit InvalidCSharp.InvalidRefFieldOverlap + extends [System.Runtime]System.ValueType +{ + .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( + 01 00 00 00 + ) + .field [0] public int32 X + .field [0] public valuetype InvalidCSharp.ContainsRef Invalid +} + // This is invalid metadata and is unable to be loaded. // - [field sig] (0x80131815 (VER_E_FIELD_SIG)) // -// .class public auto ansi sealed beforefieldinit InvalidCSharp.InvalidStructWithStaticRefField +// .class public sequential ansi sealed beforefieldinit InvalidCSharp.InvalidStructWithStaticRefField // extends [System.Runtime]System.ValueType // { // .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( @@ -24,7 +51,7 @@ // .field public static string& invalid // } -.class public auto ansi sealed beforefieldinit InvalidCSharp.WithRefField +.class public sequential ansi sealed beforefieldinit InvalidCSharp.WithRefField extends [System.Runtime]System.ValueType { .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( @@ -57,7 +84,7 @@ } } -.class public auto ansi sealed beforefieldinit InvalidCSharp.WithRefStructField +.class public sequential ansi sealed beforefieldinit InvalidCSharp.WithRefStructField extends [System.Runtime]System.ValueType { .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( @@ -91,7 +118,7 @@ } } -.class public auto ansi sealed beforefieldinit InvalidCSharp.WithTypedReferenceField`1 +.class public sequential ansi sealed beforefieldinit InvalidCSharp.WithTypedReferenceField`1 extends [System.Runtime]System.ValueType { .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( diff --git a/src/tests/Loader/classloader/RefFields/Validate.cs b/src/tests/Loader/classloader/RefFields/Validate.cs index 3e8d3b76d23d00..c9a6d4e6a06081 100644 --- a/src/tests/Loader/classloader/RefFields/Validate.cs +++ b/src/tests/Loader/classloader/RefFields/Validate.cs @@ -15,6 +15,8 @@ public static void Validate_Invalid_RefField_Fails() { Console.WriteLine($"{nameof(Validate_Invalid_RefField_Fails)}..."); Assert.Throws(() => { var t = typeof(InvalidStructWithRefField); }); + Assert.Throws(() => { var t = typeof(InvalidRefFieldAlignment); }); + Assert.Throws(() => { var t = typeof(InvalidRefFieldOverlap); }); } [Fact] From f8ab0737df195570556f8ea26ec9a7a817ea6738 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Tue, 1 Feb 2022 13:08:47 -0800 Subject: [PATCH 2/5] Remove unused parameter from varargs string format. --- src/mono/mono/metadata/class-init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index b042f6cddec48e..36fdd82960f83b 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -347,7 +347,7 @@ mono_class_setup_fields (MonoClass *klass) break; } if (m_type_is_byref (field->type) && (offset % MONO_ABI_ALIGNOF (gpointer) != 0)) { - mono_class_set_type_load_failure (klass, "Field '%s' has an invalid offset", field->name, offset); + mono_class_set_type_load_failure (klass, "Field '%s' has an invalid offset", field->name); break; } if (offset < -1) { /*-1 is used to encode special static fields */ From 4b6e1998e7f91ffeac3ab2e5ba55aef3ff79bb13 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Tue, 1 Feb 2022 15:41:13 -0800 Subject: [PATCH 3/5] Block the overlap of ref field and objectref. --- src/coreclr/vm/methodtablebuilder.cpp | 19 +++++++++++++------ .../classloader/RefFields/InvalidCSharp.il | 19 ++++++------------- .../Loader/classloader/RefFields/Validate.cs | 2 +- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index b2d3921fe6df84..048513a5f52cca 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -8432,8 +8432,8 @@ MethodTableBuilder::HandleExplicitLayout( // This is what we implement: // // 1. Verify that every OREF or BYREF is on a valid alignment. - // 2. Verify that OREFs or BYREFs only overlap with other OREFs or BYREFs. - // 3. If an OREF or BYREF does overlap with another OREF or BYREF, the class is marked unverifiable. + // 2. Verify that OREFs only overlap with other OREFs. + // 3. If an OREF does overlap with another OREF, the class is marked unverifiable. // 4. If an overlap of any kind occurs, the class will be marked NotTightlyPacked (affects ValueType.Equals()). // char emptyObject[TARGET_POINTER_SIZE]; @@ -8444,7 +8444,6 @@ MethodTableBuilder::HandleExplicitLayout( isObject[i] = oref; } - ExplicitClassTrust explicitClassTrust; UINT valueClassCacheIndex = ((UINT)(-1)); @@ -8492,10 +8491,18 @@ MethodTableBuilder::HandleExplicitLayout( // If we got here, OREF or BYREF field was not pointer aligned. THROW. break; } + + // Other than alignment, a byref can be treated like an native pointer. + if (CorTypeInfo::IsByRef(type)) + type = ELEMENT_TYPE_I; + } + + if (CorTypeInfo::IsObjRef(type)) + { // check if overlaps another object if (memcmp((void *)&pFieldLayout[pFD->GetOffset_NoLogging()], (void *)isObject, sizeof(isObject)) == 0) { - // If we got here, an OREF or BYREF overlapped another OREF or BYREF. We permit this but mark the class unverifiable. + // If we got here, an OREF overlapped another OREF. We permit this but mark the class unverifiable. fieldTrust.SetTrust(ExplicitFieldTrust::kLegal); if (firstObjectOverlapOffset == ((DWORD)(-1))) @@ -8508,13 +8515,13 @@ MethodTableBuilder::HandleExplicitLayout( // check if is empty at this point if (memcmp((void *)&pFieldLayout[pFD->GetOffset_NoLogging()], (void *)emptyObject, sizeof(emptyObject)) == 0) { - // If we got here, this OREF or BYREF is overlapping no other fields (yet). Record that these bytes now contain an OREF or BYREF . + // If we got here, this OREF is overlapping no other fields (yet). Record that these bytes now contain an OREF. memset((void *)&pFieldLayout[pFD->GetOffset_NoLogging()], oref, sizeof(isObject)); fieldTrust.SetTrust(ExplicitFieldTrust::kNonOverLayed); continue; } - // If we got here, the OREF or BYREF overlaps a non-OREF or non-BYREF. THROW. + // If we got here, the OREF overlaps a non-OREF. THROW. badOffset = pFD->GetOffset_NoLogging(); fieldTrust.SetTrust(ExplicitFieldTrust::kNone); break; diff --git a/src/tests/Loader/classloader/RefFields/InvalidCSharp.il b/src/tests/Loader/classloader/RefFields/InvalidCSharp.il index e832d275f78ce9..768f603f22d55c 100644 --- a/src/tests/Loader/classloader/RefFields/InvalidCSharp.il +++ b/src/tests/Loader/classloader/RefFields/InvalidCSharp.il @@ -9,7 +9,7 @@ extends [System.Runtime]System.ValueType { // Type requires IsByRefLikeAttribute to be valid. - .field public string& invalid + .field public string& Invalid } .class public explicit ansi sealed beforefieldinit InvalidCSharp.InvalidRefFieldAlignment @@ -18,25 +18,18 @@ .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( 01 00 00 00 ) - .field [0] public int16 X + .field [0] public int16 Field .field [2] public int32& Invalid } -.class public sequential ansi sealed beforefieldinit InvalidCSharp.ContainsRef - extends [System.Runtime]System.ValueType -{ - .field public object OverlapsWithX - .field public int32 Y -} - -.class public explicit ansi sealed beforefieldinit InvalidCSharp.InvalidRefFieldOverlap +.class public explicit ansi sealed beforefieldinit InvalidCSharp.InvalidObjectRefRefFieldOverlap extends [System.Runtime]System.ValueType { .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( 01 00 00 00 ) - .field [0] public int32 X - .field [0] public valuetype InvalidCSharp.ContainsRef Invalid + .field [0] public object Field + .field [0] public int32& Invalid } // This is invalid metadata and is unable to be loaded. @@ -48,7 +41,7 @@ // .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = ( // 01 00 00 00 // ) -// .field public static string& invalid +// .field public static string& Invalid // } .class public sequential ansi sealed beforefieldinit InvalidCSharp.WithRefField diff --git a/src/tests/Loader/classloader/RefFields/Validate.cs b/src/tests/Loader/classloader/RefFields/Validate.cs index c9a6d4e6a06081..b286d51f2f6aae 100644 --- a/src/tests/Loader/classloader/RefFields/Validate.cs +++ b/src/tests/Loader/classloader/RefFields/Validate.cs @@ -16,7 +16,7 @@ public static void Validate_Invalid_RefField_Fails() Console.WriteLine($"{nameof(Validate_Invalid_RefField_Fails)}..."); Assert.Throws(() => { var t = typeof(InvalidStructWithRefField); }); Assert.Throws(() => { var t = typeof(InvalidRefFieldAlignment); }); - Assert.Throws(() => { var t = typeof(InvalidRefFieldOverlap); }); + Assert.Throws(() => { var t = typeof(InvalidObjectRefRefFieldOverlap); }); } [Fact] From 2e600cd141eb442615df641eef882a7e6251b989 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Tue, 1 Feb 2022 16:03:45 -0800 Subject: [PATCH 4/5] Apply suggestions from code review --- src/coreclr/vm/methodtablebuilder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 048513a5f52cca..67e0a7fa6a0019 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -8492,7 +8492,7 @@ MethodTableBuilder::HandleExplicitLayout( break; } - // Other than alignment, a byref can be treated like an native pointer. + // Other than alignment, a byref can be treated like a native pointer. if (CorTypeInfo::IsByRef(type)) type = ELEMENT_TYPE_I; } From 592a98bddc00581484c3517428f2a42884c4af52 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Tue, 1 Feb 2022 18:17:32 -0800 Subject: [PATCH 5/5] Review feedback. --- src/coreclr/vm/methodtablebuilder.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 67e0a7fa6a0019..19a63dd9255443 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -8491,10 +8491,6 @@ MethodTableBuilder::HandleExplicitLayout( // If we got here, OREF or BYREF field was not pointer aligned. THROW. break; } - - // Other than alignment, a byref can be treated like a native pointer. - if (CorTypeInfo::IsByRef(type)) - type = ELEMENT_TYPE_I; } if (CorTypeInfo::IsObjRef(type))