Skip to content

Commit f9c1e9e

Browse files
adriangbclaude
adriangb
and
claude
authored
fix: prevent unparser stack overflow on deeply nested expressions (#23058)
## Which issue does this PR close? - Closes #23056. ## Rationale for this change `Unparser::expr_to_sql` overflows the OS stack on deeply nested expressions, and — surprisingly — it does so **even with the `recursive_protection` feature enabled** (the default for the `datafusion` crate). While investigating I found the root cause is subtler than "a function is missing `#[recursive]`": `expr_to_sql_inner` **is** already annotated and **is** re-entered on the function-argument / dialect-override paths, so the trampoline does fire. The real problem is that the `recursive` crate's default red zone is **128 KiB**, but a single `expr_to_sql_inner` stack frame in debug builds is **~130 KiB** — *larger than the red zone*. `stacker::maybe_grow` only grows when `remaining < red_zone`, so a frame can pass the check and then overflow before the next checkpoint. The unparser installed **no `StackGuard`**, unlike the planner, which already does exactly this in `query.rs`: ```rust // query.rs let _guard = StackGuard::new(256 * 1024); ``` with a comment noting the same debug-frame-size issue (PR #13310). Secondary leaks: several internal recursion sites recursed through the public, un-annotated `expr_to_sql` (function arguments, `make_array`, `array_element`, `named_struct`, `map`, and the `array_has` / `date_part` dialect overrides), and `remove_unnecessary_nesting` (pretty mode) was not annotated at all — it would be the next overflow site once the first is fixed. ## What changes are included in this PR? The recursive design is kept (an iterative rewrite of the tree-building unparser would be a large, high-risk change for no benefit over the trampoline). The fix mirrors the planner: - Install `StackGuard::new(256 * 1024)` once at the top-level `expr_to_sql`. - Add an annotated `pub(crate) expr_to_sql_with_nesting` recursion core that all internal recursion sites call. This keeps pretty-mode behavior identical (each level still runs `remove_unnecessary_nesting`) while making every nesting level a stack-growth checkpoint. - Annotate `remove_unnecessary_nesting` with `#[recursive]`. The PR is split into two commits for review: the failing regression test first, then the fix. ## Are these changes tested? Yes. `test_deeply_nested_expr_does_not_overflow_stack` (gated on `recursive_protection`) unparses a depth-2000 `array_has` chain on `PostgreSqlDialect` and a depth-2000 binary chain on a 2 MiB thread. It aborts the process on `main` and passes with the fix. I verified empirically that with the default 128 KiB red zone the test still aborts, and that 256 KiB carries both paths past depth 5000 on a realistic thread. Note: the guarantee holds on realistically-sized thread stacks (≥1–2 MiB). The 512 KiB-stack figure in the issue's repro is too small to bootstrap the heavier `array_has` override path even after the fix, but that is the same memory floor the planner side has and is not a configuration used by DataFusion in practice. ## Are there any user-facing changes? No public API changes. Deeply nested expressions that previously aborted the process now unparse successfully when `recursive_protection` is enabled. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01QyHc4z88pC8nQLpFx7z2vM --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
1 parent 073e9ed commit f9c1e9e

3 files changed

Lines changed: 103 additions & 11 deletions

File tree

‎datafusion/sql/src/unparser/dialect.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -434,9 +434,11 @@ impl PostgreSqlDialect {
434434
};
435435

436436
Ok(Some(ast::Expr::AnyOp {
437-
left: Box::new(unparser.expr_to_sql(needle)?),
437+
// Recurse through the annotated entry point so the stack-growth
438+
// protection engages on nested arguments; see issue #23056.
439+
left: Box::new(unparser.expr_to_sql_with_nesting(needle)?),
438440
compare_op: BinaryOperator::Eq,
439-
right: Box::new(unparser.expr_to_sql(haystack)?),
441+
right: Box::new(unparser.expr_to_sql_with_nesting(haystack)?),
440442
is_some: false,
441443
}))
442444
}

‎datafusion/sql/src/unparser/expr.rs‎

Lines changed: 97 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ use std::vec;
3131

