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

GDScript: Fix some lambda bugs #81605

Merged
merged 1 commit into from
Sep 16, 2023

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Sep 13, 2023

As stated in #81572, GDScriptFunction::_get_call_error() can crash the engine.

int errorarg = p_err.argument;
// Handle the Object to Object case separately as we don't have further class details.
#ifdef DEBUG_ENABLED
if (p_err.expected == Variant::OBJECT && argptrs[errorarg]->get_type() == p_err.expected) {

err_text = _get_call_error(err, "function '" + methodstr + (is_callable ? "" : "' in base '" + basestr) + "'", (const Variant **)argptrs);

I decided to check what err.argument was equal to, and it turned out to be -1. This is because lambda captures are hidden arguments of lambda function.

if (captures_amount > 0) {
Vector<const Variant *> args;
args.resize(p_argcount + captures_amount);
for (int i = 0; i < captures_amount; i++) {
args.write[i] = &captures[i];
}
for (int i = 0; i < p_argcount; i++) {
args.write[i + captures_amount] = p_arguments[i];
}
r_return_value = function->call(nullptr, args.ptrw(), args.size(), r_call_error);
r_call_error.argument -= captures_amount;

If for some reason a capture is invalid (for example, due to a bug in the compiler or if the object has been freed at this point), then this leads to a negative argument index and a crash.

if (!argument_types[i].is_type(*p_args[i], true)) {
r_err.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT;
r_err.argument = i;
r_err.expected = argument_types[i].builtin_type;

bool was_freed = false;
Object *obj = p_variant.get_validated_object_with_check(was_freed);
if (!obj) {
return !was_freed;
}

Status

@tool
extends EditorScript

func _run() -> void:
    var x: Node = Node.new()
    x.free()
    (func (): print(x)).call()

@dalexeev dalexeev added this to the 4.2 milestone Sep 13, 2023
@dalexeev dalexeev requested a review from a team as a code owner September 13, 2023 06:52
@akien-mga
Copy link
Member

I confirm this fixes #81572 on my project 🎉

I tested #62769 and documented my findings there for the current master branch. This PR doesn't seem to improve or worsen things.

@dalexeev
Copy link
Member Author

Thanks for testing and documenting! I also saw some of these and several others. It looks like hot reloading can cause a random effect, depending on how the bytecode length (or something else) changes. In theory this can be dangerous.

@dalexeev
Copy link
Member Author

dalexeev commented Sep 13, 2023

Maybe this also fixes/removes a crash #79707 ✔️, #75421 ❌ and/or #76108 ❌. I'll test it later.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not proficient with the GDScriptCompiler but I tested the changes and confirmed the bug fixes, without visible regression in a project that uses lambdas and awaits them a lot.

So I think we can merge so this gets tested in dev5.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another great PR from our @dalexeev that pass the test! Thanks for the numerous tests added.

@akien-mga akien-mga merged commit 6c1be30 into godotengine:master Sep 16, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment