SIGSEGV due to freeing of uninitialized local string, during exception handling

[arnetheduck]

Description

type
  Rlp* = object
    bytes: seq[byte]
    position*: int

  RlpNodeType* = enum
    rlpBlob
    rlpList

  RlpError* = object of CatchableError
  MalformedRlpError* = object of RlpError
  UnsupportedRlpError* = object of RlpError
  RlpTypeMismatch* = object of RlpError

  RlpItem = tuple[payload: Slice[int], typ: RlpNodeType]


func raiseExpectedBlob() {.noreturn, noinline.} =
  raise (ref RlpTypeMismatch)(msg: "expected blob")

func toString(self: Rlp, item: RlpItem): string =
  if item.typ != rlpBlob:
    raiseExpectedBlob()

  result = newString(item.payload.len)

func rlpItem(input: openArray[byte], start = 0): RlpItem =
  raise (ref ValueError)()

func item(self: Rlp, position: int): RlpItem =
  rlpItem(self.bytes, position)

func item(self: Rlp): RlpItem =
  self.item(self.position)

func toString(self: Rlp): string =
  self.toString(self.item())

func inspectAux(self: var Rlp, depth: int, hexOutput: bool, output: var string) =
  let str = self.toString()
  output = str

func inspect(self: Rlp, indent = 0, hexOutput = true): string =
  var rlpCopy = self
  result = newStringOfCap(self.bytes.len)
  inspectAux(rlpCopy, indent, hexOutput, result)

try:
  var rlp = Rlp(bytes: @[byte 0xfe, 0x00])
  echo inspect(rlp)
except ValueError:
  discard

N_LIB_PRIVATE N_NIMCALL(NimStringV2, _ZN6testit8toStringEN6testit3RlpE)(tyObject_Rlp__O3LNAUKNVT3ZMko7NUivvA self_p0) {
	NimStringV2 result;
	tyTuple__egi9bX55oNSSbLfoIy5fT9ag T1_;
NIM_BOOL* nimErr_;
{nimErr_ = nimErrorFlag();
	T1_ = _ZN6testit4itemEN6testit3RlpE(self_p0);
	if (NIM_UNLIKELY(*nimErr_)) goto BeforeRet_;
	result = _ZN6testit8toStringEN6testit3RlpE5tupleI5SliceI3intEN6testit11RlpNodeTypeEE(self_p0, T1_);
	if (NIM_UNLIKELY(*nimErr_)) goto BeforeRet_;
	}BeforeRet_: ;
	return result;
}
N_LIB_PRIVATE N_NIMCALL(void, _ZN6testit10inspectAuxE3varIN6testit3RlpEE3int4bool3varI6stringE)(tyObject_Rlp__O3LNAUKNVT3ZMko7NUivvA* self_p0, NI depth_p1, NIM_BOOL hexOutput_p2, NimStringV2* output_p3) {
	NimStringV2 str;
NIM_BOOL* nimErr_;
{nimErr_ = nimErrorFlag();
	str.len = 0; str.p = NIM_NIL;
	str = _ZN6testit8toStringEN6testit3RlpE((*self_p0));
	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);
}
	}
	if (NIM_UNLIKELY(*nimErr_)) goto BeforeRet_;
	}BeforeRet_: ;
}

In the C code, this is quite visible: _ZN6testit8toStringEN6testit3RlpE returns an uninitialized result because _ZN6testit4itemEN6testit3RlpE sets the error flag, then _ZN6testit10inspectAuxE3varIN6testit3RlpEE3int4bool3varI6stringE assigns that uninitialized return value to str, then goes on to deallocateShared(str) which obviously fails due to the garbage it receives.

For the crash to happen, there has to be some garbage in the stack - an easy way to get garbage on the stack is to turn on stack tracing..

nim c -r --stacktrace:on --linedir:off "testit.nim"

Nim Version

/home/arnetheduck/src/Nim/bin/nim -version
Nim Compiler Version 2.1.1 [Linux: amd64]
Compiled at 2024-05-24
Copyright (c) 2006-2024 by Andreas Rumpf

Current Output

SIGSEGV: Illegal storage access. (Attempt to read from nil?)

Expected Output

No response

Possible Solution

No response

Additional Information

No response

[arnetheduck]

same on 2.0, refc is safe

[Araq]

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);

[arnetheduck]

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)

[arnetheduck]

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

[arnetheduck]

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

[arnetheduck]

oops wrong button

[Araq]

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

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

[Araq]

Ah, got it.

[Araq]

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

[arnetheduck]

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.

[ringabout]

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)

[Araq]

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.

[arnetheduck]

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)

[Araq]

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...

[arnetheduck]

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

[Araq]

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

[arnetheduck]

...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

[arnetheduck]

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

[Araq]

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

[arnetheduck]

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.

[Araq]

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