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

Merged
merged 7 commits into from
Feb 15, 2025
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 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
cb2efb3
Remove Assert; test using LoadContext instead of RemoteExecutor
steveharter Jan 15, 2025
e8cbf27
Remove Asserts and padding
steveharter Feb 14, 2025
306bfb8
Remove unnecessary using statement
steveharter Feb 14, 2025
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");
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.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are basically two choices: throw if not aligned or align it for the user.

This PR takes the path of aligning it for the user like we do in other areas:

If we don't align it for the user, the user would need an extra LOC to do the alignment for the somewhat common case of using resources, and that extra LOC that would be needed is not obvious to the caller.

Copy link
Member Author

Choose a reason for hiding this comment

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

but if they do the emitted assembly would be bad anyways.

I don't believe the lack of alignment breaks anything, unless there is some platform-specific alignment limitations that I'm not aware of -- e.g. the test added here worked before the alignment was added.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow. If the alignment is not needed then why are we adding it?

Copy link
Member Author

Choose a reason for hiding this comment

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

There was an Assert that was failing so we either need to remove it or add the alignment \ padding. Since there was a ManagedResourcesDataAlignment public const I decided to use that (it wasn't used before). Finally, the assert was checking if alignment to 4 while the public const is 8.

I do not know why the const was added, or why the alignment and checks are there in part. Git history shows this existed 8+ years ago as part of large commits. Likely the alignment was added for efficiency when reading the subsequent data. The code may have assumed the caller would align the data, but if so then the code should have thrown and not just asserted.

/// 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);

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

Debug.Assert(offset % MetadataSizes.StreamAlignment == 0);
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