Skip to content

Commit 26232de

Browse files
Washi1337Washi1337
authored
Merge pull request #625 from Washi1337/feature/allow-unsorted-nested-typedefs
Relax Validation on Final Nested Type Ordering
2 parents b6b13e5 + cfe83ad commit 26232de

File tree

7 files changed

+188
-43
lines changed

7 files changed

+188
-43
lines changed

‎src/AsmResolver.DotNet/Builder/Discovery/MemberDiscoverer.cs

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public sealed class MemberDiscoverer
3131
private readonly TypeReference _eventHandlerTypeRef;
3232
private readonly TypeSignature _eventHandlerTypeSig;
3333

34-
private readonly Dictionary<TableIndex, IList<uint>> _freeRids = new()
34+
private readonly Dictionary<TableIndex, List<uint>> _freeRids = new()
3535
{
3636
[TableIndex.TypeDef] = new List<uint>(),
3737
[TableIndex.Field] = new List<uint>(),
@@ -41,7 +41,7 @@ public sealed class MemberDiscoverer
4141
[TableIndex.Event] = new List<uint>(),
4242
};
4343

44-
private readonly Dictionary<TableIndex, IList<IMetadataMember>> _floatingMembers = new()
44+
private readonly Dictionary<TableIndex, List<IMetadataMember>> _floatingMembers = new()
4545
{
4646
[TableIndex.TypeDef] = new List<IMetadataMember>(),
4747
[TableIndex.Field] = new List<IMetadataMember>(),
@@ -253,10 +253,7 @@ private void InsertOrAppendIfNew<TMember>(TMember member, bool queueIfNoSlotsAva
253253

254254
// Check if the slot is available.
255255
if (memberList[(int) member.MetadataToken.Rid - 1] is { } slot)
256-
{
257-
throw new ArgumentException(
258-
$"{slot.SafeToString()} and {member.SafeToString()} are assigned the same RID {member.MetadataToken.Rid}.");
259-
}
256+
throw new MetadataTokenConflictException(slot, member, member.MetadataToken.Rid);
260257

261258
memberList[(int) member.MetadataToken.Rid - 1] = member;
262259
freeRids.Remove(member.MetadataToken.Rid);

‎src/AsmResolver.DotNet/Builder/DotNetDirectoryBuffer.MemberTree.cs

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,6 @@ public MetadataToken DefineManifestResource(ManifestResource resource, bool dedu
141141
public void DefineTypes(IEnumerable<TypeDefinition> types)
142142
{
143143
var typeDefTable = Metadata.TablesStream.GetTable<TypeDefinitionRow>(TableIndex.TypeDef);
144-
var nestedClassTable = Metadata.TablesStream.GetSortedTable<TypeDefinition, NestedClassRow>(TableIndex.NestedClass);
145144

146145
if (types is ICollection<TypeDefinition> collection)
147146
typeDefTable.EnsureCapacity(typeDefTable.Count + collection.Count);
@@ -161,26 +160,6 @@ public void DefineTypes(IEnumerable<TypeDefinition> types)
161160

162161
var token = typeDefTable.Add(row);
163162
_tokenMapping.Register(type, token);
164-
165-
if (type.IsNested)
166-
{
167-
// As per the ECMA-335; nested types should always follow their enclosing types in the TypeDef table.
168-
// Proper type def collections that are passed onto this function therefore should have been added
169-
// already to the buffer. If not, we have an invalid ordering of types.
170-
171-
var enclosingTypeToken = GetTypeDefinitionToken(type.DeclaringType, type);
172-
if (enclosingTypeToken.Rid == 0)
173-
{
174-
ErrorListener.MetadataBuilder(
175-
$"Nested type {type.SafeToString()} is added before its enclosing class {type.DeclaringType.SafeToString()}.");
176-
}
177-
178-
var nestedClassRow = new NestedClassRow(
179-
token.Rid,
180-
enclosingTypeToken.Rid);
181-
182-
nestedClassTable.Add(type, nestedClassRow);
183-
}
184163
}
185164
}
186165

@@ -359,6 +338,7 @@ public void FinalizeTypes()
359338
AddCustomAttributes(typeToken, type);
360339
AddSecurityDeclarations(typeToken, type);
361340
DefineInterfaces(typeToken, type.Interfaces);
341+
AddNestedClassRow(type, rid);
362342
AddMethodImplementations(type, typeToken, type.MethodImplementations);
363343
DefineGenericParameters(typeToken, type);
364344
AddClassLayout(typeToken, type.ClassLayout);
@@ -571,6 +551,30 @@ private void FinalizeEventsInType(TypeDefinition type, uint typeRid, ref uint ev
571551
eventList += (uint) type.Events.Count;
572552
}
573553

554+
private void AddNestedClassRow(TypeDefinition type, uint rid)
555+
{
556+
if (!type.IsNested)
557+
return;
558+
559+
var table = Metadata.TablesStream.GetSortedTable<TypeDefinition, NestedClassRow>(TableIndex.NestedClass);
560+
561+
// As per the ECMA-335 II.2; nested types should always follow their enclosing types in the TypeDef table.
562+
// However, from empirical testing, the runtime actually does not seem to mind unsorted TypeDefs.
563+
// Hence, we still add the nested class even if the RID ordering was technically not correct.
564+
// See also: https://github.com/Washi1337/AsmResolver/issues/545
565+
566+
var enclosingTypeToken = GetTypeDefinitionToken(type.DeclaringType);
567+
if (enclosingTypeToken.Rid == 0 || enclosingTypeToken.Rid > rid)
568+
{
569+
ErrorListener.MetadataBuilder(
570+
$"Nested type {type.SafeToString()} is added before its enclosing class {type.DeclaringType.SafeToString()}."
571+
);
572+
}
573+
574+
var row = new NestedClassRow(rid, enclosingTypeToken.Rid);
575+
table.Add(type, row);
576+
}
577+
574578
private void AddMethodImplementations(TypeDefinition type, MetadataToken typeToken, IList<MethodImplementation> implementations)
575579
{
576580
var table = Metadata.TablesStream.GetSortedTable<MethodImplementation, MethodImplementationRow>(TableIndex.MethodImpl);

‎src/AsmResolver.DotNet/Builder/MetadataTokenConflictException.cs

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
using AsmResolver.PE.DotNet.Metadata.Tables;
2+
3+
namespace AsmResolver.DotNet.Builder;
4+
5+
/// <summary>
6+
/// Represents the exception that occurs when two metadata members are assigned the same metadata token.
7+
/// </summary>
8+
public class MetadataTokenConflictException : MetadataBuilderException
9+
{
10+
/// <summary>
11+
/// Creates a new instance of the <see cref="MetadataTokenConflictException"/> class.
12+
/// </summary>
13+
/// <param name="member1">The first conflicting member.</param>
14+
/// <param name="member2">The second conflicting member.</param>
15+
/// <param name="token">The metadata token they are both assigned to.</param>
16+
public MetadataTokenConflictException(IMetadataMember member1, IMetadataMember member2, MetadataToken token)
17+
: base($"{member1.SafeToString()} and {member2.SafeToString()} are assigned the same RID {token.Rid}.")
18+
{
19+
Member1 = member1;
20+
Member2 = member2;
21+
Token = token;
22+
}
23+
24+
/// <summary>
25+
/// Gets the first conflicting member.
26+
/// </summary>
27+
public IMetadataMember Member1
28+
{
29+
get;
30+
}
31+
32+
/// <summary>
33+
/// Gets the second conflicting member.
34+
/// </summary>
35+
public IMetadataMember Member2
36+
{
37+
get;
38+
}
39+
40+
/// <summary>
41+
/// Gets the metadata token the two members were assigned.
42+
/// </summary>
43+
public MetadataToken Token
44+
{
45+
get;
46+
}
47+
}

‎src/AsmResolver.DotNet/ModuleDefinition.cs

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,7 @@ public ModuleDefinition(Utf8String? name)
307307
RuntimeContext = new RuntimeContext(OriginalTargetRuntime);
308308
MetadataResolver = new DefaultMetadataResolver(RuntimeContext.AssemblyResolver);
309309

310-
TopLevelTypes.Add(new TypeDefinition(null, TypeDefinition.ModuleTypeName, 0));
310+
CreateAndInsertModuleType();
311311
}
312312

313313
/// <summary>
@@ -328,7 +328,7 @@ public ModuleDefinition(string? name, AssemblyReference? corLib)
328328
RuntimeContext = new RuntimeContext(OriginalTargetRuntime);
329329
MetadataResolver = new DefaultMetadataResolver(RuntimeContext.AssemblyResolver);
330330

331-
TopLevelTypes.Add(new TypeDefinition(null, TypeDefinition.ModuleTypeName, 0));
331+
CreateAndInsertModuleType();
332332
}
333333

334334
/// <summary>
@@ -466,7 +466,7 @@ public DotNetDirectoryFlags Attributes
466466
}
467467

