SIGSEGV due to freeing of uninitialized local string, during exception handling about nim HOT 21 CLOSED

Fixed by #23279

It checks the right side of result assignement to ensure it doesn't contain exceptions.

After the change:

==13653== 
==13653== HEAP SUMMARY:
==13653==     in use at exit: 0 bytes in 0 blocks
==13653==   total heap usage: 0 allocs, 0 frees, 0 bytes allocated
==13653== 
==13653== All heap blocks were freed -- no leaks are possible
==13653== 
==13653== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

from nim.

same on 2.0, refc is safe

from nim.

But it disarms the destructor via _ZN6system11eqwasMoved_E3varI6stringE((&str)) and does not perform the store to output either as *nimErr is true:

if (NIM_UNLIKELY(*nimErr_)) goto LA1_;
	_ZN6system7eqsink_E3varI6stringE6string((&(*output_p3)), str);
	_ZN6system11eqwasMoved_E3varI6stringE((&str));
	{
		LA1_:;
	}
	{
		if (str.p && !(str.p->cap & NIM_STRLIT_FLAG)) {
 deallocShared(str.p);

from nim.

But it disarms the destructor via

as far as I can read, deallocShared is still called with str.p which contains junk (ie in the debugger, str.p has the value 0x01 in my case)

from nim.

ah, in fact, it doesn't even reach deallocShared - it crashes when trying to access str.p->cap

from nim.

of course, it's a stack variable so it'll depend on conditions - that said, with --linetrace:on and -d:debug I can repro this 100% because I suspect it leaves exactly the right kind of junk in the otherwise uninitialized stack variable

from nim.

oops wrong button

from nim.

Well valgrind agrees with you, somewhat. I don't understand it yet though. :-)

Invalid free() / delete / delete[] / realloc()

from nim.

Ah, got it.

from nim.

The bug is in allPathsAsgnResult which is still not smart enough. (False positive.)

from nim.

The bug is in allPathsAsgnResult which is still not smart enough. (False positive.)

you mean, in _ZN6testit8toStringEN6testit3RlpE, where the unintialized result is returned?

to me, it also seems like a pretty serious issue that it reads str at all when the error flag is set - if I read the C code, it seems to me that if (NIM_UNLIKELY(*nimErr_)) goto LA1_; should jump to a location after any str access, since in the case of an exception, str does not yet exist in the live set of variables.

from nim.

to me, it also seems like a pretty serious issue that it reads str at all when the error flag is set

Correct, it is transformed to:

var str
try:
  str = toString(self)
  `=sink`(output, str)
  `=wasMoved`(str)
finally:
  `=destroy`(str)

When it should be transformed into:

var str
str = toString(self)
try:
  `=sink`(output, str)
  `=wasMoved`(str)
finally:
  `=destroy`(str)

However, this is a much more invasive change with unclear side effects.

from nim.

Correct, it is transformed to:

so a separate issue needed for this one, or is it known? apart from any correctness issues (that I guess can be worked around by excessive zeroing), it'll make the code harder for the compiler to optimize, because of variable liveness extensions)

from nim.

It is a known issue and my current attempt at fixing it is to make ARC produce goto based exceptions directly rather than passing on this task to the codegen which only understands coarsely grained try statements. Then the codegen can also be relieved from the task of having to produce init code.

The downside is that the other backends need to replicate what the C backend does. So the days of "Nim produces C++'s and JavaScript's exception handling code" are numbered...

from nim.

So the days of "Nim produces C++'s and JavaScript's exception handling code" are numbered...

why? can't the information be transferred to the backends? this would also break the usage of dwarf EH in nlvm

from nim.

We'll see, but in general implementing the exception handing in a central place in the compiler instead of N times has obvious benefits.

from nim.

...and downsides ;) reasoning about that global error flag is harder for llvm than reasoning about its own EH model (which btw is important for windows when integrating with SEH) so that will have significant impact on the quality of optimizations

from nim.

can have the cookie and eat it if it's implemented as a separate, optional transformation step

from nim.

How so? The C codegen uses the transformation step and the other backends do what exactly? :-)

from nim.

How so? The C codegen uses the transformation step and the other backends do what exactly? :-)

Well, the way an exception-raising function would ideally be modelled is as having two return paths (akin to an if/else) - this would allow correctly selecting happy and error code paths before any other side effects are evaluated - the way llvm models it is having a special invoke instruction that has two jump targets for happy and unhappy case - if the arc implementation can model roughly that, we're fine for nlvm at least.

Incidentally, this is also the correct way to model the above str scenario so that the error checking is sequenced before assigning the return value to str.

What I'm looking for, in order to be able to generate efficient and consistently correct code is more or less:

var either = toString(self)
if either.isexception:
  return either
var str = move(either.value)
str = toString(self)
try:
  `=sink`(output, str)
  `=wasMoved`(str)
finally:
  `=destroy`(str)

notably, in this model there is no global error flag - the fact that every exception-involved function reads a global is a pessimization for the optimizer because it can no longer assume purity which disables a lot of potential.

from nim.

The global error flag is not modelled currently either, it's nothing to worry about. The problem with invoke is how to map it back to C++'s (JavaScript's) try statement.

But maybe this works:

x = invoke f(a, b, c) on failure: (cleanup(); return)

Can be translated to:

try:
  x = f(a, b, c)
except:
  cleanup();
  return

from nim.