3232
use super::Unparser;
3333
use super::dialect::{DistinctFromStyle, IntervalStyle};
34+
use crate::stack::StackGuard;
3435
use arrow::array::{
3536
ArrayRef, Date32Array, Date64Array, PrimitiveArray,
3637
types::{
@@ -94,6 +95,26 @@ const IS: &BinaryOperator = &BinaryOperator::BitwiseAnd;
9495

9596
impl Unparser<'_> {
9697
pub fn expr_to_sql(&self, expr: &Expr) -> Result<ast::Expr> {
98+
// Unparsing recurses once per nesting level. The function-argument and
99+
// dialect scalar-function-override paths cost more per level than the
100+
// default `recursive` red zone, so without raising the minimum stack
101+
// size the stack-growing trampoline engages too late and the OS stack
102+
// overflows on deeply nested expressions (issue #23056). The size
103+
// mirrors the planner's `StackGuard` usage in `query.rs`.
104+
let _guard = StackGuard::new(256 * 1024);
105+
self.expr_to_sql_with_nesting(expr)
106+
}
107+
108+
/// Recursive entry point shared by the public [`Self::expr_to_sql`] and the
109+
/// internal recursion sites (scalar-function arguments, arrays, maps, and
110+
/// dialect scalar-function overrides).
111+
///
112+
/// This carries the `recursive` annotation so every nesting level becomes a
113+
/// stack-growth checkpoint. Internal recursion must call this rather than
114+
/// the public [`Self::expr_to_sql`]: the public entry point is not
115+
/// annotated and would re-install the [`StackGuard`] on every level.
116+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
117+
pub(crate) fn expr_to_sql_with_nesting(&self, expr: &Expr) -> Result<ast::Expr> {
97118
let mut root_expr = self.expr_to_sql_inner(expr)?;
98119
if self.pretty {
99120
root_expr = self.remove_unnecessary_nesting(root_expr, LOWEST, LOWEST);
@@ -681,7 +702,7 @@ impl Unparser<'_> {
681702
fn make_array_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
682703
let args = args
683704
.iter()
684-
.map(|e| self.expr_to_sql(e))
705+
.map(|e| self.expr_to_sql_with_nesting(e))
685706
.collect::<Result<Vec<_>>>()?;
686707
Ok(ast::Expr::Array(Array {
687708
elem: args,
@@ -708,8 +729,8 @@ impl Unparser<'_> {
708729
2,
709730
"array_element must have exactly 2 arguments"
710731
);
711-
let array = self.expr_to_sql(&args[0])?;
712-
let index = self.expr_to_sql(&args[1])?;
732+
let array = self.expr_to_sql_with_nesting(&args[0])?;
733+
let index = self.expr_to_sql_with_nesting(&args[1])?;
713734
Ok(ast::Expr::CompoundFieldAccess {
714735
root: Box::new(array),
715736
access_chain: vec![ast::AccessExpr::Subscript(Subscript::Index { index })],
@@ -732,7 +753,7 @@ impl Unparser<'_> {
732753

733754
Ok(ast::DictionaryField {
734755
key,
735-
value: Box::new(self.expr_to_sql(&chunk[1])?),
756+
value: Box::new(self.expr_to_sql_with_nesting(&chunk[1])?),
736757
})
737758
})
738759
.collect::<Result<Vec<_>>>()?;
@@ -802,15 +823,17 @@ impl Unparser<'_> {
802823
fn map_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
803824
assert_eq_or_internal_err!(args.len(), 2, "map must have exactly 2 arguments");
804825

805-
let ast::Expr::Array(Array { elem: keys, .. }) = self.expr_to_sql(&args[0])?
826+
let ast::Expr::Array(Array { elem: keys, .. }) =
827+
self.expr_to_sql_with_nesting(&args[0])?
806828
else {
807829
return internal_err!(
808830
"map expects first argument to be an array, but received: {:?}",
809831
&args[0]
810832
);
811833
};
812834

813-
let ast::Expr::Array(Array { elem: values, .. }) = self.expr_to_sql(&args[1])?
835+
let ast::Expr::Array(Array { elem: values, .. }) =
836+
self.expr_to_sql_with_nesting(&args[1])?
814837
else {
815838
return internal_err!(
816839
"map expects second argument to be an array, but received: {:?}",
@@ -944,7 +967,7 @@ impl Unparser<'_> {
944967
) {
945968
Ok(ast::FunctionArg::Unnamed(ast::FunctionArgExpr::Wildcard))
946969
} else {
947-
self.expr_to_sql(e)
970+
self.expr_to_sql_with_nesting(e)
948971
.map(|e| ast::FunctionArg::Unnamed(ast::FunctionArgExpr::Expr(e)))
949972
}
950973
})
@@ -989,6 +1012,7 @@ impl Unparser<'_> {
9891012
///
9901013
/// Also note that when fetching the precedence of a nested expression, we ignore other nested
9911014
/// expressions, so precedence of expr `(a * (b + c))` equals `*` and not `+`.
1015+
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
9921016
fn remove_unnecessary_nesting(
9931017
&self,
9941018
expr: ast::Expr,
@@ -3307,6 +3331,72 @@ mod tests {
33073331
Ok(())
33083332
}
33093333

3334+
/// Regression test for https://github.com/apache/datafusion/issues/23056
3335+
///
3336+
/// Deeply-nested expressions whose unparse path routes through scalar
3337+
/// function arguments and dialect scalar-function overrides used to
3338+
/// overflow the OS stack even with `recursive_protection` enabled,
3339+
/// because the per-level stack cost of those paths exceeds the default
3340+
/// `recursive` red zone and the unparser installed no [`StackGuard`].
3341+
///
3342+
/// This test only asserts the protected behavior, so it is gated on the
3343+
/// `recursive_protection` feature. Without that feature the unparser is
3344+
/// not stack-safe by design and a deep enough expression will overflow.
3345+
#[cfg(feature = "recursive_protection")]
3346+
#[test]
3347+
fn test_deeply_nested_expr_does_not_overflow_stack() {
3348+
// Far deeper than the ~60 levels that overflow without protection, but
3349+
// bounded so the trampoline's heap stacks stay reasonable in debug.
3350+
const DEPTH: usize = 2_000;
3351+
3352+
// Run on an explicit, realistically-sized thread stack. The work is
3353+
// performed on a spawned thread so an overflow (in the unfixed code)
3354+
// aborts the process and fails the test deterministically rather than
3355+
// depending on the harness thread's stack size.
3356+
let handle = std::thread::Builder::new()
3357+
.stack_size(2 * 1024 * 1024)
3358+
.spawn(|| {
3359+
// 1. Linear chain through a dialect scalar-function override:
3360+
// array_has(array_has(... array_has(col, 'x') ...), 'x').
3361+
// PostgreSqlDialect unparses array_has via array_has_to_sql_any,
3362+
// which recurses back into the unparser for each argument.
3363+
let mut nested_fn: Expr = col("c");
3364+
for _ in 0..DEPTH {
3365+
nested_fn = array_has(nested_fn, lit("x"));
3366+
}
3367+
let pg = PostgreSqlDialect {};
3368+
Unparser::new(&pg)
3369+
.expr_to_sql(&nested_fn)
3370+
.expect("deeply nested scalar function should unparse");
3371+
3372+
// 2. Linear chain of plain binary operators, exercising the
3373+
// inner -> inner recursion on the default dialect.
3374+
let mut nested_binary: Expr = col("c");
3375+
for _ in 0..DEPTH {
3376+
nested_binary = nested_binary + lit(1);
3377+
}
3378+
Unparser::default()
3379+
.expr_to_sql(&nested_binary)
3380+
.expect("deeply nested binary expression should unparse");
3381+
3382+
// 3. Same binary chain in pretty mode. Pretty mode runs
3383+
// `remove_unnecessary_nesting` at every level, which recurses
3384+
// alongside the unparse itself; this locks down that second
3385+
// recursion site fixed by this PR.
3386+
Unparser::default()
3387+
.with_pretty(true)
3388+
.expr_to_sql(&nested_binary)
3389+
.expect(
3390+
"deeply nested binary expression should unparse in pretty mode",
3391+
);
3392+
})
3393+
.unwrap();
3394+
3395+
// If the unparser overflows, the process aborts and this join is never
3396+
// reached; otherwise the spawned thread returns cleanly.
3397+
handle.join().expect("unparsing thread should not panic");
3398+
}
3399+
33103400
#[test]
33113401
fn test_window_func_support_window_frame() -> Result<()> {
33123402
let default_dialect: Arc<dyn Dialect> =

‎datafusion/sql/src/unparser/utils.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,7 @@ pub(crate) fn date_part_to_sql(
448448
) -> Result<Option<ast::Expr>> {
449449
match (style, date_part_args.len()) {
450450
(DateFieldExtractStyle::Extract, 2) => {
451-
let date_expr = unparser.expr_to_sql(&date_part_args[1])?;
451+
let date_expr = unparser.expr_to_sql_with_nesting(&date_part_args[1])?;
452452
if let Expr::Literal(ScalarValue::Utf8(Some(field)), _) = &date_part_args[0] {
453453
let field = match field.to_lowercase().as_str() {
454454
"year" => ast::DateTimeField::Year,
@@ -468,7 +468,7 @@ pub(crate) fn date_part_to_sql(
468468
}
469469
}
470470
(DateFieldExtractStyle::Strftime, 2) => {
471-
let column = unparser.expr_to_sql(&date_part_args[1])?;
471+
let column = unparser.expr_to_sql_with_nesting(&date_part_args[1])?;
472472

473473
if let Expr::Literal(ScalarValue::Utf8(Some(field)), _) = &date_part_args[0] {
474474
let field = match field.to_lowercase().as_str() {

0 commit comments

Comments
 (0)