Guard against native stack overflow in deep non-tail recursion

include/openscad_cpp_evaluator/evaluator.hpp

@@ -633,6 +633,47 @@ class Evaluator {
     Value evalUserFunctionCore(const std::string& name, const oscad::ASTNode& declNode, const oscad::Expression& bodyExpr,
                                 EvalContext& childCtx, const oscad::Position* callPos, int upvalueParent,
                                 F&& computeResult) {
+        // Guards against a genuinely non-tail-recursive user function --
+        // each logical call HERE is a real, unavoidable C++ recursive call
+        // (a trampolined tail call never re-enters this function at all,
+        // see evalFunctionBodyTrampoline/runCompiledFunctionTrampoline) --
+        // silently overflowing the native stack: a hard, unrecoverable
+        // process crash with no C++ exception to catch. Confirmed present
+        // (before this guard existed) for a plain, closure-free `function
+        // f(n) = n<=0 ? 0 : 1+f(n-1);`, compiled OR interpreted.
+        //
+        // kMaxUserCallDepth is a conservative, heuristic cap, NOT a
+        // precisely-calibrated one -- and specifically calibrated against
+        // the WORST measured platform, not the best. Two rounds of CI
+        // diagnostics (temporary test builds that recursed to increasing
+        // depths, printing success after each one so the CI log itself
+        // would show exactly where a crash happened) found: (1) this
+        // evaluator's own C++ recursion is safe into the THOUSANDS on an
+        // ordinary macOS/Linux desktop main-thread stack, but only into
+        // the low HUNDREDS on CI's windows-latest runner (smaller default
+        // thread stack and/or larger MSVC-compiled call frames than
+        // Clang's); (2) actually THROWING from that depth and unwinding
+        // back out through that many nested try/catch frames (this guard
+        // fires from inside evalUserFunctionCore, which every level of
+        // the recursion has its own try/catch in) costs meaningfully MORE
+        // native stack than simply being that deep without throwing --
+        // 300 survived on Windows without throwing, but throwing+
+        // unwinding through as few as 100 nested frames did not (80 was
+        // the last depth that survived). 50 leaves real margin below that
+        // 80 figure for whatever's already on the stack from this call's
+        // own caller, and for a smaller stack still (e.g. BelfrySCAD's own
+        // debug-session worker thread, plausibly smaller than any CI
+        // runner's main thread) that this evaluator has no way to probe
+        // for directly. A real OpenSCAD/BOSL2 recursive function is
+        // overwhelmingly tail-recursive in practice anyway (exactly why
+        // reduce()/accumulate()/while() and this evaluator's own
+        // trampolines exist) -- genuine non-tail recursion even
+        // approaching this depth is already the rare, likely-runaway case
+        // a controlled error serves far better than a crash.
+        static constexpr size_t kMaxUserCallDepth = 50;
+        if (callStack_.size() >= kMaxUserCallDepth) {
+            error("Recursion too deep while calling function '" + name + "'", declNode);
+        }
         std::optional<ProfileHandle> prof = profileEnter("function", name, callPos, &declNode.position());
         callStack_.push_back(CallStackFrame{CallStackFrame::Kind::Function, name, callPos, &declNode.position(), &declNode,
                                              nullptr, upvalueParent});

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build"
 
 [project]
 name = "openscad_cpp_evaluator"
-version = "0.9.1"
+version = "0.9.2"
 description = "C++ OpenSCAD evaluator with Python bindings"
 readme = "README.md"
 requires-python = ">=3.12"

tests/test_tail_calls.cpp

@@ -9,10 +9,15 @@
 // Every test here explicitly forces the interpreter path via ScopedVm(false)
 // (not the OSCAD_BYTECODE_VM env var, which is cached forever after its
 // first read within a test binary -- see Evaluator::
-// setBytecodeVmEnabledForTesting's own doc comment): the bytecode VM's own
-// call machinery (Phase B, not yet implemented as of this file) still does
-// genuine C++ recursion for every call, so a deep-recursion test run under
-// the default VM-on path would crash for the wrong reason.
+// setBytecodeVmEnabledForTesting's own doc comment). Both the interpreter
+// AND the bytecode VM's own call machinery (Phase B) do genuine C++
+// recursion for a NON-tail call (there is no other way to implement one --
+// see NonTailRecursionIsUnaffectedByTheTrampoline, below) -- a sufficiently
+// deep one used to silently overflow the native stack and crash the whole
+// process; evalUserFunctionCore's own depth guard (evaluator.hpp) now turns
+// that into a controlled EvalError well before that point, shared by both
+// paths, exercised by both compiled- and interpreted-path variants of the
+// same test at the bottom of this file.
 
 #include "openscad_cpp_evaluator/eval_error.hpp"
 #include "openscad_cpp_evaluator/evaluator.hpp"
@@ -267,3 +272,55 @@ TEST(TailCalls, DebugHookFiresPerHopInsideATailChain) {
     // single body-entry stop.
     EXPECT_EQ(bodyEntry, 501 + 500 + 1);
 }
+
+TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingInterpreted) {
+    ScopedVm vm(false);
+    // `1 + f(n-1)` is NOT a tail position (the addition happens after the
+    // recursive call returns) -- there is no trampoline for this, ever
+    // (see NonTailRecursionIsUnaffectedByTheTrampoline, above): every
+    // logical call is a real, unavoidable C++ recursive call through
+    // evalUserFunctionCore. Before its own depth guard existed, a script
+    // shaped exactly like this one reliably segfaulted the whole process
+    // (confirmed present in the wild -- a plain, closure-free
+    // FunctionDeclaration, nothing specific to closures/letrec/tail-call
+    // machinery at all). Interpreted-path variant; see the compiled-path
+    // one below. f(500) is comfortably past kMaxUserCallDepth (50, see its
+    // own doc comment, evaluator.hpp) -- the actual native recursion never
+    // gets anywhere near 500, since the guard fires at 50 regardless of
+    // what depth the script asks for.
+    Evaluator ev;
+    auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(500);");
+    auto scope = oscad::buildScopes(ast);
+    EvalContext ctx = EvalContext::makeRoot(scope.get());
+    try {
+        ev.resolveTree(ast, ctx);
+        FAIL() << "expected EvalError";
+    } catch (const EvalError& e) {
+        EXPECT_NE(std::string(e.what()).find("Recursion too deep"), std::string::npos);
+        EXPECT_NE(std::string(e.what()).find("'f'"), std::string::npos);
+    }
+}
+
+TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingCompiled) {
+    ScopedVm vm(true);
+    // Same script, same guard (evalUserFunctionCore is shared by both the
+    // interpreter and the bytecode VM's own compiled call path) -- the
+    // compiled path's own real per-call native stack usage is actually
+    // larger (more C++ frames between one logical call and the next, see
+    // runChunk/runCompiledFunctionFromBound/runCompiledFunctionFromBoundTrampoline),
+    // so this is the MORE exposed of the two paths, not a lesser check.
+    Evaluator ev;
+    auto ast = parseSrc("function f(n) = n <= 0 ? 0 : 1 + f(n - 1);\nresult = f(500);");
+    auto scope = oscad::buildScopes(ast);
+    EvalContext ctx = EvalContext::makeRoot(scope.get());
+    EXPECT_THROW(ev.resolveTree(ast, ctx), EvalError);
+}
+
+TEST(TailCalls, ShallowNonTailRecursionStillWorksUnderTheDepthGuard) {
+    ScopedVm vm(true);
+    // The guard must not fire for perfectly ordinary, shallow non-tail
+    // recursion -- the overwhelmingly common real-world shape (a handful
+    // to a few dozen levels, e.g. walking a small nested data structure).
+    const std::string script = "function fact(n) = n <= 1 ? 1 : n * fact(n - 1);\necho(fact(10));";
+    EXPECT_EQ(runCapturingEcho(script), "ECHO: 3.6288e+6");
+}