dotnet / linker

388 stars 126 forks source link

Analysis hole when reflecting over cctors #3104

Closed sbomer closed 1 year ago

sbomer commented 1 year ago
class CCtorTest {
    [RUC("RUC")]
    static void RUC() {
        Console.WriteLine("Hey there, you called a RUC method!");
    }

    [RUC("RUC")]
    class Cctor {
        static Cctor() => RUC();
    }

    public static void Test() => UseCctor(typeof(Cctor));

    static void UseCctor([DAM(DAMT.NonPublicConstructors)] Type t) {
        var cctor = t.GetConstructor(BindingFlags.Static | BindingFlags.NonPublic, new Type[0]);
        cctor.Invoke(null, null);
    }
}

This produces no warnings, but allows the use of reflection to execute RUC code. It looks like this is because we don't treat cctors in a RUC type as requiring unreferenced code here: https://github.com/dotnet/linker/blame/main/src/linker/Linker/Annotations.cs#L657-L663

vitek-karas commented 1 year ago

Good catch - I think the problem is a bit more subtle. We don't want to treat .cctor as having RUC when they're called directly from IL (is that's even doable to be honest) - the reason is that it's unreliable - we will never detect all call sites like that, so we don't do it at all. Instead we rely on the fact that access any static member or .ctor will trigger the cctor anyway in the runtime, so we guard it there.

On the other hand, reflection is an hole here - since it allows explicit access AND invocation - so we need to validate it there, but only for reflection, not for other types of accesses.

sbomer commented 1 year ago

Agreed - I was thinking that it's fine to consider it as requiring unreferenced code, and this shouldn't produce any extra warnings from IL exactly because there will be no callsites. But maybe it will be more subtle when I go to implement a fix.