Incorrect metadata token fixing in `GetTypeSpecification` #953

New issue

Open

opened 2024-06-30 09:19:51 -05:00 by Wartori54 · 2 comments

Wartori54 commented

2024-06-30 09:19:51 -05:00

(Migrated from github.com)

With the following target dll to patch:

public class Entity {}

public static class UserIO {
    public static void Dummy<T>() where T : class {
        
    }
    
    public static T Save<T>() where T : class {
        return (T)null;
    }

    public static void RandomMethod<T>() where T : Entity {
        
    }
    
    public static void RandomMethod1<T>() where T : Entity {
       
    }
    
    public static void RandomMethod3<T>() where T : Entity {
        
    }
}

And the following cecil procedure:

TypeDefinition userio = module.Types.First(t => t.Name == "UserIO");
MethodDefinition saveT = userio.Methods.First(t => t.Name == "Save");

// Verify the metadata token
Console.WriteLine("generic param 0 token type: " + saveT.GenericParameters[0].MetadataToken.TokenType);

// Replace it with an identical one
saveT.GenericParameters[0] = new GenericParameter(saveT.GenericParameters[0].Name, saveT.GenericParameters[0].Owner) {
    Attributes = saveT.GenericParameters[0].Attributes
};

// Read the body
Read(saveT.Body);

// Check again
Console.WriteLine("generic param 0 token type: " + saveT.GenericParameters[0].MetadataToken.TokenType);

// Check the constraints
Console.WriteLine(saveT.GenericParameters[0].Constraints.Count);

public static void Read(object o) {}

The output for the above snippet is the following (compiled with .net sdk 8.0.106):

generic param 0 token type: GenericParam
generic param 0 token type: TypeSpec
1

