On 09.08.2020 23:10, Daniel Sahlberg wrote:
I have investigated further and I think I have found the issue. A patch
is attached, basically changing
const String::Contents key(String(m_env, jkey));
to
const String str(m_env, jkey);
const String::Contents key(str);
in ImmutableMap.for_each.
If I understand things correctly (admittedly I'm not an expert in C++),
the lifetime of the String object is just the execution of the
constructor of the Contents class. But the Contents class saves a
reference to the String object in a member variable. When the Contents
object is destroyed at the end of the function, it references the
already previously destroyed String object.
The patch looks good to me.
Here's what happens here:
1) A temporary 'String' is constructed on stack.
2) 'String::Contents' is constructed on stack, remembering address of
'String' passed as argument.
3) Temporary 'String' goes out of scope
It doesn't have a destructor, so nothing happens.
4) Stack region occupied by 'String' becomes free to use.
5) If compiler is smart enough, it can now reuse this stack region for
other variables, changing the memory values.
6) If compiler is lazy, it will not reuse the stack region, so the
values there will be intact, as if the patch was applied.
7) Depending on (5)(6) and how stack region was used, it may crash.
So it is indeed a mistake, but it often goes unnoticed.
