Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions ndc_bin/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,33 @@ impl TryFrom<Command> for Action {
}
}

/// Stack size for the worker thread that runs the interpreter.
///
/// The parser, analyser, compiler, and VM all walk the AST recursively, one
/// native frame per nesting level. The default main-thread stack is small
/// enough that deeply (but legitimately) nested programs overflow it and abort
/// the process. A generous stack keeps that from happening for any realistic
/// source. The reservation is virtual address space; only the pages actually
/// touched cost memory.
const INTERPRETER_STACK_SIZE: usize = 512 * 1024 * 1024;

fn main() -> anyhow::Result<()> {
// Run everything on a worker thread with a large explicit stack. The
// interpreter uses `Rc` and is not `Send`, but it is created, used, and
// dropped entirely inside this closure, so nothing crosses the boundary.
let worker = std::thread::Builder::new()
.name("ndc-interpreter".into())
.stack_size(INTERPRETER_STACK_SIZE)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject deep inputs instead of only enlarging the stack

For source nested beyond what this fixed 512 MiB stack can hold, this still reaches the same recursive parser/analyser/compiler walks and aborts the process; in the inspected diff ndc_parser is unchanged, so this line only raises the crash threshold rather than turning crafted deep input into a normal error. A file like print( + many more nested parentheses than the worker stack can handle + 1 will still terminate with a stack overflow, so the CLI remains vulnerable unless the parser enforces a depth limit before later phases walk the AST.

Useful? React with 👍 / 👎.

.spawn(run)
.expect("failed to spawn interpreter thread");

match worker.join() {
Ok(result) => result,
Err(panic) => std::panic::resume_unwind(panic),
}
}

fn run() -> anyhow::Result<()> {
let cli = Cli::parse();

let action: Action = cli.command.unwrap_or_default().try_into()?;
Expand Down
46 changes: 46 additions & 0 deletions ndc_bin/tests/deep_nesting.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
//! Regression test: deeply nested input must not crash the interpreter.
//!
//! Each nesting level costs a native stack frame in the parser and in every
//! later phase that walks the AST. On the default main-thread stack this used
//! to overflow and abort the process (SIGABRT) at a few hundred levels. `ndc`
//! now runs the interpreter on a worker thread with a large explicit stack, so
//! input far deeper than that evaluates normally. See
//! <https://github.com/timfennis/andy-cpp/issues/175>.

// This integration test links `ndc_bin`'s dependencies but drives the built
// binary instead of calling them, so silence `unused-crate-dependencies`.
#![allow(unused_crate_dependencies)]

use std::fs;
use std::process::Command;

#[test]
fn deeply_nested_input_does_not_overflow_the_stack() {
// Deep enough to overflow the default main-thread stack (which gave up
// around a few hundred levels), shallow enough to fit the worker thread's
// large stack with room to spare.
let depth = 2000;
let source = format!("print({}1{})", "(".repeat(depth), ")".repeat(depth));

let path = std::env::temp_dir().join(format!("ndc_deep_nesting_{}.ndc", std::process::id()));
fs::write(&path, &source).expect("write temp script");

let output = Command::new(env!("CARGO_BIN_EXE_ndc"))
.arg(&path)
.output()
.expect("run ndc");

let _ = fs::remove_file(&path);

assert!(
output.status.success(),
"ndc crashed on {depth}-deep nesting (status {:?}); a stack overflow would show up here.\nstderr:\n{}",
output.status,
String::from_utf8_lossy(&output.stderr),
);
assert_eq!(
String::from_utf8_lossy(&output.stdout).trim(),
"1",
"the nested expression should evaluate to 1",
);
}
Loading