From 8899a2aba92c4a17f422172e7c9dd0e383eefa39 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 24 Mar 2004 22:40:29 +0000 Subject: [PATCH] Replace max_expr_depth parameter with a max_stack_depth parameter that is measured in kilobytes and checked against actual physical execution stack depth, as per my proposal of 30-Dec. This gives us a fairly bulletproof defense against crashing due to runaway recursive functions. --- doc/src/sgml/runtime.sgml | 34 +++++---- src/backend/executor/execQual.c | 17 ++++- src/backend/optimizer/util/clauses.c | 10 ++- src/backend/parser/parse_expr.c | 36 +-------- src/backend/parser/parser.c | 3 +- src/backend/tcop/postgres.c | 75 ++++++++++++++++++- src/backend/utils/misc/guc.c | 21 +++--- src/backend/utils/misc/postgresql.conf.sample | 5 +- src/bin/psql/tab-complete.c | 4 +- src/include/miscadmin.h | 13 +++- src/include/parser/parse_expr.h | 4 +- src/include/pg_config_manual.h | 7 +- src/include/tcop/tcopprot.h | 7 +- 13 files changed, 157 insertions(+), 79 deletions(-) diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml index da6d6a5fba..93902043e3 100644 --- a/doc/src/sgml/runtime.sgml +++ b/doc/src/sgml/runtime.sgml @@ -1,5 +1,5 @@ @@ -889,6 +889,26 @@ SET ENABLE_SEQSCAN TO OFF; + + max_stack_depth (integer) + + + Specifies the maximum safe depth of the server's execution stack. + The ideal setting for this parameter is the actual stack size limit + enforced by the kernel (as set by ulimit -s or local + equivalent), less a safety margin of a megabyte or so. The safety + margin is needed because the stack depth is not checked in every + routine in the server, but only in key potentially-recursive routines + such as expression evaluation. Setting the parameter higher than + the actual kernel limit will mean that a runaway recursive function + can crash an individual backend process. The default setting is + 2048 KB (two megabytes), which is conservatively small and unlikely + to risk crashes. However, it may be too small to allow execution + of complex functions. + + + + @@ -2573,18 +2593,6 @@ dynamic_library_path = '/usr/local/lib/postgresql:/home/my_project/lib:$libdir' - - max_expr_depth (integer) - - - Sets the maximum expression nesting depth of the parser. The - default value of 10000 is high enough for any normal query, - but you can raise it if needed. (But if you raise it too high, - you run the risk of server crashes due to stack overflow.) - - - - diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c index fc16cccdda..4c6f95a9a6 100644 --- a/src/backend/executor/execQual.c +++ b/src/backend/executor/execQual.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.156 2004/03/17 20:48:42 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.157 2004/03/24 22:40:28 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -27,6 +27,11 @@ * trying to speed it up, the execution plan should be pre-processed * to facilitate attribute sharing between nodes wherever possible, * instead of doing needless copying. -cim 5/31/91 + * + * During expression evaluation, we check_stack_depth only in + * ExecMakeFunctionResult rather than at every single node. This + * is a compromise that trades off precision of the stack limit setting + * to gain speed. */ #include "postgres.h" @@ -840,6 +845,9 @@ ExecMakeFunctionResult(FuncExprState *fcache, bool hasSetArg; int i; + /* Guard against stack overflow due to overly complex expressions */ + check_stack_depth(); + /* * arguments is a list of expressions to evaluate before passing to * the function manager. We skip the evaluation if it was already @@ -1058,6 +1066,9 @@ ExecMakeFunctionResultNoSets(FuncExprState *fcache, FunctionCallInfoData fcinfo; int i; + /* Guard against stack overflow due to overly complex expressions */ + check_stack_depth(); + if (isDone) *isDone = ExprSingleResult; @@ -2503,6 +2514,10 @@ ExecInitExpr(Expr *node, PlanState *parent) if (node == NULL) return NULL; + + /* Guard against stack overflow due to overly complex expressions */ + check_stack_depth(); + switch (nodeTag(node)) { case T_Var: diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index c006cd49a1..b05f760ade 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.166 2004/03/21 22:29:11 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.167 2004/03/24 22:40:28 tgl Exp $ * * HISTORY * AUTHOR DATE MAJOR EVENT @@ -2347,6 +2347,10 @@ expression_tree_walker(Node *node, */ if (node == NULL) return false; + + /* Guard against stack overflow due to overly complex expressions */ + check_stack_depth(); + switch (nodeTag(node)) { case T_Var: @@ -2720,6 +2724,10 @@ expression_tree_mutator(Node *node, if (node == NULL) return NULL; + + /* Guard against stack overflow due to overly complex expressions */ + check_stack_depth(); + switch (nodeTag(node)) { case T_Var: diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 3881cc3953..83daae9b62 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/parser/parse_expr.c,v 1.166 2004/03/17 20:48:42 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/parser/parse_expr.c,v 1.167 2004/03/24 22:40:29 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -35,9 +35,6 @@ #include "utils/syscache.h" -int max_expr_depth = DEFAULT_MAX_EXPR_DEPTH; -static int expr_depth_counter = 0; - bool Transform_null_equals = false; static Node *typecast_expression(ParseState *pstate, Node *expr, @@ -47,19 +44,6 @@ static Node *transformIndirection(ParseState *pstate, Node *basenode, List *indirection); -/* - * Initialize for parsing a new query. - * - * We reset the expression depth counter here, in case it was left nonzero - * due to ereport()'ing out of the last parsing operation. - */ -void -parse_expr_init(void) -{ - expr_depth_counter = 0; -} - - /* * transformExpr - * Analyze and transform expressions. Type checking and type casting is @@ -92,20 +76,8 @@ transformExpr(ParseState *pstate, Node *expr) if (expr == NULL) return NULL; - /* - * Guard against an overly complex expression leading to coredump due - * to stack overflow here, or in later recursive routines that - * traverse expression trees. Note that this is very unlikely to - * happen except with pathological queries; but we don't want someone - * to be able to crash the backend quite that easily... - */ - if (++expr_depth_counter > max_expr_depth) - ereport(ERROR, - (errcode(ERRCODE_STATEMENT_TOO_COMPLEX), - errmsg("expression too complex"), - errdetail("Nesting depth exceeds maximum expression depth %d.", - max_expr_depth), - errhint("Increase the configuration parameter \"max_expr_depth\"."))); + /* Guard against stack overflow due to overly complex expressions */ + check_stack_depth(); switch (nodeTag(expr)) { @@ -938,8 +910,6 @@ transformExpr(ParseState *pstate, Node *expr) break; } - expr_depth_counter--; - return result; } diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c index 4e31e79972..54cc8a1c00 100644 --- a/src/backend/parser/parser.c +++ b/src/backend/parser/parser.c @@ -14,7 +14,7 @@ * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/parser/parser.c,v 1.60 2003/11/29 19:51:52 pgsql Exp $ + * $PostgreSQL: pgsql/src/backend/parser/parser.c,v 1.61 2004/03/24 22:40:29 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -50,7 +50,6 @@ raw_parser(const char *str) scanner_init(str); parser_init(); - parse_expr_init(); yyresult = yyparse(); diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 91442d49e6..ff0ac6aa64 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.396 2004/03/21 22:29:11 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.397 2004/03/24 22:40:29 tgl Exp $ * * NOTES * this is the "main" module of the postgres backend and @@ -92,11 +92,22 @@ bool Log_disconnections = false; */ int XfuncMode = 0; +/* GUC variable for maximum stack depth (measured in kilobytes) */ +int max_stack_depth = 2048; + + /* ---------------- * private variables * ---------------- */ +/* max_stack_depth converted to bytes for speed of checking */ +static int max_stack_depth_bytes = 2048*1024; + +/* stack base pointer (initialized by PostgresMain) */ +static char *stack_base_ptr = NULL; + + /* * Flag to mark SIGHUP. Whenever the main loop comes around it * will reread the configuration file. (Better than doing the @@ -1970,6 +1981,64 @@ ProcessInterrupts(void) } +/* + * check_stack_depth: check for excessively deep recursion + * + * This should be called someplace in any recursive routine that might possibly + * recurse deep enough to overflow the stack. Most Unixen treat stack + * overflow as an unrecoverable SIGSEGV, so we want to error out ourselves + * before hitting the hardware limit. Unfortunately we have no direct way + * to detect the hardware limit, so we have to rely on the admin to set a + * GUC variable for it ... + */ +void +check_stack_depth(void) +{ + char stack_top_loc; + int stack_depth; + + /* + * Compute distance from PostgresMain's local variables to my own + * + * Note: in theory stack_depth should be ptrdiff_t or some such, but + * since the whole point of this code is to bound the value to something + * much less than integer-sized, int should work fine. + */ + stack_depth = (int) (stack_base_ptr - &stack_top_loc); + /* + * Take abs value, since stacks grow up on some machines, down on others + */ + if (stack_depth < 0) + stack_depth = -stack_depth; + /* + * Trouble? + * + * The test on stack_base_ptr prevents us from erroring out if called + * during process setup or in a non-backend process. Logically it should + * be done first, but putting it here avoids wasting cycles during normal + * cases. + */ + if (stack_depth > max_stack_depth_bytes && + stack_base_ptr != NULL) + { + ereport(ERROR, + (errcode(ERRCODE_STATEMENT_TOO_COMPLEX), + errmsg("stack depth limit exceeded"), + errhint("Increase the configuration parameter \"max_stack_depth\"."))); + } +} + +/* GUC assign hook to update max_stack_depth_bytes from max_stack_depth */ +bool +assign_max_stack_depth(int newval, bool doit, GucSource source) +{ + /* Range check was already handled by guc.c */ + if (doit) + max_stack_depth_bytes = newval * 1024; + return true; +} + + static void usage(char *progname) { @@ -2030,6 +2099,7 @@ PostgresMain(int argc, char *argv[], const char *username) GucSource gucsource; char *tmp; int firstchar; + char stack_base; StringInfoData input_message; volatile bool send_rfq = true; @@ -2069,6 +2139,9 @@ PostgresMain(int argc, char *argv[], const char *username) SetProcessingMode(InitProcessing); + /* Set up reference point for stack depth checking */ + stack_base_ptr = &stack_base; + /* * Set default values for command-line options. */ diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 280977d60c..c14b428693 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -10,7 +10,7 @@ * Written by Peter Eisentraut . * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.192 2004/03/23 01:23:48 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.193 2004/03/24 22:40:29 tgl Exp $ * *-------------------------------------------------------------------- */ @@ -1023,6 +1023,15 @@ static struct config_int ConfigureNamesInt[] = 16384, 1024, INT_MAX / 1024, NULL, NULL }, + { + {"max_stack_depth", PGC_SUSET, RESOURCES_MEM, + gettext_noop("Sets the maximum stack depth, in kilobytes."), + NULL + }, + &max_stack_depth, + 2048, 100, INT_MAX / 1024, assign_max_stack_depth, NULL + }, + { {"vacuum_cost_page_hit", PGC_USERSET, RESOURCES, gettext_noop("Vacuum cost for a page found in the buffer cache."), @@ -1097,14 +1106,6 @@ static struct config_int ConfigureNamesInt[] = 0, 0, INT_MAX, NULL, NULL }, #endif - { - {"max_expr_depth", PGC_USERSET, CLIENT_CONN_OTHER, - gettext_noop("Sets the maximum expression nesting depth."), - NULL - }, - &max_expr_depth, - DEFAULT_MAX_EXPR_DEPTH, 10, INT_MAX, NULL, NULL - }, { {"statement_timeout", PGC_USERSET, CLIENT_CONN_STATEMENT, @@ -1246,7 +1247,7 @@ static struct config_int ConfigureNamesInt[] = }, { - {"debug_shared_buffers", PGC_POSTMASTER, RESOURCES_MEM, + {"debug_shared_buffers", PGC_POSTMASTER, STATS_MONITORING, gettext_noop("Interval to report shared buffer status in seconds"), NULL }, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ff12fbdf82..0001a9ffbb 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -58,7 +58,7 @@ #shared_buffers = 1000 # min 16, at least max_connections*2, 8KB each #work_mem = 1024 # min 64, size in KB #maintenance_work_mem = 16384 # min 1024, size in KB -#debug_shared_buffers = 0 # 0-600 seconds +#max_stack_depth = 2048 # min 100, size in KB #vacuum_cost_page_hit = 1 # 0-10000 credits #vacuum_cost_page_miss = 10 # 0-10000 credits @@ -204,6 +204,8 @@ #log_executor_stats = false #log_statement_stats = false +#debug_shared_buffers = 0 # 0-600 seconds + # - Query/Index Statistics Collector - #stats_start_collector = true @@ -243,7 +245,6 @@ #explain_pretty_print = true #dynamic_library_path = '$libdir' -#max_expr_depth = 10000 # min 10 #--------------------------------------------------------------------------- diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 8b60715821..ff9e8c499d 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -3,7 +3,7 @@ * * Copyright (c) 2000-2003, PostgreSQL Global Development Group * - * $PostgreSQL: pgsql/src/bin/psql/tab-complete.c,v 1.102 2004/03/23 01:23:48 tgl Exp $ + * $PostgreSQL: pgsql/src/bin/psql/tab-complete.c,v 1.103 2004/03/24 22:40:29 tgl Exp $ */ /*---------------------------------------------------------------------- @@ -535,11 +535,11 @@ psql_completion(char *text, int start, int end) "log_statement_stats", "maintenance_work_mem", "max_connections", - "max_expr_depth", "max_files_per_process", "max_fsm_pages", "max_fsm_relations", "max_locks_per_transaction", + "max_stack_depth", "password_encryption", "port", "random_page_cost", diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index f34ebb0986..4aba00bb71 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -1,18 +1,19 @@ /*------------------------------------------------------------------------- * * miscadmin.h - * this file contains general postgres administration and initialization + * This file contains general postgres administration and initialization * stuff that used to be spread out between the following files: * globals.h global variables * pdir.h directory path crud * pinit.h postgres initialization * pmod.h processing modes - * + * Over time, this has also become the preferred place for widely known + * resource-limitation stuff, such as work_mem and check_stack_depth(). * * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/miscadmin.h,v 1.154 2004/03/23 01:23:48 tgl Exp $ + * $PostgreSQL: pgsql/src/include/miscadmin.h,v 1.155 2004/03/24 22:40:29 tgl Exp $ * * NOTES * some of the information in this file should be moved to @@ -70,7 +71,7 @@ extern volatile bool ImmediateInterruptOK; extern volatile uint32 InterruptHoldoffCount; extern volatile uint32 CritSectionCount; -/* in postgres.c */ +/* in tcop/postgres.c */ extern void ProcessInterrupts(void); #ifndef WIN32 @@ -224,6 +225,10 @@ extern char *UnixSocketDir; extern char *ListenAddresses; +/* in tcop/postgres.c */ +extern void check_stack_depth(void); + + /***************************************************************************** * pdir.h -- * * POSTGRES directory path definitions. * diff --git a/src/include/parser/parse_expr.h b/src/include/parser/parse_expr.h index d5b87da1b7..a8be03a534 100644 --- a/src/include/parser/parse_expr.h +++ b/src/include/parser/parse_expr.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/parser/parse_expr.h,v 1.32 2003/11/29 22:41:09 pgsql Exp $ + * $PostgreSQL: pgsql/src/include/parser/parse_expr.h,v 1.33 2004/03/24 22:40:29 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -18,7 +18,6 @@ /* GUC parameters */ -extern int max_expr_depth; extern bool Transform_null_equals; @@ -26,6 +25,5 @@ extern Node *transformExpr(ParseState *pstate, Node *expr); extern Oid exprType(Node *expr); extern int32 exprTypmod(Node *expr); extern bool exprIsLengthCoercion(Node *expr, int32 *coercedTypmod); -extern void parse_expr_init(void); #endif /* PARSE_EXPR_H */ diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h index 6fcf38ec7b..87ddd1f196 100644 --- a/src/include/pg_config_manual.h +++ b/src/include/pg_config_manual.h @@ -6,7 +6,7 @@ * for developers. If you edit any of these, be sure to do a *full* * rebuild (and an initdb if noted). * - * $PostgreSQL: pgsql/src/include/pg_config_manual.h,v 1.11 2004/03/12 00:25:40 neilc Exp $ + * $PostgreSQL: pgsql/src/include/pg_config_manual.h,v 1.12 2004/03/24 22:40:29 tgl Exp $ *------------------------------------------------------------------------ */ @@ -112,11 +112,6 @@ */ #define MAXPGPATH 1024 -/* - * DEFAULT_MAX_EXPR_DEPTH: default value of max_expr_depth SET variable. - */ -#define DEFAULT_MAX_EXPR_DEPTH 10000 - /* * PG_SOMAXCONN: maximum accept-queue length limit passed to * listen(2). You'd think we should use SOMAXCONN from diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h index 20c84dd925..b2520b210e 100644 --- a/src/include/tcop/tcopprot.h +++ b/src/include/tcop/tcopprot.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/tcop/tcopprot.h,v 1.62 2004/03/15 15:56:27 momjian Exp $ + * $PostgreSQL: pgsql/src/include/tcop/tcopprot.h,v 1.63 2004/03/24 22:40:29 tgl Exp $ * * OLD COMMENTS * This file was created so that other c files could get the two @@ -23,6 +23,7 @@ #include "executor/execdesc.h" #include "tcop/dest.h" +#include "utils/guc.h" extern DLLIMPORT sigjmp_buf Warn_restart; @@ -32,6 +33,7 @@ extern CommandDest whereToSendOutput; extern bool log_hostname; extern DLLIMPORT const char *debug_query_string; extern char *rendezvous_name; +extern int max_stack_depth; #ifndef BOOTSTRAP_INCLUDE @@ -43,6 +45,9 @@ extern List *pg_analyze_and_rewrite(Node *parsetree, extern List *pg_rewrite_queries(List *querytree_list); extern Plan *pg_plan_query(Query *querytree); extern List *pg_plan_queries(List *querytrees, bool needSnapshot); + +extern bool assign_max_stack_depth(int newval, bool doit, GucSource source); + #endif /* BOOTSTRAP_INCLUDE */ extern void die(SIGNAL_ARGS);