Fix TrackCache memory crash #85266
Fix TrackCache memory crash #85266
Conversation
KoBeWi
force-pushed
the
copy_more_RAM
branch
3 times, most recently
from
b76be6f
to
a69bf31
Compare
TokageItLab
force-pushed
the
copy_more_RAM
branch
from
a69bf31
to
1529f2e
Compare
KoBeWi
force-pushed
the
copy_more_RAM
branch
from
1529f2e
to
6a0c2a9
Compare
KoBeWi
force-pushed
the
copy_more_RAM
branch
from
6a0c2a9
to
ee3707c
Compare
|
There's a bunch of errors with GCC and Clang, see https://github.com/godotengine/godot/pull/85266/files or the CI logs. |
KoBeWi
force-pushed
the
copy_more_RAM
branch
from
ee3707c
to
fc5c55f
Compare
TokageItLab
force-pushed
the
copy_more_RAM
branch
3 times, most recently
from
5e1e07c
to
21ea636
Compare
scene/animation/animation_mixer.cpp
Outdated
| case Animation::TYPE_VALUE: { | ||
| AnimationMixer::TrackCacheValue *src = static_cast<AnimationMixer::TrackCacheValue *>(p_cache); | ||
| AnimationMixer::TrackCacheValue *tc = memnew(AnimationMixer::TrackCacheValue); | ||
| memcpy((void *)tc, src, sizeof(AnimationMixer::TrackCacheValue)); |
There was a problem hiding this comment.
I'm not sure it'll be safe to ignore this warning, I don't think it's overly careful saying not to memcpy virtual classes
There was a problem hiding this comment.
How it should be handled otherwise?
There was a problem hiding this comment.
With a copy constructor or copy assignment, as per the error suggestion I'd say
There was a problem hiding this comment.
From what I understand, this is only a problem if we ever add a new cache type that inherits a subtype AND the new type will use an existing enum value. It's unlikely and that would be a bug in itself, so it's not really a concern IMO.
There was a problem hiding this comment.
As long as the copying of the vtable is safe
One point further: is it really safe to memcpy a HashMap? Will it perform copy construction of that? Otherwise the pointers might be incorrect
There was a problem hiding this comment.
How do you mean? The audio track cache has a hash map in it, how is it not copied? The classes are copied here?
There was a problem hiding this comment.
If that is a problem, it may be safe not to copy for the audio, method, and animation tracks although I think the more problem is that the Reset track is allowed for them in the first place.
There was a problem hiding this comment.
It's be safe to copy just as long as you do a copy with the constructor and not just a raw copy of the memory, copying the memory stored in a hash map is unsafe as it uses raw data
This is what the first error that was popping up was about, it warns when the copy constructor isn't trivial, i.e. POD or like
There was a problem hiding this comment.
I haven't tested it as I'm not able to right now but my assumption here is that the members of these classes are copied wholesale byte by byte, but the problem with that is that at least HashMap is going to be unsafe if it is copied that way, as it handles memory by pointers, in the "best" case the resulting copied map would share data with the original, which at minimum causes problems with manipulation of one, might cause crashes due to invalid pointer access in the worst case
I also am not sure it's safe to memcpy a Ref as it should do refcounting and I suspect you'd have a double-reference in two elements, so no additional referencing would happen, causing extra freeing
I'd simply suggest copy constructors be added, it's simple enough and safe
There was a problem hiding this comment.
I agree so much with @AThousandShips that I've made #85302.
TokageItLab
force-pushed
the
copy_more_RAM
branch
from
21ea636
to
abd6121
Compare
KoBeWi
force-pushed
the
copy_more_RAM
branch
2 times, most recently
from
8f63b3e
to
e28ff3f
Compare
KoBeWi
force-pushed
the
copy_more_RAM
branch
from
e28ff3f
to
1c0a0f5
Compare
|
Tested on Linux with GCC, I confirm this fixes the MRP in #85233. The concerns outlined by @AThousandShips above still need further consideration, but given that this seems to fix the crashes on Windows and Linux without apparent further regression, I'm merging it already to include into the 4.2-rc2 build I'm starting now. Worse cast if there's some data corruption with the way hashmaps are copied, it shouldn't be worse than what we currently have before this PR. 💦 |
|
Thanks! |
|
Crossing my fingers it won't cause any problems, not familiar enough with it to test it specifically, but the thing to look out for is likely data loss, and the likely most visible error would be the safety check failing in Fortunately fixing it is trivial if there are issues, just add some copy constructors (might not even need explicit ones) and use Can throw together something when I have the time (likely not until tomorrow) just to be safe going forward, the change should be free for most of the cases as most of the types are trivially copyable For some context, see here |
Fixes #85250
Fixes #85233