From d0bb2236a4af0d174165ab670963e4cdc76cbfce Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Wed, 7 Aug 2024 12:33:11 -0400 Subject: [PATCH 1/2] [Mono.Cecil] Enforce empty body when a MethodDefinition's rva value is 0 The prior behavior assumed a MethodDefinition's RVA value of 0 meant an effectively empty body with a single RET instruction. This change removes the RET insertion and leaves the method completely empty. This is important for mono's UnsafeAccessor detection around methods specified as extern as it uses a quicker method of an empty vs non-empty body to bypass loading / checking custom attributes. Fixes https://github.com/dotnet/runtime/issues/102045 --- .../src/Mono.Cecil/MethodDefinition.cs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.DotNet.CilStrip.Sources/src/Mono.Cecil/MethodDefinition.cs b/src/Microsoft.DotNet.CilStrip.Sources/src/Mono.Cecil/MethodDefinition.cs index 16d37d0bc..3ff3bc3de 100644 --- a/src/Microsoft.DotNet.CilStrip.Sources/src/Mono.Cecil/MethodDefinition.cs +++ b/src/Microsoft.DotNet.CilStrip.Sources/src/Mono.Cecil/MethodDefinition.cs @@ -49,6 +49,8 @@ internal sealed class MethodDefinition : MethodReference, IMemberDefinition, PInvokeInfo m_pinvoke; readonly ParameterDefinition m_this; + bool m_enforceNoBody; + public MethodAttributes Attributes { get { return m_attributes; } set { m_attributes = value; } @@ -524,7 +526,8 @@ public bool HasBody { (m_implAttrs & MethodImplAttributes.InternalCall) == 0 && (m_implAttrs & MethodImplAttributes.Native) == 0 && (m_implAttrs & MethodImplAttributes.Unmanaged) == 0 && - (m_implAttrs & MethodImplAttributes.Runtime) == 0; + (m_implAttrs & MethodImplAttributes.Runtime) == 0 && + (!m_enforceNoBody); } } @@ -563,7 +566,19 @@ public MethodDefinition (string name, MethodAttributes attrs, TypeReference retu internal void LoadBody () { - if (m_body == null && this.HasBody) { + // if m_rva is zero, it is assumed that there is no method body and it will not even + // try to read the actual code size value. Therefore, if it is zero, make sure to enforce + // there is no body. This is important for extern methods as there is no equivalent IL to detect. + // + // The default in cecil if you do have a method body is to have at least a RET instruction. If we were to let extern methods + // fall through, then that interfere with mono's UnsafeAccessor detection as it tries to detect via body vs no body because + // it is much cheaper. + // + // See https://github.com/dotnet/runtime/issues/102045 + if (m_rva == RVA.Zero) { + m_enforceNoBody = true; + } + else if (m_body == null && this.HasBody) { m_body = new MethodBody (this); ModuleDefinition module = DeclaringType != null ? DeclaringType.Module : null; From aa357d9b757207ec14d17cb7a2e0093473223baf Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Wed, 7 Aug 2024 12:43:21 -0400 Subject: [PATCH 2/2] Type-o --- .../src/Mono.Cecil/MethodDefinition.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.DotNet.CilStrip.Sources/src/Mono.Cecil/MethodDefinition.cs b/src/Microsoft.DotNet.CilStrip.Sources/src/Mono.Cecil/MethodDefinition.cs index 3ff3bc3de..4b22dd458 100644 --- a/src/Microsoft.DotNet.CilStrip.Sources/src/Mono.Cecil/MethodDefinition.cs +++ b/src/Microsoft.DotNet.CilStrip.Sources/src/Mono.Cecil/MethodDefinition.cs @@ -571,7 +571,7 @@ internal void LoadBody () // there is no body. This is important for extern methods as there is no equivalent IL to detect. // // The default in cecil if you do have a method body is to have at least a RET instruction. If we were to let extern methods - // fall through, then that interfere with mono's UnsafeAccessor detection as it tries to detect via body vs no body because + // fall through, then that would interfere with mono's UnsafeAccessor detection as it tries to detect via body vs no body because // it is much cheaper. // // See https://github.com/dotnet/runtime/issues/102045