Resolve GDScript::clear() heap-use-after-free ASAN errors
#71028
Conversation
adamscott
force-pushed
the
make-gdscript-clear-less-prone-to-heap-use-after-free
branch
from
29a5d6d to
d5940e1
Compare
adamscott
changed the title
GDScript::clear() less prone to heap-use-after-free ASAN errorGDScript::clear() heap-use-after-free ASAN errors
| subclasses.clear(); | ||
| // subclasses are also held by constants, clear those as well | ||
| for (KeyValue<StringName, Variant> &E : constants) { | ||
| GDScript *gdscr = _get_gdscript_from_variant(E.value); |
There was a problem hiding this comment.
Do you need to consider situations like const EnumNewName = SomeOtherClass.NamedEnum? Does this count as some sort of weak reference?
There was a problem hiding this comment.
You cannot have enums without a GDScript instance.
What I mean is that SomeOtherClass (and its components) is a dependency of ThisClass
There was a problem hiding this comment.
I trust what you say! It just occurred to me as someone who isn't knowledgeable about this code :)
So basically, SomeOtherClass had to show up somewhere in the code, and therefore has already been taken into account somewhere else.
What happens with
class_name A
const B = preload("b.gd")I take it it is also accounted for as dependency by this code? Or is it taken to be a subclass?
Let me know if this isn't useful - this is me taking shots in the dark trying to make sense of the code :)
There was a problem hiding this comment.
What happens with
class_name A const B = preload("b.gd")I take it it is also accounted for as dependency by this code? Or is it taken to be a subclass?
From that example, B is a dependency of A
There was a problem hiding this comment.
Or is it taken to be a subclass?
A subclass is a class within a gdscript file. It has it's own GDScript instance.
class_name ThisIsNotASubclass
class ThisIsASubclass:
passThere was a problem hiding this comment.
Hahah, the naming of all this is messing with me :) I would call that an inner class, whereas a subclass would be class B extends A!
Sounds like you've gotten all this figured out though, so I'm happy to see this merged! The changes make sense!
| // remove instances that are still in the clear loop | ||
| for (GDScript *E : must_clear_dependencies) { | ||
| must_clear_dependencies_objectids.insert(E, E->get_instance_id()); | ||
| GDScript::ClearData data; |
There was a problem hiding this comment.
A comment in this section of code stating that this defines whether this is the "parent" which is responsible for destroying the scripts later would make sense, I think?
There was a problem hiding this comment.
In fact, maybe the name initialized_clear_data ought to be is_responsible_for_clearing or something that's a bit more obvious?
adamscott
force-pushed
the
make-gdscript-clear-less-prone-to-heap-use-after-free
branch
2 times, most recently
from
193c644 to
d83efbb
Compare
adamscott
force-pushed
the
make-gdscript-clear-less-prone-to-heap-use-after-free
branch
from
d83efbb to
d221999
Compare
|
Thanks! |
Depending of the code changed, ASAN would complain about
heap-use-after-freeerrors.That happens to #69590, the build fails, but the code is valid. It just shows a flaw in the current
GDScript::clear()implementation.This PR makes it so that the parent script collects every functions and scripts about to be cleaned by the descendants, then cleans them itself, rather than having to deal with deleted references (and to deal with
heap-use-after-freeinstances).Removed the now useless
GDScript::skip_dependenciesboolean.