CorrM / Unreal-Finder-Tool

Useful tool to help you fetch and dump Unreal Engine 4 Games information.
GNU General Public License v3.0
434 stars 169 forks source link

Wrong indirection level in generated SDK #46

Closed jwright4531 closed 5 years ago

jwright4531 commented 5 years ago

I had a problem that every parameter in generated SDK had extra indirection level (float became float, UWorld became UWorld**, etc.):

// Wrong
void UCanvas::K2_DrawBox(struct FVector2D* ScreenPosition, struct FVector2D* ScreenSize, float* Thickness, struct FLinearColor* RenderColor) { ... }
// Correct
void UCanvas::K2_DrawBox(const struct FVector2D& ScreenPosition, const struct FVector2D& ScreenSize, float Thickness, const struct FLinearColor& RenderColor) { ... }

After debugging I found out that due to typos in Package::GenerateMethods following piece of code performs reads of garbage data. If read resulted in 0 or 1, bug wasn't noticable but in my case I had large negative numbers there.

void Package::GenerateMethods(const UEClass& classObj, std::vector<Method>& methods) const
{
    // prop represents UFunction which should NOT be casted to UEProperty/UProperty because inheritance sequence looks like this: UFunction -> UStruct -> UField -> UObject
    // We are only using GetNext here so casts should be to UEField/UField
    for (auto prop = classObj.GetChildren().Cast<UEProperty>(); prop.IsValid(); prop = prop.GetNext().Cast<UEProperty>())
    {
        if (prop.IsA<UEFunction>())
        {
            // param represents UProperty, this is fine, inheritance sequence: UProperty -> UField -> UObject
            for (auto param = function.GetChildren().Cast<UEProperty>(); param.IsValid(); param = param.GetNext().Cast<UEProperty>())
            {
                //...

                if (p.ParamType == Type::Default)
                {
                    // Here is the major problem, GetArrayDim() should be called on UProperty representing current parameter (param variable) NOT on UFunction which makes no sense and will always read garbage
                    if (prop.GetArrayDim() > 1)
                    {
                        p.CppType = p.CppType + "*";
                    }
                    else if (info.CanBeReference)
                    {
                        p.PassByReference = true;
                    }
                }

                // prop usage seems invalid here as well for same reason, why would we need same prop (UFunction) for every p? This should be param
                parameters.emplace_back(std::make_pair(prop, std::move(p)));
            }

            // In current state this is usless because every first is exactly the same and isn't even UEProperty/UProperty
            std::sort(std::begin(parameters), std::end(parameters), [](auto && lhs, auto && rhs) { return ComparePropertyLess(lhs.first, rhs.first); });
        }
    }
}

I will submit a pull request for this, issue posted for reference.

CorrM commented 5 years ago
for (auto prop = classObj.GetChildren().Cast<UEProperty>(); prop.IsValid(); prop = prop.GetNext().Cast<UEProperty>())

here i get the children which is UEProperty (Children is just pointer to address) then i get that address and read it as UEProperty, then after one loop, i need to get the next (next is just pointer to address) then i just read it's memory as UEProperty

We are only using GetNext here so casts should be to UEField/UField

cast here not as you expect, cast here is just like Read Memory. then what's happen with this line here,

i don't do it for get next, as i said it's just pointer. as here i think it's just to fit the prop type. i mean just to get object as UEProperty. not necessary to be UEProperty it's just to fit prop var type.

About IsA: it's not about what the object inherit from. as you see here it's just check FName Index.

then about your pull i will not close it until u give me a replay.

========================= can u explain more about .?

I had a problem that every parameter in generated SDK had extra indirection level (float became float*, UWorld* became UWorld**, etc.):
jwright4531 commented 5 years ago

I know what Cast method does, I know it reads remote memory. I studied it and debugged it. The problem is that you read something as UEProperty while it's NOT UEProperty. That wouldn't be a big deal if you just used GetNext, that would be very unelegant (because cast to UEField is enough) but it would work.

However here that's not the case. "prop" is UEFunction and it's used as such most of the time:

if (!prop.IsA<UEFunction>())
    continue;

auto function = prop.Cast<UEFunction>();

Method m;
m.Index = function.GetIndex();
m.FullName = function.GetFullName();
m.Name = MakeValidName(function.GetName());

m.Name = generator->GetSafeKeywordsName(m.Name);
if (uniqueMethods.find(m.FullName) != std::end(uniqueMethods))
    continue;

uniqueMethods.insert(m.FullName);

m.IsNative = function.GetFunctionFlags() & UEFunctionFlags::Native;
m.IsStatic = function.GetFunctionFlags() & UEFunctionFlags::Static;
m.FlagsString = StringifyFlags(function.GetFunctionFlags());

Problem is that later you use it as UEProperty when doing GetArrayDim(). This is clearly an error, it's being done inside inner loop that iterates over parameters (which ARE UEProperties). Why would you refer to "prop" that represents the function in a loop that loops over parameters in order to fill "Method::Parameter p" variable? Why would you execute "parameters.emplace_back(std::make_pair(prop, std::move(p)));" when prop will be constant in the context of inner loop?

I see two errors here:

Both errors are fixed in pull request. For your last question please see K2_DrawBox example. GetArrayDim was reading garbage and condition was:

if (prop.GetArrayDim() > 1)
{
    p.CppType = p.CppType + "*";
}

If your 4 bytes of garbage evaluated to 0 or 1 you were fine. In my case it read part of some pointer and I ended up with negative numbers. I got SDK full of pointers to primitive types and pointers to pointers.

If you are confused by my fragment in the original issue please refer to original code because I omitted some parts. You will clearly see that references to prop inside inner loop were just wrong and served no real purpose.

CorrM commented 5 years ago

as you i used other sdk as BASE, so some thing like that i can just not see it as it's not generate an issue before.

anyway i think that's will be the most important Issue i get for this repo 😂 . so i will see what i can do. and i will give u another replay, i will test and see your pull.

thanks for all debugging, Suggestions and fixes

CorrM commented 5 years ago

Thank you for the pull, it's accepted as u said it's not accessory to be UProperty. for 2nd problem i will check it after do some stuff on this branch SDK-New-Featuers

if u already fixed it i love to get new pull request.

jwright4531 commented 5 years ago

Both were fixed in that PR :) Two occurrences of prop were replaced with references to param so it's all fine now. New version of inner loop properly iterates over parameters. Thanks for merging it.

CorrM commented 5 years ago

❤️