Resolved memory management issues for CFunction.

[CookStar]

This addresses the problematic memory management issue for CFunction. The specific bugs to be fixed this time are memory leaks and crashes, but I think the code is now easier to understand.

What complicated the issue was the lack of clarity on who was responsible for CustomCallingConvention and NormalCallingConvention.

Now, CustomCallingConvention is managed by DynamicHooks or CFunction::DeleteHook, not by CFunction. For NormalCallingConvention, DynamicHooks is normally responsible when hooked, CFunction::DeleteHook is responsible if CFunction is destroyed first, and CFunction is responsible if CFunction::DeleteHook is called first.

Test Code:

from core import PLATFORM
from memory import CallingConvention
from memory import Convention
from memory import DataType
from memory import find_binary
from memory.hooks import HookType

class TestConvention(CallingConvention):
    default_convention = Convention.THISCALL

server = find_binary("server", srv_check=False)

if PLATFORM == "linux":
    signature = b"\x55\x89\xE5\x83\xEC\x18\x89\x5D\xF8\x8B\x5D\x10\x89\x75\xFC\x0F\xB6\x45\x0C"
else:
    signature = b"\x55\x8B\xEC\x53\x57\x8B\x7D\x0C\x8B\xD9\x85\xFF\x75\x2A\x5F"

# char const * CCSGameRules::GetChatFormat( bool bTeamOnly, CBasePlayer * pPlayer )
get_chat_format_normal = server[signature].make_function(
    Convention.THISCALL, (
        DataType.POINTER,
        DataType.BOOL,
        DataType.POINTER,
    ),
    DataType.STRING,
)

get_chat_format_custom = server[signature].make_function(
    TestConvention, (
        DataType.POINTER,
        DataType.BOOL,
        DataType.POINTER,
    ),
    DataType.STRING,
)

def func(args):
    pass

get_chat_format_normal.add_hook(HookType.PRE, func)
get_chat_format_normal._delete_hook()
del get_chat_format_normal # Memory Leak!

print("1")
get_chat_format_custom.add_hook(HookType.PRE, func)
print("2")
get_chat_format_custom._delete_hook()
print("3")
get_chat_format_custom.add_hook(HookType.PRE, func)
print("4")
get_chat_format_custom._delete_hook()
print("5")
get_chat_format_custom.add_hook(HookType.PRE, func) # Segmentation fault!
print("6")

[jordanbriere]

Convention.CDECL

Should not this be THISCALL?

del get_chat_format_normal # Memory Leak!

It doesn't leak here, but don't get collected instantly. If you force a collection with gc.collect(), or wait until the thresholds are met, the python proxy that own the pointer should be traversed and released.

get_chat_format_custom.add_hook(HookType.PRE, func) # Segmentation fault!

I can indeed reproduce the crash. It makes sense, since m_pCallingConvention is deleted through DynamicHooks into DeleteHook. Though, I'm not particularly interested to dive into the convention rabbit hole again. 😆

I trust that, as always, you did extensive testings but I will ask either way; did all contexts that were discussed into #344 been tested?

Thanks, and welcome back, btw! :)

[CookStar]

Should not this be THISCALL?

Oh, my mistake, I must have mixed them up when I was switching the codes.

It doesn't leak here, but don't get collected instantly. If you force a collection with gc.collect(), or wait until the thresholds are met, the python proxy that own the pointer should be traversed and released.

It is not a CustomCallingConvention we are talking about here. When the function is hooked, the ownership of m_pCallingConvention is moved to DynamicHooks. However, if CFunction::DeleteHook() is executed before the CFunction is destroyed, m_bHooked will remain true and m_pCallingConvention will leak when the CFunction gets destroyed.

Test Code:

def test():
    for i in range(10000000):
        get_chat_format = server[signature].make_function(
            Convention.THISCALL, (
                DataType.POINTER,
                DataType.BOOL,
                DataType.POINTER,
            ),
            DataType.STRING,
        )

        get_chat_format.add_hook(HookType.PRE, func)
        get_chat_format._delete_hook()

test()

I trust that, as always, you did extensive testings but I will ask either way; did all contexts that were discussed into #344 been tested?

I've passed the situations I can think of, and the code I usually work with, but are there any other situations that make you nervous?

[jordanbriere]

I've passed the situations I can think of, and the code I usually work with, but are there any other situations that make you nervous?

Nothing in particular, just wanted to confirm that issues for normal uses are not being re-introduced to fix problems related to manual hooks deletion.