468468
/// <summary>
469-
/// Gets an object responsible for assigning new <see cref="MetadataToken"/> to members
469+
/// Gets an object responsible for assigning new metadata tokens to metadata members.
470470
/// </summary>
471471
public TokenAllocator TokenAllocator
472472
{
@@ -1044,12 +1044,27 @@ public IEnumerable<TypeDefinition> GetAllTypes()
10441044
/// <returns>The module type.</returns>
10451045
public TypeDefinition GetOrCreateModuleType()
10461046
{
1047-
var moduleType = GetModuleType();
1047+
return GetModuleType() ?? CreateAndInsertModuleType();
1048+
}
1049+
1050+
private TypeDefinition CreateAndInsertModuleType()
1051+
{
1052+
var moduleType = new TypeDefinition(null, TypeDefinition.ModuleTypeName, 0);
1053+
TopLevelTypes.Insert(0, moduleType);
10481054

1049-
if (moduleType is null)
1055+
// Check if we can assign RID 1 to the type using the token allocator. This avoids users accidentally
1056+
// overriding the module type later by using the token allocator.
1057+
var token = TokenAllocator.GetNextAvailableToken(TableIndex.TypeDef);
1058+
if (token.Rid == 1)
10501059
{
1051-
moduleType = new TypeDefinition(null, TypeDefinition.ModuleTypeName, 0);
1052-
TopLevelTypes.Insert(0, moduleType);
1060+
TokenAllocator.AssignNextAvailableToken(moduleType);
1061+
}
1062+
else
1063+
{
1064+
// If RID 1 is not possible, we still want to assign RID 1 to the type explicitly, to have metadata
1065+
// builders configured to preserve RIDs deliberately crash on conflicting RIDs.
1066+
1067+
moduleType.MetadataToken = new MetadataToken(TableIndex.TypeDef, 1);
10531068
}
10541069

10551070
return moduleType;

‎test/AsmResolver.DotNet.Tests/Builder/TokenPreservation/TokenPreservationTestBase.cs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,15 +32,16 @@ protected static List<TMember> GetMembers<TMember>(ModuleDefinition module, Tabl
3232

3333
protected static ModuleDefinition RebuildAndReloadModule(ModuleDefinition module, MetadataBuilderFlags builderFlags)
3434
{
35-
var builder = new ManagedPEImageBuilder(builderFlags);
35+
return RebuildAndReloadModule(module, builderFlags, new DiagnosticBag());
36+
}
37+
38+
protected static ModuleDefinition RebuildAndReloadModule(ModuleDefinition module, MetadataBuilderFlags builderFlags, DiagnosticBag bag)
39+
{
40+
var builder = new ManagedPEImageBuilder(new DotNetDirectoryFactory(builderFlags), bag);
3641

3742
var result = builder.CreateImage(module);
38-
if (result.HasFailed)
39-
{
40-
if (result.ErrorListener is DiagnosticBag diagnosticBag)
41-
throw new AggregateException(diagnosticBag.Exceptions);
42-
throw new Exception("Image creation failed.");
43-
}
43+
if (bag.IsFatal || result.HasFailed)
44+
throw new AggregateException(bag.Exceptions);
4445

4546
var newImage = result.ConstructedImage;
4647

‎test/AsmResolver.DotNet.Tests/Builder/TokenPreservation/TypeDefTokenPreservationTest.cs

Lines changed: 75 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System;
12
using System.Linq;
23
using AsmResolver.DotNet.Builder;
34
using AsmResolver.PE.DotNet.Metadata.Tables;
@@ -67,12 +68,84 @@ public void PreserveTypeDefsRemoveTypeShouldStuffTypeDefSlots()
6768
var newModule = RebuildAndReloadModule(module, MetadataBuilderFlags.PreserveTypeDefinitionIndices);
6869
var newTypeDefs = GetMembers<TypeDefinition>(newModule, TableIndex.TypeDef);
6970

70-
for (int i = 0; i < originalTypeDefs.Count; i++)
71+
Assert.All(Enumerable.Range(0, originalTypeDefs.Count), i =>
7172
{
7273
if (i != indexToRemove)
7374
Assert.Equal(originalTypeDefs[i], newTypeDefs[i], Comparer);
74-
}
75+
});
7576
}
7677

78+
[Fact]
79+
public void PreserveTypeDefsAfterInsertingNewModuleTypeShouldThrow()
80+
{
81+
// Prepare.
82+
var module = new ModuleDefinition("SomeModule", KnownCorLibs.SystemRuntime_v8_0_0_0);
83+
module = RebuildAndReloadModule(module, MetadataBuilderFlags.None);
84+
85+
// Insert new module type, but keep original module type around.
86+
module.GetOrCreateModuleType().Name = "<NotModule>";
87+
var newModuleType = module.GetOrCreateModuleType();
88+
89+
// Rebuild with type preservation should conflict.
90+
var exception = Assert.Throws<AggregateException>(() => RebuildAndReloadModule(module, MetadataBuilderFlags.PreserveTypeDefinitionIndices));
91+
Assert.Contains(exception.InnerExceptions, ex =>
92+
{
93+
return ex is MetadataTokenConflictException conflict
94+
&& (conflict.Member1 == newModuleType || conflict.Member2 == newModuleType)
95+
&& conflict.Token.Rid == 1;
96+
});
97+
}
98+
99+
[Fact]
100+
public void PreserveTypeDefsAfterInsertingNewModuleTypeAndRemovingOldShouldNotThrow()
101+
{
102+
// Prepare.
103+
var module = new ModuleDefinition("SomeModule", KnownCorLibs.SystemRuntime_v8_0_0_0);
104+
var moduleType = module.GetOrCreateModuleType();
105+
moduleType.Fields.Add(new FieldDefinition("SomeField", FieldAttributes.Static, module.CorLibTypeFactory.Int32));
106+
module = RebuildAndReloadModule(module, MetadataBuilderFlags.None);
107+
108+
// Insert new module type, and remove original module type.
109+
module.TopLevelTypes.Remove(module.GetModuleType());
110+
moduleType = module.GetOrCreateModuleType();
111+
moduleType.Fields.Add(new FieldDefinition("OtherField", FieldAttributes.Static, module.CorLibTypeFactory.Int16));
112+
113+
// Rebuild should not conflict.
114+
var newModule = RebuildAndReloadModule(module, MetadataBuilderFlags.PreserveTypeDefinitionIndices);
115+
116+
// Assert that the module type was replaced with the new one.
117+
var newModuleType = newModule.GetModuleType();
118+
Assert.NotNull(newModuleType);
119+
var fields = Assert.Single(newModuleType.Fields);
120+
Assert.Equal("OtherField", fields.Name);
121+
}
122+
123+
[Fact]
124+
public void PreserveInvalidNestedTypeOrderingShouldNonFatallyThrow()
125+
{
126+
// Prepare.
127+
var module = new ModuleDefinition("SomeModule", KnownCorLibs.SystemRuntime_v8_0_0_0);
128+
module.TopLevelTypes.Add(new TypeDefinition(null, "Type1", TypeAttributes.Public));
129+
module.TopLevelTypes.Add(new TypeDefinition(null, "Type2", TypeAttributes.Public));
130+
module = RebuildAndReloadModule(module, MetadataBuilderFlags.None);
131+
132+
// Move type1 into type2.
133+
var type1 = module.TopLevelTypes[1];
134+
var type2 = module.TopLevelTypes[2];
135+
module.TopLevelTypes.Remove(type1);
136+
type2.NestedTypes.Add(type1);
137+
type1.IsNestedPublic = true;
138+
139+
// Rebuild with type token preservation should not fatally throw.
140+
var bag = new DiagnosticBag();
141+
var newModule = RebuildAndReloadModule(module, MetadataBuilderFlags.PreserveTypeDefinitionIndices, bag);
142+
143+
// Assert exception was recorded.
144+
Assert.Contains(bag.Exceptions, ex => ex is MetadataBuilderException);
145+
146+
// Assert that nested type relation is preserved.
147+
var newType2 = newModule.TopLevelTypes.First(t => t.Name == type2.Name);
148+
Assert.Contains(newType2.NestedTypes, t => t.Name == type1.Name);
149+
}
77150
}
78151
}

‎test/AsmResolver.DotNet.Tests/ModuleDefinitionTest.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,14 @@ public void GetModuleTypeLookalikeNet6()
590590
Assert.Same(type, module.GetModuleType());
591591
}
592592

593+
[Fact]
594+
public void CreateModuleTypeShouldHaveFirstRid()
595+
{
596+
var module = new ModuleDefinition("SomeModule");
597+
var type = module.GetOrCreateModuleType();
598+
Assert.Equal(1u, type.MetadataToken.Rid);
599+
}
600+
593601
[Theory]
594602
[InlineData(false, false, false)]
595603
[InlineData(false, true, false)]

0 commit comments

Comments
 (0)