Biohazrd icon indicating copy to clipboard operation
Biohazrd copied to clipboard

Create an analyzer to error when an unmanaged struct is dereferenced

Open PathogenDavid opened this issue 3 years ago • 1 comments

The analyzer should also error if a field/variable of a non-pointer type is created for these structs.

Doubleplussame for TranslatedUndefinedRecord since they couldn't even be dereferenced in C++.

PathogenDavid avatar Sep 10 '20 01:09 PathogenDavid

Interesting case that came up with for this:

https://github.com/ocornut/imgui/blob/839ecce57154a33ede985eeab746828f8102c8d9/imgui_internal.h#L1293

ImDockRequest is not defined in imgui_internal.h, It's defined in imgui.cpp:

https://github.com/ocornut/imgui/blob/839ecce57154a33ede985eeab746828f8102c8d9/imgui.cpp#L12090-L12111

However, ImVector<T> does not depend on T for its layout, only for its API:

https://github.com/ocornut/imgui/blob/839ecce57154a33ede985eeab746828f8102c8d9/imgui.h#L1692-L1696

In C++ this it totally fine. ImVector<ImDockRequest> will be totally usable with the exception of anything which relies on the layout of T. (Which is most of the functions, but still.) As seen here, the errors only occur when calling the affected methods:

template<typename T>
struct TestTemplate
{
    T* data;

    T* GetData()
    {
        return data;
    }

    T* GetSecondElement()
    {
        return data[1];
    }

    int SizeOf()
    {
        return sizeof(T);
    }
};

struct ForwardDefined;

static TestTemplate<ForwardDefined> Global; // Legal to define

ForwardDefined* Test1()
{
    return Global.GetData();
}

ForwardDefined* Test2()
{
    // Not legal since method indirectly relies on sizeof(T)
    // error C2027: use of undefined type 'ForwardDefined'
    return Global.GetSecondElement();
}

int Test3()
{
    // Not legal since method relies on sizeof(T)
    // error C2027: use of undefined type 'ForwardDefined'
    return Global.SizeOf();
}

In InfectedImGui, we manually translate ImVector<T> since it's fairly simple and makes more sense for performance and simplicity for it to be a C# generic. However, this presents a puzzling challenge for a an analyzer like this. How do we know if a generic is going to try and dereference the type? Do we try to filter which methods are callable?

It's worth remembering that reference assemblies sometimes omit private fields too, so we can't necessarily rely on them. (Although I think that isn't super common anymore since it caused some issues with the BCL.)

PathogenDavid avatar Feb 27 '21 18:02 PathogenDavid