The issue here is the appearance of any IL instructions that may have its operand serialized as a TypeSpec. In this specific case the offender is initobj !!0 inside the Save method (and for #931 it was a box !!0).
When reading the operand for that instruction GetTypeSpecification will be called, which, if the token for the generic parameter it references was uninitialized or cleared (which is what replacing the generic parameter effectively does), it will try to fix it (source):

var type = reader.ReadTypeSignature ();
// Here type is an instance of `GenericParameter`
if (type.token.RID == 0)
	type.token = new MetadataToken (TokenType.TypeSpec, rid);

It can be seen that when the condition is full filled the metadata token for that generic parameter will point to the TypeSpec table.
The issue with it is when the constraints for that same GenericParameter are read, it is assumed that the generic parameter RID will point to the GenericParam table, which is not correct and as such it will point to an arbitrary generic constraint list in the inverse mappings generated by cecil (the dictionary named GenericConstraints) (source):

public bool TryGetGenericConstraintMapping (GenericParameter generic_parameter, out Collection<Row<uint, MetadataToken>> mapping)
{
	// The RID is used blindly with the inverse mappings generated from the `GenericParamConstraint` table, which always points to the `GenericParam` table, and not to the `TypeSpec` table
	return GenericConstraints.TryGetValue (generic_parameter.token.RID, out mapping);
}

Possible fixes for the issue may be:

do not fix the metadata token, because the new GenericParameter does not have binary representation in the read file, and as such no token can be assigned to it until it is serialized.
verify that the RID points to the correct table when reading mappings.

Now that the issue has been exposed in full detail I want to also explain why the commit c4cfe16 had an impact into this issue manifesting more often.

That commit eliminates the removal of mappings when they have been read, since they should be cached afterwards. But a side effect that it had was making it harder for a GenericParameter with a wrong metadata token linking itself into some generic constraint list.
This is why using Cecil 0.11.5 with MonoMod very easily triggers this issue.

Follow up issue from #931, since the problem that it exposed is not restricted to Cecil 0.11.5, and has been there for a long time. I have made a small program where this issue can be demonstrated, its obvious side effect is the linking of the generic parameter with a wrong constraint list, which #931 described. With the following target dll to patch: ```cs public class Entity {} public static class UserIO { public static void Dummy<T>() where T : class { } public static T Save<T>() where T : class { return (T)null; } public static void RandomMethod<T>() where T : Entity { } public static void RandomMethod1<T>() where T : Entity { } public static void RandomMethod3<T>() where T : Entity { } } ``` And the following cecil procedure: ```cs TypeDefinition userio = module.Types.First(t => t.Name == "UserIO"); MethodDefinition saveT = userio.Methods.First(t => t.Name == "Save"); // Verify the metadata token Console.WriteLine("generic param 0 token type: " + saveT.GenericParameters[0].MetadataToken.TokenType); // Replace it with an identical one saveT.GenericParameters[0] = new GenericParameter(saveT.GenericParameters[0].Name, saveT.GenericParameters[0].Owner) { Attributes = saveT.GenericParameters[0].Attributes }; // Read the body Read(saveT.Body); // Check again Console.WriteLine("generic param 0 token type: " + saveT.GenericParameters[0].MetadataToken.TokenType); // Check the constraints Console.WriteLine(saveT.GenericParameters[0].Constraints.Count); public static void Read(object o) {} ``` The output for the above snippet is the following (compiled with .net sdk 8.0.106): ``` generic param 0 token type: GenericParam generic param 0 token type: TypeSpec 1 ``` The issue here is the appearance of any IL instructions that may have its operand serialized as a `TypeSpec`. In this specific case the offender is `initobj !!0` inside the `Save` method (and for #931 it was a `box !!0`). When reading the operand for that instruction `GetTypeSpecification` will be called, which, if the token for the generic parameter it references was uninitialized or cleared (which is what replacing the generic parameter effectively does), it will try to fix it ([source](<https://github.com/jbevain/cecil/blob/8e1ae7b4ea67ccc38cb8db3ded6802643109ffd7/Mono.Cecil/AssemblyReader.cs#L1204>)): ```cs var type = reader.ReadTypeSignature (); // Here type is an instance of `GenericParameter` if (type.token.RID == 0) type.token = new MetadataToken (TokenType.TypeSpec, rid); ``` It can be seen that when the condition is full filled the metadata token for that generic parameter will point to the `TypeSpec` table. The issue with it is when the constraints for that same `GenericParameter` are read, it is assumed that the generic parameter RID will point to the `GenericParam` table, which is not correct and as such it will point to an arbitrary generic constraint list in the inverse mappings generated by cecil (the dictionary named `GenericConstraints`) ([source](https://github.com/jbevain/cecil/blob/8e1ae7b4ea67ccc38cb8db3ded6802643109ffd7/Mono.Cecil/MetadataSystem.cs#L301-L304)): ```cs public bool TryGetGenericConstraintMapping (GenericParameter generic_parameter, out Collection<Row<uint, MetadataToken>> mapping) { // The RID is used blindly with the inverse mappings generated from the `GenericParamConstraint` table, which always points to the `GenericParam` table, and not to the `TypeSpec` table return GenericConstraints.TryGetValue (generic_parameter.token.RID, out mapping); } ``` Possible fixes for the issue may be: - do not fix the metadata token, because the new `GenericParameter` does not have binary representation in the read file, and as such no token can be assigned to it until it is serialized. - verify that the RID points to the correct table when reading mappings. --- Now that the issue has been exposed in full detail I want to also explain why the commit [`c4cfe16`](https://github.com/jbevain/cecil/commit/c4cfe168450ad7c14d881c1388e981564a06eab7) had an impact into this issue manifesting more often. That commit eliminates the removal of mappings when they have been read, since they should be cached afterwards. But a side effect that it had was making it harder for a `GenericParameter` with a wrong metadata token linking itself into some generic constraint list. This is why using Cecil 0.11.5 with MonoMod very easily triggers this issue.

Wartori54 commented

2024-07-07 16:24:01 -05:00

(Migrated from github.com)

It is important to point out here that the default reader settings were used, thus ReadingMode.Deferred was used in all my tests.
Obviously using ReadingMode.Immediate fixes all of these issues and is currently the "workaround" we are using.

But this brings up the question, is patching assemblies in that default mode fully supported (either currently or planned in the future) by Cecil?

If it is, this is an issue that should be addressed.
If it is not, then this issue can be closed as-is since its a no-issue.

It is important to point out here that the default reader settings were used, thus `ReadingMode.Deferred` was used in all my tests. Obviously using `ReadingMode.Immediate` fixes all of these issues and is currently the "workaround" we are using. But this brings up the question, is patching assemblies in that default mode fully supported (either currently or planned in the future) by Cecil? - If it is, this is an issue that should be addressed. - If it is not, then this issue can be closed as-is since its a no-issue.

josesimoes commented

2025-12-10 13:58:09 -06:00

(Migrated from github.com)