From 7c09af480e3a5d2ba65d6ea19740a209a8637143 Mon Sep 17 00:00:00 2001 From: Ajin Cherian Date: Tue, 19 Oct 2021 07:14:00 -0400 Subject: [PATCH v33] PS - POC Row filter validation walker. This patch implements a parse-tree "walker" to validate a row-filter expression. Only very simple filer expression are permitted. Specifially: - no user-defined operators. - no functions. - no parse nodes of any kind other than Var, OpExpr, Const, BoolExpr This POC patch also disables (#if 0) all other row-filter errors which were thrown for EXPR_KIND_PUBLICATION_WHERE in the 0001 patch. Some regression tests are updated due to the modified validation error messages. --- src/backend/catalog/dependency.c | 68 +++++++++++++++++++++++++++++++ src/backend/catalog/pg_publication.c | 14 +++++-- src/backend/parser/parse_agg.c | 5 ++- src/backend/parser/parse_expr.c | 6 ++- src/backend/parser/parse_func.c | 3 ++ src/backend/parser/parse_oper.c | 2 + src/include/catalog/dependency.h | 2 +- src/test/regress/expected/publication.out | 17 +++++--- src/test/regress/sql/publication.sql | 2 + 9 files changed, 107 insertions(+), 12 deletions(-) diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index e81f093..405b3cd 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -132,6 +132,12 @@ typedef struct int subflags; /* flags to pass down when recursing to obj */ } ObjectAddressAndFlags; +/* for rowfilter_walker */ +typedef struct +{ + char *relname; +} rf_context; + /* for find_expr_references_walker */ typedef struct { @@ -1554,6 +1560,68 @@ ReleaseDeletionLock(const ObjectAddress *object) } /* + * Walker checks that the row filter extression is legal. Allow only simple or + * or compound expressions like: + * + * "(Var Op Const)" or + * "(Var Op Const) Bool (Var Op Const)" + * + * Nothing more complicated is permitted. Specifically, no functions of any kind + * and no user-defined operators. + */ +static bool +rowfilter_walker(Node *node, rf_context *context) +{ + char *forbidden = NULL; + + if (node == NULL) + return false; + + if (IsA(node, Var) || IsA(node, Const) || IsA(node, BoolExpr)) + { + /* OK */ + } + else if (IsA(node, OpExpr)) + { + /* OK, except user-defined operators are not allowed. */ + if (((OpExpr *)node)->opno >= FirstNormalObjectId) + forbidden = _("user-defined operators are not allowed"); + } + else if (IsA(node, FuncExpr)) + { + forbidden = _("function calls are not allowed"); + } + else + { + elog(DEBUG1, "row filter contained something unexpected: %s", nodeToString(node)); + forbidden = _("too complex"); + } + + if (forbidden) + ereport(ERROR, + (errmsg("invalid publication WHERE expression for relation \"%s\"", + context->relname), + errdetail("%s", forbidden), + errhint("only simple expressions using columns and constants are allowed") + )); + + return expression_tree_walker(node, rowfilter_walker, (void *)context); +} + +/* + * Walk the parse-tree of this publication row filter expression and throw an + * error if it encounters anything not permitted or unexpected. + */ +void +rowfilter_validator(char *relname, Node *expr) +{ + rf_context context = {0}; + + context.relname = relname; + rowfilter_walker(expr, &context); +} + +/* * Find all the columns referenced by the row-filter expression and return what * is found as a list of RfCol. This list is used for row-filter validation. */ diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index eac7449..e49b3ca 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -144,7 +144,7 @@ pg_relation_is_publishable(PG_FUNCTION_ARGS) * Walk the parse-tree to decide if the row-filter is valid or not. */ static void -rowfilter_expr_checker(Publication *pub, Node *rfnode, Relation rel) +rowfilter_expr_checker(Publication *pub, ParseState *pstate, Node *rfnode, Relation rel) { Oid relid = RelationGetRelid(rel); char *relname = RelationGetRelationName(rel); @@ -152,6 +152,14 @@ rowfilter_expr_checker(Publication *pub, Node *rfnode, Relation rel) /* * Rule: * + * Walk the parse-tree and reject anything more complicated than a very + * simple expression. + */ + rowfilter_validator(relname, rfnode); + + /* + * Rule: + * * If the publish operation contains "delete" then only columns that * are allowed by the REPLICA IDENTITY rules are permitted to be used in * the row-filter WHERE clause. @@ -305,13 +313,13 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri, whereclause = transformWhereClause(pstate, copyObject(pri->whereClause), EXPR_KIND_PUBLICATION_WHERE, - "PUBLICATION"); + "PUBLICATION WHERE"); /* Fix up collation information */ assign_expr_collations(pstate, whereclause); /* Validate the row-filter. */ - rowfilter_expr_checker(pub, whereclause, targetrel); + rowfilter_expr_checker(pub, pstate, whereclause, targetrel); } /* Form a tuple. */ diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index 193c87d..5e0c391 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -552,11 +552,12 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr) break; case EXPR_KIND_PUBLICATION_WHERE: +#if 0 if (isAgg) err = _("aggregate functions are not allowed in publication WHERE expressions"); else err = _("grouping operations are not allowed in publication WHERE expressions"); - +#endif break; case EXPR_KIND_CYCLE_MARK: @@ -951,7 +952,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc, errkind = true; break; case EXPR_KIND_PUBLICATION_WHERE: +#if 0 err = _("window functions are not allowed in publication WHERE expressions"); +#endif break; /* diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 3d43839..3519e62 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -201,6 +201,7 @@ transformExprRecurse(ParseState *pstate, Node *expr) case T_FuncCall: { +#if 0 /* * Forbid functions in publication WHERE condition */ @@ -209,6 +210,7 @@ transformExprRecurse(ParseState *pstate, Node *expr) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("functions are not allowed in publication WHERE expressions"), parser_errposition(pstate, exprLocation(expr)))); +#endif result = transformFuncCall(pstate, (FuncCall *) expr); break; @@ -1777,7 +1779,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink) err = _("cannot use subquery in column generation expression"); break; case EXPR_KIND_PUBLICATION_WHERE: +#if 0 err = _("cannot use subquery in publication WHERE expression"); +#endif break; /* @@ -3100,7 +3104,7 @@ ParseExprKindName(ParseExprKind exprKind) case EXPR_KIND_CYCLE_MARK: return "CYCLE"; case EXPR_KIND_PUBLICATION_WHERE: - return "publication expression"; + return "publication WHERE expression"; /* * There is intentionally no default: case here, so that the diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index e946f17..de9600f 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -2656,7 +2656,10 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location) errkind = true; break; case EXPR_KIND_PUBLICATION_WHERE: + pstate->p_hasTargetSRFs = true; +#if 0 err = _("set-returning functions are not allowed in publication WHERE expressions"); +#endif break; /* diff --git a/src/backend/parser/parse_oper.c b/src/backend/parser/parse_oper.c index 29f8835..b3588df 100644 --- a/src/backend/parser/parse_oper.c +++ b/src/backend/parser/parse_oper.c @@ -718,12 +718,14 @@ make_op(ParseState *pstate, List *opname, Node *ltree, Node *rtree, opform->oprright)), parser_errposition(pstate, location))); +#if 0 /* Check it's not a custom operator for publication WHERE expressions */ if (pstate->p_expr_kind == EXPR_KIND_PUBLICATION_WHERE && opform->oid >= FirstNormalObjectId) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("user-defined operators are not allowed in publication WHERE expressions"), parser_errposition(pstate, location))); +#endif /* Do typecasting and build the expression tree */ if (ltree == NULL) diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h index 2c7310e..dd69aff 100644 --- a/src/include/catalog/dependency.h +++ b/src/include/catalog/dependency.h @@ -16,7 +16,6 @@ #include "catalog/objectaddress.h" - /* * Precise semantics of a dependency relationship are specified by the * DependencyType code (which is stored in a "char" field in pg_depend, @@ -156,6 +155,7 @@ typedef struct RfCol { int attnum; } RfCol; extern List *rowfilter_find_cols(Node *expr, Oid relId); +extern void rowfilter_validator(char *relname, Node *expr); extern void recordDependencyOnExpr(const ObjectAddress *depender, Node *expr, List *rtable, diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out index e7c8c19..e10adc8 100644 --- a/src/test/regress/expected/publication.out +++ b/src/test/regress/expected/publication.out @@ -218,16 +218,21 @@ Tables: -- fail - functions disallowed ALTER PUBLICATION testpub5 ADD TABLE testpub_rf_tbl4 WHERE (length(g) < 6); -ERROR: functions are not allowed in publication WHERE expressions -LINE 1: ...ICATION testpub5 ADD TABLE testpub_rf_tbl4 WHERE (length(g) ... - ^ +ERROR: invalid publication WHERE expression for relation "testpub_rf_tbl4" +DETAIL: function calls are not allowed +HINT: only simple expressions using columns and constants are allowed -- fail - user-defined operators disallowed CREATE FUNCTION testpub_rf_func(integer, integer) RETURNS boolean AS $$ SELECT hashint4($1) > $2 $$ LANGUAGE SQL; CREATE OPERATOR =#> (PROCEDURE = testpub_rf_func, LEFTARG = integer, RIGHTARG = integer); CREATE PUBLICATION testpub6 FOR TABLE testpub_rf_tbl3 WHERE (e =#> 27); -ERROR: user-defined operators are not allowed in publication WHERE expressions -LINE 1: ...ICATION testpub6 FOR TABLE testpub_rf_tbl3 WHERE (e =#> 27); - ^ +ERROR: invalid publication WHERE expression for relation "testpub_rf_tbl3" +DETAIL: user-defined operators are not allowed +HINT: only simple expressions using columns and constants are allowed +-- fail - row-filter expression is not simple +CREATE PUBLICATION testpub7 FOR TABLE testpub_rf_tbl1 WHERE (a IN (SELECT generate_series(1,5))); +ERROR: invalid publication WHERE expression for relation "testpub_rf_tbl1" +DETAIL: too complex +HINT: only simple expressions using columns and constants are allowed -- fail - WHERE not allowed in DROP ALTER PUBLICATION testpub5 DROP TABLE testpub_rf_tbl3 WHERE (e < 27); ERROR: syntax error at or near "WHERE" diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql index 6701d50..a30657b 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -123,6 +123,8 @@ ALTER PUBLICATION testpub5 ADD TABLE testpub_rf_tbl4 WHERE (length(g) < 6); CREATE FUNCTION testpub_rf_func(integer, integer) RETURNS boolean AS $$ SELECT hashint4($1) > $2 $$ LANGUAGE SQL; CREATE OPERATOR =#> (PROCEDURE = testpub_rf_func, LEFTARG = integer, RIGHTARG = integer); CREATE PUBLICATION testpub6 FOR TABLE testpub_rf_tbl3 WHERE (e =#> 27); +-- fail - row-filter expression is not simple +CREATE PUBLICATION testpub7 FOR TABLE testpub_rf_tbl1 WHERE (a IN (SELECT generate_series(1,5))); -- fail - WHERE not allowed in DROP ALTER PUBLICATION testpub5 DROP TABLE testpub_rf_tbl3 WHERE (e < 27); -- 1.8.3.1