Fixed ExtractAddress causing Segmentation fault if it received None object. (#481)

[CookStar]

#481

[jordanbriere]

I don't think this should raise unless bValidate is true and should simply return 0 otherwise. Main reason being for consistency with all the other functions exported (e.g. you can pass None as NULL for any argument that accept a pointer, or you will get None back for a call that returns NULL, etc.).

[CookStar]

I don't think this should raise unless bValidate is true and should simply return 0 otherwise. Main reason being for consistency with all the other functions exported (e.g. you can pass None as NULL for any argument that accept a pointer, or you will get None back for a call that returns NULL, etc.).

I thought the same thing at first, but there is one thing that clearly breaks consistency, and that is when you override the hooked function.

from entities.hooks import EntityCondition, EntityPreHook

@EntityPreHook(EntityCondition.is_player, 'give_named_item')
def pre_give_named_item(args):
    return None # None cannot override the function, nor can it return NULL.

And those who do not clearly know the following behavior will create unnecessary confusion, unable to understand why None returns NULL elsewhere, but not with a function override.

Source.Python/src/core/modules/memory/memory_hooks.cpp

Lines 129 to 132 in bcea09e

	if (!pyretval.is_none())
	{
	bOverride = true;
	switch(pHook->m_pCallingConvention->m_returnType)

[jordanbriere]

That's a callback, not a function. And that's by design, because None is automatically returned when you omit the return statement.

When you call any functions exported from C++, None from Python is extracted as 0. A good example for this is get_angle_vectors(forward=None, right=None, up=None) interpreted as AngleVectors(NULL, NULL, NULL). Same for functions exported from the engine that return NULL, etc. And I don't see why every function that forward an argument to ExtractAddress should behave differently.

Moreover, you are likely to get the same crash if oPtr defines a _ptr method that does return None.

[CookStar]

That's a callback, not a function. And that's by design, because None is automatically returned when you omit the return statement.

I am not talking about the callback, I'm talking about the function that is expected to return DataType.POINTER, give_named_item is a function that returns DataType.POINTER. You cannot return NULL (Pointer(0)) with None.

If NULL exists to serve as NULL, then everything should align with NULL. If not, then why does NULL exist?

Source.Python/src/core/modules/memory/memory_wrap.cpp

Line 1052 in bcea09e

_memory.attr("NULL") = object(CPointer());

[jordanbriere]

I am not talking about the callback, I'm talking about the function that is expected to return DataType.POINTER, give_named_item is a function that returns DataType.POINTER. You cannot return NULL (Pointer(0)) with None.

It's by design. Calling a Python function that does not return anything, for example:

def pre_give_named_item(args):
    do_whatever_but_does_not_return_anything()

Automatically return None. How would you differentiate a callback that just want to listen with one that want to override the returned value of the original function? In such case, this is where NULL becomes useful because you explicitly tell the hook manager that you want to override the call with your own value.

Regardless, there is a clear distinction between a function and a callback. When None is passed to a function, it should be interpreted as NULL for consistency with every other functions, and more importantly; backward compatibility. Changing the behaviour now would break many existing codes/features.

EDIT: As mentioned above, this crash:

class Foo:
    def _ptr(self):
        return None

ptr.set_pointer(Foo()) # Crash

This is inconsistent:

ptr.set_string_pointer(None) # Work
ptr.set_pointer(None) # ValueError: None was passed, expected Pointer(0) or NULL.

This is also inconsistent:

player.give_named_item('weapon_awp', econ_item_view=None) # Work

@EntityPreHook(EntityCondition.is_player, 'give_named_item')
def pre_give_named_item(args):
    args[3] = None # ValueError: None was passed, expected Pointer(0) or NULL.

Etc. The fix for this should really be as simple as ensuring pPtr is non-NULL prior to interacting with it (untested):

inline unsigned long ExtractAddress(object oPtr, bool bValidate = false)
{
	CPointer* pPtr;

	extract<CPointer *> extractor(oPtr);
	if (!extractor.check())
	{
		oPtr = oPtr.attr(GET_PTR_NAME)();
		pPtr = extract<CPointer *>(oPtr);
	}
	else
	{
		pPtr = extractor();
	}

	if (!pPtr)
	{
		if (bValidate)
			BOOST_RAISE_EX...
		else
			return 0;
	}

	if (bValidate)
		pPtr->Validate();

	return pPtr->m_ulAddr;
}

The same should also be done there:

Source.Python/src/core/sp_python.cpp

Lines 339 to 340 in 12a24f4

	CPointer* pAddr = extract<CPointer*>(obj);
	return (void *) pAddr->m_ulAddr;

[CookStar]

It's by design. Calling a Python function that does not return anything, for example:

def pre_give_named_item(args):
    do_whatever_but_does_not_return_anything()

Automatically return None. How would you differentiate a callback that just want to listen with one that want to override the returned value of the original function? In such case, this is where NULL becomes useful because you explicitly tell the hook manager that you want to override the call with your own value.

Regardless, there is a clear distinction between a function and a callback. When None is passed to a function, it should be interpreted as NULL for consistency with every other functions, and more importantly; backward compatibility. Changing the behaviour now would break many existing codes/features.

It is not by design, both are just compromises made due to the constraints imposed by Python and Boost.Python.

The function arguments provided by Source.Python accept None because Boost.Python foolishly associated None with NULL, and that wound should not be expanded. The type of None is never checked, this means that anytime you mistakenly pass None to a function, you are in danger of crashing.

make_object(Vector, None) # Crash!

Entity.create(None) # Crash!

ptr.set_string_array(None) # Crash!

It should be considered an exception to the function provided by Boost.Python and should not get users into the habit of setting NULL with None. NULL should be used in situations where NULL(Pointer(0)) can be used.

[Ayuto]

Oops, linked the wrong PR. I actually meant #481.

For now, the fix should be sufficient. In the past we allowed None to be used instead of NULL for convenience. The main reason why NULL exists are pre hooks. As you know you can return a value unequal to None to prevent the actual function from executing. If you do so, the returned value is returned to the caller.

If a function does not return anything in Python, None is returned implicitly. So, in the hook handler we check against None and allow the original function to be executed. But this is a problem if you really want to return a NULL pointer. That's why we added NULL, which can be used in these cases.

I do agree that explicitly passing NULL instead of None would make it more safe. Then we also need to update our hook handler to receive a tuple containing two values: a hook action and the value that should be returned to the caller. Or maybe some classes like this:

def my_hook1(args):
    return HookAction.Continue() # or None

def my_hook2(args):
    return HookAction.Override(my_value)