Incorrect metadata token fixing in `GetTypeSpecification`

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:

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.

jbevain / cecil

Incorrect metadata token fixing in `GetTypeSpecification` #953