Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix alignment padding and add test for saving managed resources #110915

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+174 −74
Open

Fix alignment padding and add test for saving managed resources #110915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f21da2f
Add test for saving and reading managed resources
steveharter Dec 23, 2024
d199ed7
Use TempFile helper for tests; align Resource section
steveharter Dec 23, 2024
c011569
Align just the resource buffer
steveharter Dec 23, 2024
f857e01
Verify ManagedPEBuilder.Serialize() doesn't modify incoming resource …
steveharter Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 64 additions & 64 deletions src/libraries/System.Reflection.Emit/tests/AssemblyBuilderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,100 +411,100 @@ private static void VerifyAssemblyBuilder(AssemblyBuilder assembly, AssemblyName
Assert.Empty(assembly.GetTypes());
}

private static void SamplePrivateMethod ()
{
}
private static void SamplePrivateMethod()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace-only changes to this file

{
}

internal static void SampleInternalMethod ()
{
}
internal static void SampleInternalMethod()
{
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
void Invoke_Private_CrossAssembly_ThrowsMethodAccessException()
{
TypeBuilder tb = Helpers.DynamicType(TypeAttributes.Public);
var mb = tb.DefineMethod ("MyMethod", MethodAttributes.Public | MethodAttributes.Static, typeof(void), new Type[] { });
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
void Invoke_Private_CrossAssembly_ThrowsMethodAccessException()
{
TypeBuilder tb = Helpers.DynamicType(TypeAttributes.Public);
var mb = tb.DefineMethod("MyMethod", MethodAttributes.Public | MethodAttributes.Static, typeof(void), new Type[] { });

var ilg = mb.GetILGenerator ();
var ilg = mb.GetILGenerator();

var callee = typeof (AssemblyTests).GetMethod ("SamplePrivateMethod", BindingFlags.Static | BindingFlags.NonPublic);
var callee = typeof(AssemblyTests).GetMethod("SamplePrivateMethod", BindingFlags.Static | BindingFlags.NonPublic);

ilg.Emit (OpCodes.Call, callee);
ilg.Emit (OpCodes.Ret);
ilg.Emit(OpCodes.Call, callee);
ilg.Emit(OpCodes.Ret);

var ty = tb.CreateType ();
var ty = tb.CreateType();

var mi = ty.GetMethod ("MyMethod", BindingFlags.Static | BindingFlags.Public);
var mi = ty.GetMethod("MyMethod", BindingFlags.Static | BindingFlags.Public);

var d = (Action) mi.CreateDelegate (typeof(Action));
var d = (Action)mi.CreateDelegate(typeof(Action));

Assert.Throws<MethodAccessException>(() => d ());
}
Assert.Throws<MethodAccessException>(() => d());
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
void Invoke_Internal_CrossAssembly_ThrowsMethodAccessException()
{
TypeBuilder tb = Helpers.DynamicType(TypeAttributes.Public);
var mb = tb.DefineMethod ("MyMethod", MethodAttributes.Public | MethodAttributes.Static, typeof(void), new Type[] { });
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
void Invoke_Internal_CrossAssembly_ThrowsMethodAccessException()
{
TypeBuilder tb = Helpers.DynamicType(TypeAttributes.Public);
var mb = tb.DefineMethod("MyMethod", MethodAttributes.Public | MethodAttributes.Static, typeof(void), new Type[] { });

var ilg = mb.GetILGenerator ();
var ilg = mb.GetILGenerator();

var callee = typeof (AssemblyTests).GetMethod ("SampleInternalMethod", BindingFlags.Static | BindingFlags.NonPublic);
var callee = typeof(AssemblyTests).GetMethod("SampleInternalMethod", BindingFlags.Static | BindingFlags.NonPublic);

ilg.Emit (OpCodes.Call, callee);
ilg.Emit (OpCodes.Ret);
ilg.Emit(OpCodes.Call, callee);
ilg.Emit(OpCodes.Ret);

var ty = tb.CreateType ();
var ty = tb.CreateType();

var mi = ty.GetMethod ("MyMethod", BindingFlags.Static | BindingFlags.Public);
var mi = ty.GetMethod("MyMethod", BindingFlags.Static | BindingFlags.Public);

var d = (Action) mi.CreateDelegate (typeof(Action));
var d = (Action)mi.CreateDelegate(typeof(Action));

Assert.Throws<MethodAccessException>(() => d ());
}
Assert.Throws<MethodAccessException>(() => d());
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
void Invoke_Private_SameAssembly_ThrowsMethodAccessException()
{
ModuleBuilder modb = Helpers.DynamicModule();
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
void Invoke_Private_SameAssembly_ThrowsMethodAccessException()
{
ModuleBuilder modb = Helpers.DynamicModule();

string calleeName = "PrivateMethod";
string calleeName = "PrivateMethod";

TypeBuilder tbCalled = modb.DefineType ("CalledClass", TypeAttributes.Public);
var mbCalled = tbCalled.DefineMethod (calleeName, MethodAttributes.Private | MethodAttributes.Static);
mbCalled.GetILGenerator().Emit (OpCodes.Ret);
TypeBuilder tbCalled = modb.DefineType("CalledClass", TypeAttributes.Public);
var mbCalled = tbCalled.DefineMethod(calleeName, MethodAttributes.Private | MethodAttributes.Static);
mbCalled.GetILGenerator().Emit(OpCodes.Ret);

var tyCalled = tbCalled.CreateType();
var callee = tyCalled.GetMethod (calleeName, BindingFlags.NonPublic | BindingFlags.Static);
var tyCalled = tbCalled.CreateType();
var callee = tyCalled.GetMethod(calleeName, BindingFlags.NonPublic | BindingFlags.Static);

TypeBuilder tb = modb.DefineType("CallerClass", TypeAttributes.Public);
var mb = tb.DefineMethod ("MyMethod", MethodAttributes.Public | MethodAttributes.Static, typeof(void), new Type[] { });
TypeBuilder tb = modb.DefineType("CallerClass", TypeAttributes.Public);
var mb = tb.DefineMethod("MyMethod", MethodAttributes.Public | MethodAttributes.Static, typeof(void), new Type[] { });

var ilg = mb.GetILGenerator ();
var ilg = mb.GetILGenerator();

ilg.Emit (OpCodes.Call, callee);
ilg.Emit (OpCodes.Ret);
ilg.Emit(OpCodes.Call, callee);
ilg.Emit(OpCodes.Ret);

var ty = tb.CreateType ();
var ty = tb.CreateType();

var mi = ty.GetMethod ("MyMethod", BindingFlags.Static | BindingFlags.Public);
var mi = ty.GetMethod("MyMethod", BindingFlags.Static | BindingFlags.Public);

var d = (Action) mi.CreateDelegate (typeof(Action));
var d = (Action)mi.CreateDelegate(typeof(Action));

Assert.Throws<MethodAccessException>(() => d ());
}
Assert.Throws<MethodAccessException>(() => d());
}

[Fact]
public void DefineDynamicAssembly_AssemblyBuilderLocationIsEmpty_InternalAssemblyBuilderLocationIsEmpty()
{
AssemblyBuilder assembly = Helpers.DynamicAssembly(nameof(DefineDynamicAssembly_AssemblyBuilderLocationIsEmpty_InternalAssemblyBuilderLocationIsEmpty));
Assembly internalAssemblyBuilder = AppDomain.CurrentDomain.GetAssemblies()
.FirstOrDefault(a => a.FullName == assembly.FullName);
[Fact]
public void DefineDynamicAssembly_AssemblyBuilderLocationIsEmpty_InternalAssemblyBuilderLocationIsEmpty()
{
AssemblyBuilder assembly = Helpers.DynamicAssembly(nameof(DefineDynamicAssembly_AssemblyBuilderLocationIsEmpty_InternalAssemblyBuilderLocationIsEmpty));
Assembly internalAssemblyBuilder = AppDomain.CurrentDomain.GetAssemblies()
.FirstOrDefault(a => a.FullName == assembly.FullName);

Assert.Empty(assembly.Location);
Assert.NotNull(internalAssemblyBuilder);
Assert.Empty(internalAssemblyBuilder.Location);
}
Assert.Empty(assembly.Location);
Assert.NotNull(internalAssemblyBuilder);
Assert.Empty(internalAssemblyBuilder.Location);
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public static void ThrowsWhenDynamicCodeNotSupported()
Expand Down
96 changes: 96 additions & 0 deletions ...raries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveResourceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
using System.Globalization;
using System.IO;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;
using System.Reflection.PortableExecutable;
using System.Resources;
using Microsoft.DotNet.RemoteExecutor;
using Xunit;

namespace System.Reflection.Emit.Tests
{
[ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
public class AssemblySaveResourceTests
{
[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[InlineData(new byte[] { 1 }, "01")]
[InlineData(new byte[] { 1, 2 }, "01-02")] // Verify blob alignment padding by adding a byte.
public void ManagedResources(byte[] byteValue, string byteValueExpected)
{
PersistedAssemblyBuilder ab = new PersistedAssemblyBuilder(new AssemblyName("MyAssemblyWithResource"), typeof(object).Assembly);
ab.DefineDynamicModule("MyModule");
MetadataBuilder metadata = ab.GenerateMetadata(out BlobBuilder ilStream, out _);

using MemoryStream memoryStream = new MemoryStream();
ResourceWriter myResourceWriter = new ResourceWriter(memoryStream);
myResourceWriter.AddResource("StringResource", "Value");
myResourceWriter.AddResource("ByteResource", byteValue);
myResourceWriter.Close();

byte[] data = memoryStream.ToArray();
BlobBuilder resourceBlob = new BlobBuilder();
resourceBlob.WriteInt32(data.Length);
resourceBlob.WriteBytes(data);
int resourceBlobSize = resourceBlob.Count;

metadata.AddManifestResource(
ManifestResourceAttributes.Public,
metadata.GetOrAddString("MyResource.resources"),
implementation: default,
offset: 0);

ManagedPEBuilder peBuilder = new ManagedPEBuilder(
header: PEHeaderBuilder.CreateLibraryHeader(),
metadataRootBuilder: new MetadataRootBuilder(metadata),
ilStream: ilStream,
managedResources: resourceBlob);

BlobBuilder blob = new BlobBuilder();
peBuilder.Serialize(blob);

// Ensure the the resource blob wasn't modified by Serialize() due to alignment padding.
// Serialize() pads after the blob to align at ManagedTextSection.ManagedResourcesDataAlignment bytes.
Assert.Equal(resourceBlobSize, resourceBlob.Count);

// Create a temporary assembly.
string tempFilePath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName() + ".dll");
steveharter marked this conversation as resolved.
Show resolved Hide resolved
using TempFile file = new TempFile(tempFilePath, blob.ToArray());

// To verify the resources work with runtime APIs, load the assembly into the process instead of
// the normal testing approach of using MetadataLoadContext.
using RemoteInvokeHandle remoteHandle = RemoteExecutor.Invoke(static (tempFilePath, byteValue, byteValueExpected) =>
{
Assembly readAssembly = Assembly.LoadFile(tempFilePath);

// Use ResourceReader to read the resources.
using Stream readStream = readAssembly.GetManifestResourceStream("MyResource.resources")!;
using ResourceReader reader = new(readStream);
Verify(reader.GetEnumerator());

// Use ResourceManager to read the resources.
ResourceManager rm = new ResourceManager("MyResource", readAssembly);
ResourceSet resourceSet = rm.GetResourceSet(CultureInfo.InvariantCulture, createIfNotExists: true, tryParents: false);
Verify(resourceSet.GetEnumerator());

void Verify(IDictionaryEnumerator resources)
{
Assert.True(resources.MoveNext());
DictionaryEntry resource = (DictionaryEntry)resources.Current;
Assert.Equal("ByteResource", resource.Key);
Assert.Equal(byteValueExpected, byteValue);

Assert.True(resources.MoveNext());
resource = (DictionaryEntry)resources.Current;
Assert.Equal("StringResource", resource.Key);
Assert.Equal("Value", resource.Value);

Assert.False(resources.MoveNext());
}
}, tempFilePath, BitConverter.ToString(byteValue), byteValueExpected);
}
}
}
1 change: 1 addition & 0 deletions src/libraries/System.Reflection.Emit/tests/System.Reflection.Emit.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
<Compile Include="PersistedAssemblyBuilder\AssemblySaveTypeBuilderAPIsTests.cs" />
<Compile Include="PersistedAssemblyBuilder\AssemblySaveModuleBuilderTests.cs" />
<Compile Include="PersistedAssemblyBuilder\AssemblySavePropertyBuilderTests.cs" />
<Compile Include="PersistedAssemblyBuilder\AssemblySaveResourceTests.cs" />
<Compile Include="PersistedAssemblyBuilder\AssemblySaveTools.cs" />
<Compile Include="PersistedAssemblyBuilder\AssemblySaveTypeBuilderTests.cs" />
<Compile Include="PortablePdb\ILGeneratorScopesAndSequencePointsTests.cs" />
Expand Down
2 changes: 1 addition & 1 deletion ...raries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataSizes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace System.Reflection.Metadata.Ecma335
/// </summary>
public sealed class MetadataSizes
{
private const int StreamAlignment = 4;
internal const int StreamAlignment = 4;

// Call the length of the string (including the terminator) m (we require m <= 255);
internal const int MaxMetadataVersionByteCount = 0xff - 1;
Expand Down
21 changes: 12 additions & 9 deletions ...System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Diagnostics;
using System.Reflection.Internal;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;

namespace System.Reflection.PortableExecutable
{
Expand Down Expand Up @@ -40,8 +41,8 @@ internal sealed class ManagedTextSection
public int MetadataSize { get; }

/// <summary>
/// The size of managed resource data stream.
/// Aligned to <see cref="ManagedResourcesDataAlignment"/>.
/// The size of managed resource data stream (unaligned).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmat Could you please take a look at the changes in this file? How are the alignment requirements expected to be handled by ManagedPEBuilder?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @tmat

Copy link
Member

@tmat tmat Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember what the expectations were. It's been a while.

To me it looks like we should have checked the alignments in ManagedPEBuilder constructor and throw if they are off. Can we add that check? It would break someone who emits unaligned data, but if they do the emitted assembly would be bad anyways.

/// When written, will be aligned to <see cref="ManagedResourcesDataAlignment"/>.
/// </summary>
public int ResourceDataSize { get; }

Expand Down Expand Up @@ -147,14 +148,16 @@ public int CalculateOffsetToMappedFieldDataStream()

internal int ComputeOffsetToDebugDirectory()
{
Debug.Assert(MetadataSize % 4 == 0);
Debug.Assert(ResourceDataSize % 4 == 0);
Debug.Assert(MetadataSize % MetadataSizes.StreamAlignment == 0);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure if this is the intent (4 byte alignment across the various sections); I think we could remove this Assert since they should all be aligned anyway


return
int offset =
ComputeOffsetToMetadata() +
MetadataSize +
ResourceDataSize +
BitArithmetic.Align(ResourceDataSize, ManagedResourcesDataAlignment) +
StrongNameSignatureSize;

Debug.Assert(offset % MetadataSizes.StreamAlignment == 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StrongNameSignatureSize is an arbitrary number passed in by the user code. I do not think that this assert holds - it should be possible to write a simple test that hits it.

return offset;
}

private int ComputeOffsetToImportTable()
Expand Down Expand Up @@ -254,7 +257,6 @@ public void Serialize(
Debug.Assert(ilBuilder.Count == ILStreamSize);
Debug.Assert((mappedFieldDataBuilderOpt?.Count ?? 0) == MappedFieldDataSize);
Debug.Assert((resourceBuilderOpt?.Count ?? 0) == ResourceDataSize);
Debug.Assert((resourceBuilderOpt?.Count ?? 0) % 4 == 0);

// TODO: avoid recalculation
int importTableRva = GetImportTableDirectoryEntry(relativeVirtualAddess).RelativeVirtualAddress;
Expand All @@ -278,6 +280,7 @@ public void Serialize(
if (resourceBuilderOpt != null)
{
builder.LinkSuffix(resourceBuilderOpt);
builder.WriteBytes(0, BitArithmetic.Align(resourceBuilderOpt.Count, ManagedTextSection.ManagedResourcesDataAlignment) - resourceBuilderOpt.Count);
}

// strong name signature:
Expand Down Expand Up @@ -387,7 +390,7 @@ private void WriteCorHeader(BlobBuilder builder, int textSectionRva, int entryPo

int metadataRva = textSectionRva + ComputeOffsetToMetadata();
int resourcesRva = metadataRva + MetadataSize;
int signatureRva = resourcesRva + ResourceDataSize;
int signatureRva = resourcesRva + BitArithmetic.Align(ResourceDataSize, ManagedResourcesDataAlignment);

int start = builder.Count;

Expand All @@ -410,7 +413,7 @@ private void WriteCorHeader(BlobBuilder builder, int textSectionRva, int entryPo

// ResourcesDirectory:
builder.WriteUInt32((uint)(ResourceDataSize == 0 ? 0 : resourcesRva)); // 28
builder.WriteUInt32((uint)ResourceDataSize);
builder.WriteUInt32((uint)BitArithmetic.Align(ResourceDataSize, ManagedResourcesDataAlignment));

// StrongNameSignatureDirectory:
builder.WriteUInt32((uint)(StrongNameSignatureSize == 0 ? 0 : signatureRva)); // 36
Expand Down
Loading