Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

CookStar
Copy link
Contributor

@jordanbriere
Copy link
Contributor

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

@jordanbriere jordanbriere linked an issue Jun 29, 2023 that may be closed by this pull request
@CookStar
Copy link
Contributor Author

CookStar commented Jun 29, 2023

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.

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

@jordanbriere
Copy link
Contributor

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
Copy link
Contributor Author

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?

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

@jordanbriere
Copy link
Contributor

jordanbriere commented Jun 29, 2023

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:

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

@CookStar
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pointer.set_pointer(None) causes Segmentation fault.
2 participants