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.

https://github.com/Source-Python-Dev-Team/Source.Python/blob/bcea09ef14a15cefdc4a5bd01a1521337af06879/src/core/modules/memory/memory_hooks.cpp#L129-L132

[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? https://github.com/Source-Python-Dev-Team/Source.Python/blob/bcea09ef14a15cefdc4a5bd01a1521337af06879/src/core/modules/memory/memory_wrap.cpp#L1052

[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: https://github.com/Source-Python-Dev-Team/Source.Python/blob/12a24f43db1e9ae86b233d244d12a6bba12cb351/src/core/sp_python.cpp#L339-L340

[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.