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

Fix call with wrong arguments in Callable.callv() when passing a readonly (const) Array #93636

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nolkaloid
Copy link
Contributor

Use a duplicate of the passed array when it is read-only.

Prior to this fix, when the array was marked as read-only, the temporary address of p_arguments->read_only was used for all parameters. Consequently, this caused all parameters to reference the last element of the array.

Fixes #93600.

@AThousandShips
Copy link
Member

This would really benefit from having a test added under modules/gdscript/tests/scripts/runtime/features to make sure it actually fixes the bug and makes sure it remains fixed

@Nolkaloid
Copy link
Contributor Author

As this is not GDScript specific, shouldn't it be a unit test in tests/core/variant/test_callable.h ?

@AThousandShips
Copy link
Member

AThousandShips commented Jun 26, 2024

We do many of these tests there

Also how did you confirm this was not script specific? I at least suspect it's specific to arguments in script methods being called, due to how the bindings and data passing works

My bad missed some details, but I think the script side tests works best to fix this

@AThousandShips
Copy link
Member

AThousandShips commented Jun 26, 2024

Given some of the details here I think I have a potential alternative solution that might be better, I'll look at it along with my other changes in:

But if you want to try it out I'll drop the alternative here, it probably would be faster and easier avoiding a full duplication with:

	Variant *args = nullptr;
	const Variant **argptrs = nullptr;

	if (argcount) {
		argptrs = (const Variant **)alloca(sizeof(Variant *) * argcount);

		// Copy the arguments if read-only, otherwise the address of p_arguments->read_only will be copied.
		if (p_arguments.is_read_only()) {
			args = (Variant *)alloca(sizeof(Variant) * argcount);
			for (int i = 0; i < argcount; i++) {
				memnew_placement(&args[i], Variant(p_arguments[i]));
				argptrs[i] = &args[i];
			}
		} else {
			for (int i = 0; i < argcount; i++) {
				argptrs[i] = &p_arguments[i];
			}
		}
	}

	CallError ce;
	Variant ret;
	callp(argptrs, argcount, ret, ce);

	if (args) {
		for (int i = 0; i < argcount; i++) {
			args[i].~Variant();
		}
	}

	return ret;

Edit: Tested and it works well, a bit more code but avoids unnecessary duplication

@Nolkaloid
Copy link
Contributor Author

Nolkaloid commented Jun 26, 2024

Having a second buffer allocated for a copy of the elements is what I have done initially, but I chose to use duplicate instead in the end, because I found it more readable.

I'm not sure that the call to duplicate has that much overhead since the underlying Vector uses CoW if I recall correctly. But I might be wrong.

@AThousandShips
Copy link
Member

I'm not sure that the call to duplicate has that much overhead since the underlying Vector uses CoW if I recall correctly. But I might be wrong.

I'm not sure, that's a good point and it might not matter, but i don't think the code is significantly less readable and it avoids potential weirdness

@Nolkaloid
Copy link
Contributor Author

I did a quick check, and indeed the duplication is done using CoW. If it were up to me I'd keep the duplicate as it's simpler IMO, but I don't have strong opinions about it. You tell me. I'll add the test in modules/gdscript/tests/scripts/runtime/features in the meantime.

@AThousandShips
Copy link
Member

You decide, both are fine by me, and let's see what the core team says

@RandomShaper
Copy link
Member

A few thoughts:

  • CoW involves dealing with the atomic refcount, with performance implications.
  • This looks like an ad hoc fix for an issue due to how read-only arrays are implemented. It may be better to assess solutions to the root issue.

@AThousandShips
Copy link
Member

I think it might be difficult to handle the issue with read access on the array itself, given the nature of the reference, but I'd say those points reinforce the use of my suggestion above, it avoids the COW issues, and avoids any issues with the data access

I can't think of a reliable way to handle the read only status of array without breaking compatibility, or allocating more data somehow, but regardless I think this situation warrants at least a temporary local solution

@AThousandShips
Copy link
Member

This also needs to be done for Object::callv

core/object/object.cpp Outdated Show resolved Hide resolved
core/object/object.cpp Outdated Show resolved Hide resolved
core/variant/callable.cpp Outdated Show resolved Hide resolved
core/variant/callable.cpp Outdated Show resolved Hide resolved
core/object/object.cpp Show resolved Hide resolved
@Nolkaloid Nolkaloid force-pushed the fix-const-callv branch 3 times, most recently from b778928 to c15e83b Compare June 28, 2024 14:45
@Nolkaloid Nolkaloid force-pushed the fix-const-callv branch 2 times, most recently from d9a34ce to 6a2bab7 Compare June 28, 2024 14:47
@Nolkaloid
Copy link
Contributor Author

Indeed this solution is not perfect, and we should consider modifying how read-only arrays are implemented. I will try to think of a nice solution when I have more time.

@AThousandShips
Copy link
Member

Made an issue tracking the broader issues of the read-only state:

@vnen vnen modified the milestones: 4.3, 4.4 Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Callable.callv() will make call with wrong arguments if a const Array is passed
5 participants