Stack overflow for function call in module scope not rejected

[bbannier]

The following code aborts with a stack overflow error from the OS.

module hello;

function foo(n: uint64) {
    foo(n);
}

foo(134);

For some parsing code we inject additional code into function bodies to measure the stack size, and perform a controlled abort if the stack grows too large. We should do the same for this code.

[bbannier]

Depending on the compiler this might create never terminating code (seen e.g., with GCC) or create a stack overflow like I saw with macOS's Clang. The runtime behavior might also change if the user code was tail-recursive (in which case we'd emit C++ code which could be tail-recursive as well). I am unsure whether all these cases can be caught with checking the stack size.

[rsmmr]

Actually we do insert that here as well:

static void __hlt::hello::foo(const ::hilti::rt::integer::safe<uint64_t>& n) {
    ::hilti::rt::detail::checkStack();
    foo(n);
}

checkStack() checks on every 2nd call if it still has at least 20K available on the stack, which seems should catch this.

That said, if I run this, I just hangs and doesn't terminate/crash for me.

[rsmmr]

Depending on the compiler this might create never terminating code

Ack, that's what I see.

I am unsure whether all these cases can be caught with checking the stack size.

I'd think yes, because the compiler can't change semantics of the code. I'd think that before every iteration it must continue to call check stack.

[awelzel]

I am unsure whether all these cases can be caught with checking the stack size.

I'd think yes, because the compiler can't change semantics of the code. I'd think that before every iteration it must continue to call check stack.

But with tail call optimization the stack won't grow anymore, maybe -fno-optimize-sibling-calls for user code assuming it doesn't hurt performance too much?

[rsmmr]

But with tail call optimization the stack won't grow anymore

But that's ok, isn't it? If the stack doesn't grow, we won't get a stack overflow either, so the test case just turns into an infinite loop as one would expect it to do in the first place.

[awelzel]

But that's ok, isn't it?

It might be if you say so :-)

I was thinking about it as a safety feature, similar to RecursionErrors / recursion limit in Python. If one ends up writing such a construct by mistake in a parser, a detected stack overflow could (would?) trigger an analyzer violation on the Zeek side and remove the parser. An endless loop, however, would have Zeek stuck and not make any progress at all.

[rsmmr]

I was thinking about it as a safety feature

Agree that, in principle, that would be useful. However, while I think we could implement that, it would come with a performance cost as we'd need to track recursion depth manually somehow (probably through an extra function parameter); plus, there are other ways to create infinite loops that we aren't able to detect either, so it'd just cover one specific case.

That all said, I still don't see how this code can trigger a OS stack overflow (and can't reproduce it).

[bbannier]

That all said, I still don't see how this code can trigger a OS stack overflow (and can't reproduce it).

I am not 100% sure I saw a stackoverflow either, just a segfault at frame depth 261949. For that I ran above snippet in debug mode in our zeek-7.1 Docker container zeek/zeek:7.1 which is configured to use Debian's gcc-12.2.0-14 for compilation:

$ spicyc -dj hello.spicy
/tmp/hello_a21e12ec74f02dba-d4605b34fcb421b1.cc: In function ‘void __hlt::hello::foo(hilti::rt::integer::safe<long unsigned int>&)’:
/tmp/hello_a21e12ec74f02dba-d4605b34fcb421b1.cc:26:13: warning: infinite recursion detected [-Winfinite-recursion]
   26 | static void __hlt::hello::foo(const ::hilti::rt::integer::safe<uint64_t>& n) {
      |             ^~~~~
/tmp/hello_a21e12ec74f02dba-d4605b34fcb421b1.cc:29:8: note: recursive call
   29 |     foo(n);
      |     ~~~^~~
Segmentation fault (core dumped)

[rsmmr]

Looking at code with Benjamin, we might have a lead: not clear of the checkStack() catches stack overflows during initialization. I'll check.