Skip to content

Commit

Permalink
sql: avoid stack overflow on deeply nested ASTs
Browse files Browse the repository at this point in the history
In the SQL planner, use the same technique as the SQL parser for
automatically growing the stack when presented with a deeply nested AST.

The stack-checking code is generalized and moved to a new `ore::stack`
module.

Fix #9103.
  • Loading branch information
benesch committed Nov 16, 2021
1 parent 6a5d809 commit 1499132
Show file tree
Hide file tree
Showing 8 changed files with 179 additions and 64 deletions.
6 changes: 3 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/ore/Cargo.toml
Expand Up @@ -34,6 +34,7 @@ openssl = { version = "0.10.38", features = ["vendored"], optional = true }
pin-project = "1"
prometheus = { git = "https://github.com/MaterializeInc/rust-prometheus.git", default-features = false, optional = true }
smallvec = { version = "1.7.0", optional = true }
stacker = "0.1.14"
structopt = { version = "0.3.25", optional = true }
tokio = { version = "1.13.0", features = ["io-util", "net", "rt-multi-thread", "time"], optional = true }
tokio-openssl = { version = "0.6.3", optional = true }
Expand Down
1 change: 1 addition & 0 deletions src/ore/src/lib.rs
Expand Up @@ -49,6 +49,7 @@ pub mod panic;
pub mod result;
#[cfg(feature = "network")]
pub mod retry;
pub mod stack;
pub mod stats;
pub mod str;
#[cfg(feature = "test")]
Expand Down
112 changes: 112 additions & 0 deletions src/ore/src/stack.rs
@@ -0,0 +1,112 @@
// Copyright Materialize, Inc. and contributors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License in the LICENSE file at the
// root of this repository, or online at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//! Stack management utilities.

use std::cell::RefCell;
use std::error::Error;
use std::fmt;

// The red zone is the amount of stack space that must be available on
// the current stack in order to call `f` without allocating a new
// stack.
const STACK_RED_ZONE: usize = 32 << 10; // 32KiB

// The size of any freshly-allocated stacks. It was chosen to match the
// default stack size for threads in Rust.
const STACK_SIZE: usize = 2 << 20; // 2MiB

/// A trait for types which support recursion without stack overflow.
pub trait CheckedRecursion {
/// Extracts a reference to the recursion guard embedded within the type.
fn recursion_guard(&self) -> &RecursionGuard;

/// Checks whether it is safe to recur and calls `f` if so.
///
/// If the recursion limit `LIMIT` has been reached, returns a
/// `RecursionLimitError`. Otherwise, it will call `f`, possibly growing
/// the stack if necessary.
///
/// Calls to this function must be manually inserted at any point that
/// mutual recursion occurs.
fn checked_recur<F, T, E>(&self, f: F) -> Result<T, E>
where
F: FnOnce(&Self) -> Result<T, E>,
E: From<RecursionLimitError>,
{
self.recursion_guard().descend()?;
let out = stacker::maybe_grow(STACK_RED_ZONE, STACK_SIZE, || f(self));
self.recursion_guard().ascend();
out
}

/// Like [`CheckedRecursion::checked_recur`], but operates on a mutable
/// reference to `Self`.
fn checked_recur_mut<F, T, E>(&mut self, f: F) -> Result<T, E>
where
F: FnOnce(&mut Self) -> Result<T, E>,
E: From<RecursionLimitError>,
{
self.recursion_guard().descend()?;
let out = stacker::maybe_grow(STACK_RED_ZONE, STACK_SIZE, || f(self));
self.recursion_guard().ascend();
out
}
}

/// A guard which is embedded in a type that implements [`CheckedRecursion`]
/// to track the depth of recursion.
#[derive(Default, Debug, Clone)]
pub struct RecursionGuard {
depth: RefCell<usize>,
limit: usize,
}

impl RecursionGuard {
/// Constructs a new recursion guard with the specified recursion
/// limit.
pub fn with_limit(limit: usize) -> RecursionGuard {
RecursionGuard {
depth: RefCell::new(0),
limit,
}
}

fn descend(&self) -> Result<(), RecursionLimitError> {
let mut depth = self.depth.borrow_mut();
if *depth < self.limit {
*depth += 1;
Ok(())
} else {
Err(RecursionLimitError)
}
}

fn ascend(&self) {
*self.depth.borrow_mut() -= 1;
}
}

/// A [`RecursionGuard`]'s recursion limit was reached.
#[derive(Clone, Debug)]
pub struct RecursionLimitError;

impl fmt::Display for RecursionLimitError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.write_str("recursion limit exceeded")
}
}

impl Error for RecursionLimitError {}
1 change: 0 additions & 1 deletion src/sql-parser/Cargo.toml
Expand Up @@ -14,7 +14,6 @@ lazy_static = "1.4.0"
log = "0.4.13"
ore = { path = "../ore", default-features = false }
phf = { version = "0.10.0", features = ["uncased"] }
stacker = "0.1.14"
uncased = "0.9.6"

[dev-dependencies]
Expand Down
84 changes: 28 additions & 56 deletions src/sql-parser/src/parser.rs
Expand Up @@ -28,11 +28,17 @@ use log::{debug, warn};

use ore::collections::CollectionExt;
use ore::option::OptionExt;
use ore::stack::{CheckedRecursion, RecursionGuard, RecursionLimitError};

use crate::ast::*;
use crate::keywords::*;
use crate::lexer::{self, Token};

// NOTE(benesch): this recursion limit was chosen based on the maximum amount of
// nesting I've ever seen in a production SQL query (i.e., about a dozen) times
// a healthy factor to be conservative.
const RECURSION_LIMIT: usize = 128;

