My friends, gather around as I learned something about the C standard
that is horrifying and may keep you, dear reader, up at night.
My journey began trying to fix something entirely unrelated and not
wanting to wait for ubsan builds when changing a testcase. So me, in my
infinite naivete just built the engine with tests=yes, but
optimizations turned on.
This resulted in a segfault on "[Audio][AudioStreamWAV] Save empty file".
Well, then, I thought. Lets built with asan then and find out where this
happens! Would it surprise you, my fellow traveler, that the results
were that no such crash occurred?
Thus, to the debugger I go! Fearless, with great optimism. Where I find
that through many an indirection the crash came because, somehow,
CowData::_unref() was getting called with a _ptr set to 0x1.
This can of course only end in tears. Or segmentation faults as we try
to read an atomic variable at the somewhat inconveniently situated
address at 0xfffffffffffffff0.
So I went and looked at the likely culprit, blaming many an innocent
recent change along the way. I shall spare you the falsly accused. But
if for some reason you slept poorly last night, I can assure you that
the voodoo dolls have been put away and will not be harmed further.
So in AudioStreamWAV::get_data() we go, where we find a perfectly
reasonable function! It checks to see whether or not its data is empty,
and if it is not it will resize a temporary Vector to have data_bytes of
space, after which it will do a perfectly pedestrian memcpy() and all is
well in the world.
Or so it seems! After many an hour of despair and disassembly I, at
last, decided to look at where the data gets set! A breakthrough!
Because of the padding data is never empty! So the code always runs!
Eureka! One would think. But then, foolishly, I looked into the
get_data() function one more. My mortal enemy was staring me in the
face, laughing. Because it did not care about this. Sure, the check was
worthless but still... What are we left with.
At this point I could feel the method mocking me.
"I resize the vector to 0, I then memcpy zero bytes into it." It said,
DARING me to object to this state of affairs.
And yet, if I changed the function to check for "data_bytes" rather
than data.is_empty() no crashes.
Was this a compiler bug? Am I losing my mind? But then... I remembered
the mantra of the wise compiler druids... "It Is Not A Compiler Bug".
But what then! The bug does not happen when memory is being watched!
Valgrind agreed that while accessing the SafeRefCount at
0xfffffffffffffff0 was incredibly rude, it did not inform me of anything
else untoward happening. So I read the memcpy() manpage... nothing... I
read the the memcpy() posix spec... nothing.
Finally, in despair and because I had nothing left to lose... The ISO C
language specification. As I was reading, I could hear
AudioStreamWAV::get_data() cackling, knowing that its time was up, but
proud of the madness it caused in my soul. Knowing I would never be the
same.
The behavior is undefined if either dest or src is an invalid or null pointer.
So... Here I stand before you, a broken person. But one richer in
knowledge.
I write you this from the depths of madness in the hopes that you, dear
reader, can be spared this ordeal.
May god have mercy on our souls.
We trigger the following sequence of events:
* memcpy(null, null, 0) is UB, thus dest and src cannot be null
* we inline the calls to the ctor and dtor
* now we have a function that does something that "proves"
dest cannot be null
* we inline cowdata::_unref() which does a null check, on something
that the compiler just convinced itself cannot be null
* the compiler removes the dead code branch where _ptr == nullptr
* we start to do pointer arithmetic on a nullptr and get send to
uninitialized memory.
Co-Authored-By: Jason Beckmann <jasonabeckmann@gmail.com>
We now cache the Node*<>TreeItem* mapping in the SceneTreeEditor. This
allows us to make targeted updates to the Tree used to display the scene
tree in the editor.
Previously on almost all changes to the scene tree the editor would
rebuild the entire widget, causing a large number of deallocations an
allocations. We now carefully manipulate the Tree widget in-situ saving
a large number of these allocations.
In order to know what Nodes need to be updated we add a
editor_state_changed signal to Node, this is a TOOLS_ENABLED,
editor-only signal fired when changes to Node happen that are relevant
to editor state.
We also now make sure that when nodes are moved/renamed we don't check
expensive properties that cannot contain NodePaths. This saves a lot of
time when SceneTreeDock renames a node in a scene with a lot of
MeshInstances. This makes renaming nodes go from ~27 seconds to ~2
seconds on large scenes.
SceneTreeEditor instances will now also not do all of the potentially
expensive update work if they are invisible. This behavior is turned off
by default so it won't affect existing users. This change allows the
editor to only update SceneTreeEditors that actually in view. In
practice this means that for most changes instead of updating 6
SceneTreeEditors we only update 1 instantly, and the others only when
they become visible.
There is definitely more that could be done, but this is already a
massive improvement. In complex scenes we see an improvement of 10x,
things that used to take ~30 seconds now only take 2.
This fixes#83460
I want to thank KoBeWi, TokisanGames, a-johnston, aniel080400 for
their tireless testing. And AeioMuch for their testing and providing a
fix for the hover issue.
This uniform was already defined for other uses previously.
`textureSize()` is known to be slow on mobile platforms due to how
the drivers implement it there, so it's best avoided.
This prevents the antialiasing feather from becoming too wide
at viewport sizes higher than the default, which can lead to blurry visuals.
This is adjusted to account for the 2D scale factor returned on the root
Window, so it takes both the `canvas_items` scaling and
`content_scale_factor` into account.
