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

Add size check in Control::_edit_set_state() to fix crash #46620

Merged

Conversation

sps1112
Copy link
Contributor

@sps1112 sps1112 commented Mar 3, 2021

fixes #46017
add size check to prevent crash when executing Control.new()._edit_set_state({})

@sps1112 sps1112 requested a review from a team as a code owner March 3, 2021 11:03
@sps1112 sps1112 changed the title Add size check in Control._edit_set_state to prevent crash Add size check in Control:_edit_set_state() to fix crash Mar 3, 2021
@sps1112 sps1112 changed the title Add size check in Control:_edit_set_state() to fix crash Add size check in Control::_edit_set_state() to fix crash Mar 3, 2021
@KoBeWi KoBeWi added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release crash topic:gui labels Mar 3, 2021
@KoBeWi KoBeWi added this to the 4.0 milestone Mar 3, 2021
@KoBeWi
Copy link
Member

KoBeWi commented Mar 3, 2021

This method will also crash when the passed Dictionary is missing any of the used keys, e.g. Control.new()._edit_set_state({"test": 1}) will still crash.

So to fix this properly, you'd have to add a check for each existence of each key. I wonder if it's worth it though, the method isn't visible in documentation. It's mean for internal use only (see godotengine/godot-proposals#2285) and we didn't have crash reports from editor usage (and even if we had, the error doesn't help much as it still needs to be fixed).

@sps1112 sps1112 force-pushed the fix-control.edit_set_state-crash branch from 022f289 to d92b37e Compare March 3, 2021 12:54
scene/gui/control.cpp Outdated Show resolved Hide resolved
@sps1112 sps1112 force-pushed the fix-control.edit_set_state-crash branch from d92b37e to ef1d58f Compare March 4, 2021 05:26
@akien-mga akien-mga merged commit 8ff25ff into godotengine:master Mar 4, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

akien-mga commented Mar 4, 2021

Cherry-picked for 3.2.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Mar 4, 2021
@akien-mga
Copy link
Member

akien-mga commented Mar 4, 2021

Reverted with 372b1b8 (and undid cherry-pick before pushing) as the change is wrong:

ERROR: Condition "(p_state.size() <= 0) || p_state["rotation"].is_null() || p_state["scale"].is_null() || p_state["pivot"].is_null() || p_state["anchors"].is_null() || p_state["offsets"].is_null()" is true.
   at: _edit_set_state (scene/gui/control.cpp:76)

Variant::is_null() checks if the Variant is a null Object, that's not the right way to validate the existence of dictionary keys.

Try to move any Control, this change spams:

ERROR: Condition "(p_state.size() <= 0) || p_state["rotation"].is_null() || p_state["scale"].is_null() || p_state["pivot"].is_null() || p_state["anchors"].is_null() || p_state["offsets"].is_null()" is true.
   at: _edit_set_state (scene/gui/control.cpp:76)

@KoBeWi
Copy link
Member

KoBeWi commented Mar 4, 2021

Right, I didn't test it in the editor before approving and was surprised it works .-.

This should be p_state.has("rotation") etc.

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.

Executing Control.new()._edit_set_state({}) crashes Godot
3 participants