JS_ThrowStackOverflow may overflow stack itself

[Bargest]

Hello,

QuickJS supports stack limiting with JS_SetMaxStackSize(), and many operations check remaining stack size before alloca() or calling functions. These checks call JS_ThrowStackOverflow(), which should throw corresponding exception. Obviously this exception has a stacktrace.
But the problem is that build_backtrace() function consumes huge amount of stack itself. For example, just this line implies x64 JSCallSiteData structs, each of them is x3 JSValue (+ 9 bytes + align), and each JSValue may be up to 16 bytes on x64 machine (depending on flags). This gives us 64 * aligned(3 * 16 + 9) = 3840 bytes. The full call chain performed to fill backtrace also includes other stack-heavy things, like char buf[256]. Since the exception itself was thrown because we've already reached stack limit, there is a risk of "physically" overflowing the stack if the limit was configured somewhat near the real stack size.

For now the only workaround is to set limit way lower than real stack size, but the needed spare space seems to be unpredictable.
Proposed solutions:

1. Calculate and enforce maximum stack consumption for JS_ThrowStackOverflow() and specify it in documentation;
2. Make build_backtrace() and other corresponding functions less stack-heavy, moving every possible local value on (pre-allocated?) heap buffer;
3. Add some stack checks to throwing-related functions and fallback to skipping backtrace if building stacktrace is impossible.

[bnoordhuis]

What I think we could do is split build_backtrace in two: one that invokes Error.prepareStackTrace and one that doesn't, then ensure we only use the latter when out of stack space.

That function has ballooned in size and is becoming unwieldy, and nothing good can come of calling into JS code under stack overflow conditions anyway.

Calculate and enforce maximum stack consumption for JS_ThrowStackOverflow() and specify it in documentation

I don't think we could commit to anything more specific than a not-super-helpful "reserve some headroom" because there are just too many factors.

You wouldn't in general expect quickjs to run in just 2-3 kB of stack space. This isn't really different.

Make build_backtrace() and other corresponding functions less stack-heavy, moving every possible local value on (pre-allocated?) heap buffer

I don't really like that because it increases the footprint of JSContext or JSRuntime quite a bit while seldomly used. I'm also not 100% sure it'd actually work because of recursion.

Add some stack checks to throwing-related functions and fallback to skipping backtrace if building stacktrace is impossible.

Definitely possible but definitely bad DX, IMO.

[Bargest]

@bnoordhuis, thanks for your reply.

Yes, your suggestion seems to be a good way to address this issue.

You wouldn't in general expect quickjs to run in just 2-3 kB of stack space.

In my toy embed I have ~15kb of stack and QuickJS works prefect. Well, until I try to compile smth like (((((((((((((((((((((((((1))))))))))))))))))))))))) which will immediately overload stack on js_parse_expr_binary recursions :D.

To prevent crashes I ended up reserving 512 bytes of stack and patching JS_ThrowStackOverflow() to set in_out_of_memory flag which disables backtrace for all stack overflow exceptions. It's a dirty hack and I lose reason of stack overflows, but it's better than nothing.

[chqrlie]

@Bargest stay tuned, we might have a better suited version for your purpose sometime soon. Do you use any features not available in Javascript 5 such as destructuring, let, const, async, generators... ?