// Use `Parser::expected` instead, if possible
macro_rules! parser_err {
($parser:expr, $pos:expr, $MSG:expr) => {
Expand Down Expand Up @@ -116,6 +122,18 @@ impl fmt::Display for ParserError {

impl Error for ParserError {}

impl From<RecursionLimitError> for ParserError {
fn from(_: RecursionLimitError) -> ParserError {
ParserError {
pos: 0,
message: format!(
"statement exceeds nested expression limit of {}",
RECURSION_LIMIT
),
}
}
}

impl ParserError {
/// Constructs an error with the provided message at the provided position.
pub(crate) fn new<S>(pos: usize, message: S) -> ParserError
Expand All @@ -135,8 +153,7 @@ struct Parser<'a> {
tokens: Vec<(Token, usize)>,
/// The index of the first unprocessed token in `self.tokens`
index: usize,
/// Tracks recursion depth.
depth: usize,
recursion_guard: RecursionGuard,
}

/// Defines a number of precedence classes operators follow. Since this enum derives Ord, the
Expand Down Expand Up @@ -173,7 +190,7 @@ impl<'a> Parser<'a> {
sql,
tokens,
index: 0,
depth: 0,
recursion_guard: RecursionGuard::with_limit(RECURSION_LIMIT),
}
}

Expand Down Expand Up @@ -269,7 +286,7 @@ impl<'a> Parser<'a> {

/// Parse tokens until the precedence changes
fn parse_subexpr(&mut self, precedence: Precedence) -> Result<Expr<Raw>, ParserError> {
let expr = self.check_descent(|parser| parser.parse_prefix())?;
let expr = self.checked_recur_mut(|parser| parser.parse_prefix())?;
self.parse_subexpr_seeded(precedence, expr)
}

Expand All @@ -278,7 +295,7 @@ impl<'a> Parser<'a> {
precedence: Precedence,
mut expr: Expr<Raw>,
) -> Result<Expr<Raw>, ParserError> {
self.check_descent(|parser| {
self.checked_recur_mut(|parser| {
debug!("prefix: {:?}", expr);
loop {
let next_precedence = parser.get_next_precedence();
Expand Down Expand Up @@ -478,7 +495,7 @@ impl<'a> Parser<'a> {
// whether it belongs to the query or the expression.

// Parse to the closing parenthesis.
let either = parser.check_descent(parse)?;
let either = parser.checked_recur_mut(parse)?;
parser.expect_token(&Token::RParen)?;

// Decide if we need to associate any tokens after the closing
Expand Down Expand Up @@ -3247,7 +3264,7 @@ impl<'a> Parser<'a> {
/// by `ORDER BY`. Unlike some other parse_... methods, this one doesn't
/// expect the initial keyword to be already consumed
fn parse_query(&mut self) -> Result<Query<Raw>, ParserError> {
self.check_descent(|parser| {
self.checked_recur_mut(|parser| {
let ctes = if parser.parse_keyword(WITH) {
// TODO: optional RECURSIVE
parser.parse_comma_separated(Parser::parse_cte)?
Expand Down Expand Up @@ -4311,55 +4328,10 @@ impl<'a> Parser<'a> {
options,
}))
}
}

/// Checks whether it is safe to descend another layer of nesting in the
/// parse tree, and calls `f` if so.
///
/// The nature of a recursive descent parser, like this SQL parser, is that
/// deeply nested queries can easily cause a stack overflow, as each level
/// of nesting requires a new stack frame of a few dozen bytes, and those
/// bytes add up over time. That means that user input can trivially cause a
/// process crash, which does not make for a good user experience.
///
/// This method uses the [`stacker`] crate to automatically grow the stack
/// as necessary. It also enforces a hard but arbitrary limit on the maximum
/// depth, as it's good practice to have *some* limit. Real-world queries
/// tend not to have more than a dozen or so layers of nesting.
///
/// Calls to this function must be manually inserted in the parser at any
/// point that mutual recursion occurs; i.e., whenever parsing of a nested
/// expression or nested SQL query begins.
fn check_descent<F, R>(&mut self, f: F) -> Result<R, ParserError>
where
F: FnOnce(&mut Parser) -> Result<R, ParserError>,
{
// NOTE(benesch): this recursion limit was chosen based on the maximum
// amount of nesting I've ever seen in a production SQL query (i.e.,
// about a dozen) times a healthy factor to be conservative.
const RECURSION_LIMIT: usize = 128;

// The red zone is the amount of stack space that must be available on
// the current stack in order to call `f` without allocating a new
// stack.
const STACK_RED_ZONE: usize = 32 << 10; // 32KiB

// The size of any freshly-allocated stacks. It was chosen to match the
// default stack size for threads in Rust.
const STACK_SIZE: usize = 2 << 20; // 2MiB

if self.depth > RECURSION_LIMIT {
return parser_err!(
self,
self.peek_prev_pos(),
"query exceeds nested expression limit of {}",
RECURSION_LIMIT
);
}

self.depth += 1;
let out = stacker::maybe_grow(STACK_RED_ZONE, STACK_SIZE, || f(self));
self.depth -= 1;

out
impl CheckedRecursion for Parser<'_> {
fn recursion_guard(&self) -> &RecursionGuard {
&self.recursion_guard
}
}
8 changes: 4 additions & 4 deletions src/sql-parser/tests/testdata/recursion
Expand Up @@ -16,13 +16,13 @@
parse-statement
((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((SELECT 1))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))
----
error: query exceeds nested expression limit of 128
error: statement exceeds nested expression limit of 128
((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((SELECT 1))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))
^
^

parse-statement
SELECT ((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((1))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))
----
error: query exceeds nested expression limit of 128
error: statement exceeds nested expression limit of 128
SELECT ((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((1))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))
^
^

0 comments on commit 1499132

Please sign in to comment.