Skip to content

[GDScript] Prevent some vararg methods binding arguments to member variables #88905

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

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

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Feb 27, 2024

Calling some methods from GDScript with member variable arguments causes the pointer to that argument being passed instead of the value, affects:

  • Signal.emit
  • Object.emit_signal
  • SceneTree.call_group/_flags

Tried a separate method that's possibly more secure, by copying arguments into temporaries if they are member variables, this is a bit hit and miss and had some problems applying them just for vararg methods, this problem doesn't really affect other cases significantly either as most of the time it doesn't cause this kind of chain effect, so I feel this is more of a targeted situation where we can just fix it in the places where it's needed, instead of hitting every method called with a member variable with a temporary

I only added these fixes to the bound methods, so it won't affect calls from c++ normally, like Signal.emit, but instead the bound method under variant_call

Note also that the ptrcall methods in Variant use temporary storage for variants, and these are called from extensions AFAIK, so there's precedent for this solution

The immediate, indiscriminate solution (as I couldn't reliably test for vararg, and therefore would miss some cases) would have been:

diff --git a/modules/gdscript/gdscript_compiler.cpp b/modules/gdscript/gdscript_compiler.cpp
index 13ed66710c..5a9a5b2264 100644
--- a/modules/gdscript/gdscript_compiler.cpp
+++ b/modules/gdscript/gdscript_compiler.cpp
@@ -614,6 +614,11 @@ GDScriptCodeGenerator::Address GDScriptCompiler::_parse_expression(CodeGen &code
                                if (r_error) {
                                        return GDScriptCodeGenerator::Address();
                                }
+                               if (arg.mode == GDScriptCodeGenerator::Address::MEMBER) {
+                                       GDScriptCodeGenerator::Address temp = codegen.add_temporary(arg.type);
+                                       gen->write_assign(temp, arg);
+                                       arg = temp;
+                               }
                                arguments.push_back(arg);
                        }

This is a bit brute force, but it works as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't test the other methods here AFAIK as groups don't work in the testing, unless I'm mistaken

Comment on lines 1075 to 1124
LocalVector<Variant> args;
LocalVector<const Variant *> argptrs;
Copy link
Member

Choose a reason for hiding this comment

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

Allocations here are are too bad for performance. I'd suggest using alloca().

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do, wasn't well versed in using it when I wrote this one

Copy link
Member Author

Choose a reason for hiding this comment

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

Added everywhere, might not be as relevant to the variant call one but shouldn't hurt

@AThousandShips AThousandShips force-pushed the gdscript_arg_ref branch 2 times, most recently from 312936d to 4d06340 Compare April 3, 2024 13:22
args = (Variant *)alloca(sizeof(Variant) * argc);
argptrs = (const Variant **)alloca(sizeof(Variant *) * argc);
for (int i = 0; i < argc; i++) {
memnew_placement(&args[i], Variant(*p_args[i + 1]));
Copy link
Member Author

Choose a reason for hiding this comment

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

@AThousandShips

This comment was marked as outdated.

@AThousandShips AThousandShips changed the title [GDScript] Prevent some vararg methods accessing member variables [GDScript] Prevent some vararg methods binding arguments to member variables Apr 3, 2024
@AThousandShips AThousandShips added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Jul 25, 2024
@AThousandShips AThousandShips modified the milestones: 4.3, 4.4 Jul 25, 2024
@AThousandShips AThousandShips added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jul 25, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner October 11, 2024 10:47
@AThousandShips AThousandShips force-pushed the gdscript_arg_ref branch 2 times, most recently from cf2ccae to f8ec296 Compare December 23, 2024 22:26
@AThousandShips AThousandShips modified the milestones: 4.4, 4.5 Jan 28, 2025
@AThousandShips AThousandShips added the cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release label Feb 5, 2025
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 19, 2025
Calling some methods from GDScript with member variable arguments causes
the pointer to that argument being passed instead of the value, affects:
* `Signal.emit`
* `Object.emit_signal`
* `SceneTree.call_group/_flags`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release topic:gdscript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameter emitted with a signal references member variable instead of passed Variant
3 participants