fix(gta-core-five): crash when get invalid player bone matrix

[DaniGP17]

Goal of this PR

It should fix a crash caused when trying to get the global matrix of a bone in a skeleton, I guess it can occur when trying to get the matrix of a bone id that does not exist.

I haven't been able to test on my local server if this works correctly because I don't know how cheaters do it but it should work. This is my first time doing this type of fix so I don't know if what I've done is a good solution.

How is this PR achieving the goal

It hooks the GetGlobalMtx function and checks if m_Objects and all columns are null, if they are valid the original function is called.

This PR applies to the following area(s)

FiveM

Successfully tested on

Game builds: 1604, 2060, 2189, 3095, 3258

Platforms: Windows

Checklist

• Code compiles and has been tested successfully.
• Code explains itself well and/or is documented.
• My commit message explains what the changes do and what they are for.
• No extra compilation warnings are added by these changes.

Fixes issues

fixes #3095
fixes #2963

[florijanftw]

Hi, can you share any code that we can implement right now in fivem?

[d22tny]

Hi, can you share any code that we can implement right now in fivem?

.. the fix stands in gta-core, there's nothing you can implement to fix it on your side.

[Gogsi]

I briefly took a look at the crash a few days ago and came to the same conclusion as you about why it happens. However I'm not sure this is the cleanest fix, because this function probably gets called hundreds of times every frame. Hooks are kinda slow and on top of that you have multiple branches or a call back to the original function.

Despite all of this I'm not sure what the best fix is. Maybe the GetGlobalMtx method can be patched in-place to add a conditional jump? Can you try to do a benchmark with and without your fix to see if performance is actually affected, maybe I'm overestimating how expensive it is? Both from custom Release builds would be preferable to make sure adhesive or compiler settings aren't interfering.

[Gogsi]

Hi, can you share any code that we can implement right now in fivem?

.. the fix stands in gta-core, there's nothing you can implement to fix it on your side.

Until the proper fix is implemented, as a temporary hack maybe you can solve it by just kicking players that don't have a human player model. That is of course if your server doesn't have some functionality to switch your player model, otherwise you'll get false positives

[DaniGP17]

I briefly took a look at the crash a few days ago and came to the same conclusion as you about why it happens. However I'm not sure this is the cleanest fix, because this function probably gets called hundreds of times every frame. Hooks are kinda slow and on top of that you have multiple branches or a call back to the original function.

Despite all of this I'm not sure what the best fix is. Maybe the GetGlobalMtx method can be patched in-place to add a conditional jump? Can you try to do a benchmark with and without your fix to see if performance is actually affected, maybe I'm overestimating how expensive it is? Both from custom Release builds would be preferable to make sure adhesive or compiler settings aren't interfering.

Yes, this function is being executed many times, I mostly assume it is because of FootIK, it is a good idea to directly patch the function, I will try to do it 😅

[florijanftw]

Hi, can you share any code that we can implement right now in fivem?

.. the fix stands in gta-core, there's nothing you can implement to fix it on your side.

Until the proper fix is implemented, as a temporary hack maybe you can solve it by just kicking players that don't have a human player model. That is of course if your server doesn't have some functionality to switch your player model, otherwise you'll get false positives

Instructing thing is that player actually as you said did not have a human model, he was a fish if I remember correctly. So that is the way to tell who he is, right? Does he also have to be that fish to actually start crashing? Maybe we can fix it by just prohibiting that “fish” model.

And also I want to mention that yes we ban players like this, but for 1 person to have ability to disrupt like big server is for me crazy that can happen and still is not fixed 1 month later. So we have to be alert 0-24 for a kid that bought Lumia and has this crasher, I mean Its hard for me to wrap my head around it.

[Gogsi]

Hi, can you share any code that we can implement right now in fivem?

.. the fix stands in gta-core, there's nothing you can implement to fix it on your side.

Until the proper fix is implemented, as a temporary hack maybe you can solve it by just kicking players that don't have a human player model. That is of course if your server doesn't have some functionality to switch your player model, otherwise you'll get false positives

Instructing thing is that player actually as you said did not have a human model, he was a fish if I remember correctly. So that is the way to tell who he is, right? Does he also have to be that fish to actually start crashing? Maybe we can fix it by just prohibiting that “fish” model.

And also I want to mention that yes we ban players like this, but for 1 person to have ability to disrupt like big server is for me crazy that can happen and still is not fixed 1 month later. So we have to be alert 0-24 for a kid that bought Lumia and has this crasher, I mean Its hard for me to wrap my head around it.

The crash I saw was done with the fish model but I suspect other models will also work. Kicking/banning non-human models is probably better than targeting just the fish. Also I recommend at first kicking and logging the player, just to make sure you don't accidentally ban innocent people.

[topordenis]

The problem arises from the task driveby -> vehicle gun, which relies on the vehicle's task to fire a gun, combined with the removal of mounted weapons from the inventory. When this happens, the system defaults to the player's firing logic. However, the firing position is determined based on the upper arm (derived from the matrix), which vehicles, of course, don’t have.
//skeleton data is not streamed over network

[florijanftw]

The problem arises from the task driveby -> vehicle gun, which relies on the vehicle's task to fire a gun, combined with the removal of mounted weapons from the inventory. When this happens, the system defaults to the player's firing logic. However, the firing position is determined based on the upper arm (derived from the matrix), which vehicles, of course, don’t have. //skeleton data is not streamed over network

So I will make a script that will just switch back to original PED if cheater tries to use fish or any other non-human entity, I guess that will fix the problem right.

[tens0rfl0w]

I'm not sure if this patch will have the desired effect. After the early return, the callee will still reference void* *outMtx, which will lead to undefined behavior as the matrix datatype has no default initialization. To properly address this issue, the patch would need to modify the previous caller on the stack to ensure outMtx is handled correctly before the early return. (Maybe some sane default value instead of returning "nothing" will also work)

[DaniGP17]

I'm not sure if this patch will have the desired effect. After the early return, the callee will still reference void* *outMtx, which will lead to undefined behavior as the matrix datatype has no default initialization. To properly address this issue, the patch would need to modify the previous caller on the stack to ensure outMtx is handled correctly before the early return. (Maybe some sane default value instead of returning "nothing" will also work)

Not sure about this, but I think the outMtx is having a default value, I've tested without modifying it and the FootIK looks like is taking 0, 0, 0 or some specific position.

[tens0rfl0w]

I was able to reproduce the crash in this specific section by spoofing the model hash in the CPlayerAppearanceDataNode:

(lol)

Unfortunately, the proposed fix did not prevent the crashes in this area as intended.

After further investigation and experimenting with the hook (especially considering that we already have other hooks in place for similar calls, though specific), I believe I have found a solution that prevents crashes in this section:
3e05874
(May require some additional cleanup and maybe some different approach than using a trampoline regarding performance concerns).

[DaniGP17]

I was able to reproduce the crash in this specific section by spoofing the model hash in the CPlayerAppearanceDataNode

Could you share the modification did you made to test the crash?

After further investigation and experimenting with the hook (especially considering that we already have other hooks in place for similar calls, though specific), I believe I have found a solution that prevents crashes in this section: 3e05874 (May require some additional cleanup and maybe some different approach than using a trampoline regarding performance concerns).

Nice, the different approach refers to patching the function as Gogsi said?