public inbox for [email protected]help / color / mirror / Atom feed
Re: Fix search_path for all maintenance commands 11+ messages / 6 participants [nested] [flat]
* Re: Fix search_path for all maintenance commands @ 2023-07-07 01:39 Jeff Davis <[email protected]> 0 siblings, 2 replies; 11+ messages in thread From: Jeff Davis @ 2023-07-07 01:39 UTC (permalink / raw) To: pgsql-hackers; +Cc: Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>; GurjeetSingh <[email protected]> On Fri, 2023-05-26 at 16:21 -0700, Jeff Davis wrote: > Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, > REINDEX, and VACUUM) currently run as the table owner, and as a > SECURITY_RESTRICTED_OPERATION. > > I propose that we also fix the search_path to "pg_catalog, pg_temp" > when running maintenance commands New patch attached. We need this patch for several reasons: * If you have a functional index, and the function depends on the search_path, then it's easy to corrupt your index if you (or a superuser) run a REINDE/CLUSTER with the wrong search_path. * The MAINTAIN privilege needs a safe search_path, and was reverted from 16 because the search_path in 16 is not restricted. * In general, it's a good idea for things like functional indexes and materialized views to be independent of the search_path. * The search_path is already restricted in some other contexts, like logical replication and autoanalyze. Others have raised some concerns though: * It might break for users who have a functional index where the function implicitly depends on a search_path containing a namespace other than pg_catalog. My opinion is that such functional indexes are conceptually broken and we need to desupport them, and there will be some breakage, but I'm open to suggestion about how we minimize that (a compatibility GUC or something?). * The fix might not go far enough or might be in the wrong place. I'm open to suggestion here, too. Maybe we can make it part of the general function call mechanism, and can be overridden by explicitly setting the function search path? Or maybe we need new syntax where the function can acquire the search path from the session explicitly, but uses a safe search path by default? -- Jeff Davis PostgreSQL Contributor Team - AWS Attachments: [text/x-patch] v5-0001-Restrict-search_path-when-performing-maintenance.patch (16.4K, ../../[email protected]/2-v5-0001-Restrict-search_path-when-performing-maintenance.patch) download | inline diff: From 46dbbf86e927e1872004c37f651e6a5b5c62bdd0 Mon Sep 17 00:00:00 2001 From: Jeff Davis <[email protected]> Date: Thu, 6 Jul 2023 13:06:22 -0700 Subject: [PATCH v5] Restrict search_path when performing maintenance. When executing maintenance operations (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, REINDEX, or VACUUM), set search_path to 'pg_catalog, pg_temp'. Functions that are used for functional indexes, in index expressions, or in materialized views and depend on a different search path should be declared with CREATE FUNCTION ... SET search_path='...'. This change was previously committed as 05e1737351, but was not acceptable for 16 and reverted. Discussion: https://postgr.es/m/CA%2BTgmoZVCHERUkXhAMT2Er-sKBc5C6_iX%2BTpxxivBevDHzq2TQ%40mail.gmail.com Discussion: https://postgr.es/m/e44327179e5c9015c8dda67351c04da552066017.camel%40j-davis.com --- contrib/amcheck/verify_nbtree.c | 2 ++ src/backend/access/brin/brin.c | 2 ++ src/backend/catalog/index.c | 8 ++++++++ src/backend/commands/analyze.c | 2 ++ src/backend/commands/cluster.c | 2 ++ src/backend/commands/indexcmds.c | 6 ++++++ src/backend/commands/matview.c | 2 ++ src/backend/commands/vacuum.c | 2 ++ src/bin/scripts/t/100_vacuumdb.pl | 4 ---- src/include/utils/guc.h | 6 ++++++ .../test_oat_hooks/expected/test_oat_hooks.out | 4 ++++ src/test/regress/expected/privileges.out | 12 ++++++------ src/test/regress/expected/vacuum.out | 2 +- src/test/regress/sql/privileges.sql | 8 ++++---- src/test/regress/sql/vacuum.sql | 2 +- 15 files changed, 48 insertions(+), 16 deletions(-) diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c index 94a9759322..35035967f9 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -281,6 +281,8 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed, SetUserIdAndSecContext(heaprel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); } else { diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 3c6a956eaa..11e78cb62c 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -1066,6 +1066,8 @@ brin_summarize_range(PG_FUNCTION_ARGS) SetUserIdAndSecContext(heapRel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); } else { diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 67b743e251..64127a894f 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1476,6 +1476,8 @@ index_concurrently_build(Oid heapRelationId, SetUserIdAndSecContext(heapRel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); indexRelation = index_open(indexRelationId, RowExclusiveLock); @@ -3007,6 +3009,8 @@ index_build(Relation heapRelation, SetUserIdAndSecContext(heapRelation->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* Set up initial progress report status */ { @@ -3343,6 +3347,8 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot) SetUserIdAndSecContext(heapRelation->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); indexRelation = index_open(indexId, RowExclusiveLock); @@ -3603,6 +3609,8 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence, SetUserIdAndSecContext(heapRelation->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); if (progress) { diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index 9c8413eef2..c71051eaf2 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -348,6 +348,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params, SetUserIdAndSecContext(onerel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* measure elapsed time iff autovacuum logging requires it */ if (IsAutoVacuumWorkerProcess() && params->log_min_duration >= 0) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 03b24ab90f..393c732315 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -355,6 +355,8 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params) SetUserIdAndSecContext(OldHeap->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* * Since we may open a new transaction for each relation, we have to check diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 403f5fc143..8f182c1493 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -575,6 +575,8 @@ DefineIndex(Oid relationId, int root_save_nestlevel; root_save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* * Some callers need us to run with an empty default_tablespace; this is a @@ -1300,6 +1302,8 @@ DefineIndex(Oid relationId, SetUserIdAndSecContext(childrel->rd_rel->relowner, child_save_sec_context | SECURITY_RESTRICTED_OPERATION); child_save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* * Don't try to create indexes on foreign tables, though. Skip @@ -3767,6 +3771,8 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) SetUserIdAndSecContext(heapRel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* determine safety of this index for set_indexsafe_procflags */ idx->safe = (indexRel->rd_indexprs == NIL && diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c index f9a3bdfc3a..9114a7924c 100644 --- a/src/backend/commands/matview.c +++ b/src/backend/commands/matview.c @@ -179,6 +179,8 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString, SetUserIdAndSecContext(relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* Make sure it is a materialized view. */ if (matviewRel->rd_rel->relkind != RELKIND_MATVIEW) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 7fe6a54c06..6dd0f1e77a 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -2171,6 +2171,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, SetUserIdAndSecContext(rel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* * If PROCESS_MAIN is set (the default), it's time to vacuum the main diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl index eccfcc54a1..f91b5127a8 100644 --- a/src/bin/scripts/t/100_vacuumdb.pl +++ b/src/bin/scripts/t/100_vacuumdb.pl @@ -109,15 +109,11 @@ $node->safe_psql( CREATE FUNCTION f1(int) RETURNS int LANGUAGE SQL AS 'SELECT f0($1)'; CREATE TABLE funcidx (x int); INSERT INTO funcidx VALUES (0),(1),(2),(3); - CREATE INDEX i0 ON funcidx ((f1(x))); CREATE SCHEMA "Foo"; CREATE TABLE "Foo".bar(id int); |); $node->command_ok([qw|vacuumdb -Z --table="need""q(uot"(")x") postgres|], 'column list'); -$node->command_fails( - [qw|vacuumdb -Zt funcidx postgres|], - 'unqualified name via functional index'); $node->command_fails( [ 'vacuumdb', '--analyze', '--table', 'vactable(c)', 'postgres' ], diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index d5253c7ed2..edd82a551b 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -201,6 +201,12 @@ typedef enum #define GUC_QUALIFIER_SEPARATOR '.' +/* + * Safe search path when executing code as the table owner, such as during + * maintenance operations. + */ +#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp" + /* * Bit values in "flags" of a GUC variable. Note that these don't appear * on disk, so we can reassign their values freely. diff --git a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out index f80373aecc..effdc49145 100644 --- a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out +++ b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out @@ -89,11 +89,15 @@ NOTICE: in object access: superuser finished create (subId=0x0) [internal] NOTICE: in process utility: superuser finished CREATE TABLE CREATE INDEX regress_test_table_t_idx ON regress_test_table (t); NOTICE: in process utility: superuser attempting CREATE INDEX +NOTICE: in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed] +NOTICE: in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed] NOTICE: in object access: superuser attempting create (subId=0x0) [explicit] NOTICE: in object access: superuser finished create (subId=0x0) [explicit] NOTICE: in process utility: superuser finished CREATE INDEX GRANT SELECT ON Table regress_test_table TO public; NOTICE: in process utility: superuser attempting GRANT +NOTICE: in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed] +NOTICE: in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed] NOTICE: in process utility: superuser finished GRANT CREATE FUNCTION regress_test_func (t text) RETURNS text AS $$ SELECT $1; diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 3e4dfcc2ec..6457da9531 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -1769,7 +1769,7 @@ SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS 'GRANT regress_priv_group2 TO regress_sro_user'; CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS - 'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true'; + 'DECLARE c CURSOR WITH HOLD FOR SELECT public.unwanted_grant(); SELECT true'; -- REFRESH of this MV will queue a GRANT at end of transaction CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA; REFRESH MATERIALIZED VIEW sro_mv; @@ -1783,12 +1783,12 @@ SET SESSION AUTHORIZATION regress_sro_user; -- INSERT to this table will queue a GRANT at end of transaction CREATE TABLE sro_trojan_table (); CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS - 'BEGIN PERFORM unwanted_grant(); RETURN NULL; END'; + 'BEGIN PERFORM public.unwanted_grant(); RETURN NULL; END'; CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan(); -- Now, REFRESH will issue such an INSERT, queueing the GRANT CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS - 'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true'; + 'INSERT INTO public.sro_trojan_table DEFAULT VALUES; SELECT true'; REFRESH MATERIALIZED VIEW sro_mv; ERROR: cannot fire deferred trigger within security-restricted operation CONTEXT: SQL function "mv_action" statement 1 @@ -1800,15 +1800,15 @@ BEGIN; SET CONSTRAINTS ALL IMMEDIATE; REFRESH MATERIALIZED VIEW sro_mv; COMMIT; ERROR: permission denied to grant role "regress_priv_group2" DETAIL: Only roles with the ADMIN option on role "regress_priv_group2" may grant this role. CONTEXT: SQL function "unwanted_grant" statement 1 -SQL statement "SELECT unwanted_grant()" -PL/pgSQL function sro_trojan() line 1 at PERFORM +SQL statement "SELECT public.unwanted_grant()" +PL/pgSQL function public.sro_trojan() line 1 at PERFORM SQL function "mv_action" statement 1 -- REFRESH MATERIALIZED VIEW CONCURRENTLY use of eval_const_expressions() SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int IMMUTABLE LANGUAGE plpgsql AS $$ BEGIN - PERFORM unwanted_grant(); + PERFORM public.unwanted_grant(); RAISE WARNING 'owned'; RETURN 1; EXCEPTION WHEN OTHERS THEN diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 4def90b805..330fcd884c 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -64,7 +64,7 @@ CLUSTER vaccluster; CREATE FUNCTION do_analyze() RETURNS VOID VOLATILE LANGUAGE SQL AS 'ANALYZE pg_am'; CREATE FUNCTION wrap_do_analyze(c INT) RETURNS INT IMMUTABLE LANGUAGE SQL - AS 'SELECT $1 FROM do_analyze()'; + AS 'SELECT $1 FROM public.do_analyze()'; CREATE INDEX ON vaccluster(wrap_do_analyze(i)); INSERT INTO vaccluster VALUES (1), (2); ANALYZE vaccluster; diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index 134809e8cc..e961748d79 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -1177,7 +1177,7 @@ SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS 'GRANT regress_priv_group2 TO regress_sro_user'; CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS - 'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true'; + 'DECLARE c CURSOR WITH HOLD FOR SELECT public.unwanted_grant(); SELECT true'; -- REFRESH of this MV will queue a GRANT at end of transaction CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA; REFRESH MATERIALIZED VIEW sro_mv; @@ -1188,12 +1188,12 @@ SET SESSION AUTHORIZATION regress_sro_user; -- INSERT to this table will queue a GRANT at end of transaction CREATE TABLE sro_trojan_table (); CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS - 'BEGIN PERFORM unwanted_grant(); RETURN NULL; END'; + 'BEGIN PERFORM public.unwanted_grant(); RETURN NULL; END'; CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan(); -- Now, REFRESH will issue such an INSERT, queueing the GRANT CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS - 'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true'; + 'INSERT INTO public.sro_trojan_table DEFAULT VALUES; SELECT true'; REFRESH MATERIALIZED VIEW sro_mv; \c - REFRESH MATERIALIZED VIEW sro_mv; @@ -1204,7 +1204,7 @@ SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int IMMUTABLE LANGUAGE plpgsql AS $$ BEGIN - PERFORM unwanted_grant(); + PERFORM public.unwanted_grant(); RAISE WARNING 'owned'; RETURN 1; EXCEPTION WHEN OTHERS THEN diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql index 51d7b1fecc..0b63ef8dc6 100644 --- a/src/test/regress/sql/vacuum.sql +++ b/src/test/regress/sql/vacuum.sql @@ -49,7 +49,7 @@ CLUSTER vaccluster; CREATE FUNCTION do_analyze() RETURNS VOID VOLATILE LANGUAGE SQL AS 'ANALYZE pg_am'; CREATE FUNCTION wrap_do_analyze(c INT) RETURNS INT IMMUTABLE LANGUAGE SQL - AS 'SELECT $1 FROM do_analyze()'; + AS 'SELECT $1 FROM public.do_analyze()'; CREATE INDEX ON vaccluster(wrap_do_analyze(i)); INSERT INTO vaccluster VALUES (1), (2); ANALYZE vaccluster; -- 2.34.1 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2023-07-07 03:22 Isaac Morland <[email protected]> parent: Jeff Davis <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Isaac Morland @ 2023-07-07 03:22 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers; Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>; GurjeetSingh <[email protected]> On Thu, 6 Jul 2023 at 21:39, Jeff Davis <[email protected]> wrote: I apologize in advance if anything I’ve written below is either too obvious or too crazy or misinformed to belong here. I hope I have something to say that is on point, but feel unsure what makes sense to say. * It might break for users who have a functional index where the > function implicitly depends on a search_path containing a namespace > other than pg_catalog. My opinion is that such functional indexes are > conceptually broken and we need to desupport them, and there will be > some breakage, but I'm open to suggestion about how we minimize that (a > compatibility GUC or something?). > I agree this is OK. If somebody has an index whole meaning depends on the search_path, then the best that can be said is that their database hasn't been corrupted yet. At the same time, I can see that somebody would get upset if they couldn't upgrade their database because of this. Maybe pg_upgrade could apply "SET search_path TO pg_catalog, pg_temp" to any function used in a functional index that doesn't have a search_path setting of its own? (BEGIN ATOMIC functions count, if I understand correctly, as having a search_path setting, because the lookups happen at definition time) Now I'm doing more reading and I'm worried about SET TIME ZONE (or more precisely, its absence) and maybe some other ones. * The fix might not go far enough or might be in the wrong place. I'm > open to suggestion here, too. Maybe we can make it part of the general > function call mechanism, and can be overridden by explicitly setting > the function search path? Or maybe we need new syntax where the > function can acquire the search path from the session explicitly, but > uses a safe search path by default? > Change it so by default each function gets handled as if "SET search_path FROM CURRENT" was applied to it? That's what I do for all my functions (maybe hurting performance?). Expand on my pg_upgrade idea above by applying it to all functions? I feel that this may tie into other behaviour issues where to me it is obvious that the expected behaviour should be different from the actual behaviour. If a view calls a function, shouldn't it be called in the context of the view's definer/owner? It's weird that I can write a view that filters a table for users of the view, but as soon as the view calls functions they run in the security context of the user of the view. Are views security definers or not? Similar comment for triggers. Also as far as I can tell there is no way for a security definer function to determine who (which user) invoked it. So I can grant/deny access to run a particular function using permissions, but I can't have the supposed security definer define security for different callers. Is the fundamental problem that we now find ourselves wanting to do things that require different defaults to work smoothly? On some level I suspect we want lexical scoping, which is what most of us have in our programming languages, in the database; but the database has many elements of dynamic scoping, and changing that is both a compatibility break and requires significant changes in the way the database is designed. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2023-07-07 15:42 Jeff Davis <[email protected]> parent: Isaac Morland <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Jeff Davis @ 2023-07-07 15:42 UTC (permalink / raw) To: Isaac Morland <[email protected]>; +Cc: pgsql-hackers; Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>; GurjeetSingh <[email protected]> Hi, On Thu, 2023-07-06 at 23:22 -0400, Isaac Morland wrote: > Maybe pg_upgrade could apply "SET search_path TO pg_catalog, pg_temp" > to any function used in a functional index that doesn't have a > search_path setting of its own? I don't think we want to go down the road of trying to solve this at upgrade time. > Now I'm doing more reading and I'm worried about SET TIME ZONE (or > more precisely, its absence) and maybe some other ones. That's a good point that it's not limited to search_path, but search_path is by far the biggest problem. For one thing, functions affected by TimeZone or other GUCs are typically marked STABLE, and can't be used in index expressions. Also, search_path affects a lot more functions. > Change it so by default each function gets handled as if "SET > search_path FROM CURRENT" was applied to it? Yes, that's one idea, along with some syntax to get the old behavior (inherit search_path at runtime) if you want. It feels weird to make search_path too special in the syntax though. If we want a general solution, we could do something like: CREATE FUNCTION ... [DEPENDS ON CONFIGURATION {NONE|{some_guc}[, ...]}] [CONFIGURATION IS {STATIC|DYNAMIC}] where STATIC means "all of the GUC dependencies are SET FROM CURRENT unless specified otherwise" and DYNAMIC means "all of the GUC dependencies come from the session at runtime unless specified otherwise". The default would be "DEPENDS CONFIGURATION search_path CONFIGURATION IS STATIC". That would make search_path special only because, by default, every function would depend on it. Which I think summarizes the reason search_path really is special. That also opens up opportunities to do other things we might want to do: * have a compatibility GUC to set the default back to DYNAMIC * track other dependencies of functions better ("DEPENDS ON TABLE ...") * provide better error messages, like "can't use function xyz in index expression because it depends on configuration parameter foo" * be more consistent about using STABLE to mean that the function depends on a snapshot, rather than overloading it for GUC dependencies The question is, given that the acute problem is search_path, do we want to invent all of the syntax above? Are there other use cases for it, or should we just focus on search_path? > That's what I do for all my functions (maybe hurting performance?). It doesn't look cheap, although I think we could optimize it. > If a view calls a function, shouldn't it be called in the context of > the view's definer/owner? Yeah, there are a bunch of problems along those lines. I don't know if we can solve them all in one release. > Is the fundamental problem that we now find ourselves wanting to do > things that require different defaults to work smoothly? On some > level I suspect we want lexical scoping, which is what most of us > have in our programming languages, in the database; but the database > has many elements of dynamic scoping, and changing that is both a > compatibility break and requires significant changes in the way the > database is designed. Does that suggest another approach? Regards, Jeff Davis ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2023-07-13 18:56 Jeff Davis <[email protected]> parent: Jeff Davis <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Jeff Davis @ 2023-07-13 18:56 UTC (permalink / raw) To: pgsql-hackers; +Cc: Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]>; GurjeetSingh <[email protected]> On Thu, 2023-07-06 at 18:39 -0700, Jeff Davis wrote: > * The fix might not go far enough or might be in the wrong place. I'm > open to suggestion here, too. Maybe we can make it part of the > general > function call mechanism, and can be overridden by explicitly setting > the function search path? Or maybe we need new syntax where the > function can acquire the search path from the session explicitly, but > uses a safe search path by default? I'm inclined to proceed with the current approach here, which is to just fix search_path for maintenance commands. Other approaches may be possible, but: (a) We already special-case the way functions are executed for maintenance commands in other ways -- we run the functions as the table owner (even SECURITY INVOKER functions) and run them as a SECURITY_RESTRICTED_OPERATION. Setting the search_path to a safe value seems like a natural extension of that; and (b) The lack of a safe search path is blocking other useful features, such as the MAINTAIN privilege; and (c) I don't see other proposals, aside from a few ideas I put forward here[1], which didn't get any responses. The current approach seemed to get support from Noah, Nathan, and Greg Stark. Concerns were raised by Gurjeet, Tom, and Robert in the 16 cycle; but I'm not sure whether those objections were specific to the 16 cycle or whether they are objections to the approach entirely. Comments? Regards, Jeff Davis [1] https://www.postgresql.org/message-id/6781cc79580c464a63fc0a1343637ed2b2b0cf09.camel%40j-davis.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2023-07-13 19:54 Gurjeet Singh <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Gurjeet Singh @ 2023-07-13 19:54 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers; Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]> On Thu, Jul 13, 2023 at 11:56 AM Jeff Davis <[email protected]> wrote: > > The current approach seemed to get support from Noah, Nathan, and Greg > Stark. > > Concerns were raised by Gurjeet, Tom, and Robert in the 16 cycle; but I didn't see Tom's or Robert's concerns raised in this thread. I see now that for some reason there are two threads with slightly different subjects. I'll catch-up on that, as well. The other thread's subject: "pgsql: Fix search_path to a safe value during maintenance operations" > I'm not sure whether those objections were specific to the 16 cycle or > whether they are objections to the approach entirely. Comments? The approach seems good to me. My concern is with this change's potential to cause an extended database outage. Hence sending it out as part of v16, without any escape hatch for the DBA, is my objection. It it were some commercial database, we would have simply introduced a hidden event, or a GUC, with default off. But a GUC for this feels too heavy handed. Perhaps we can provide an escape hatch as follows (warning, pseudocode ahead). > if (first_element(search_path) != "dont_override_search_patch_for_maintenance") > SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, ...); So, if someone desperately wants to just plow through the maintenance command, and are not ready or able to fix their dependence on their search_path, the community can show them this escape-hatch. Best regards, Gurjeet http://Gurje.et ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2023-07-13 20:37 David G. Johnston <[email protected]> parent: Gurjeet Singh <[email protected]> 0 siblings, 2 replies; 11+ messages in thread From: David G. Johnston @ 2023-07-13 20:37 UTC (permalink / raw) To: Gurjeet Singh <[email protected]>; +Cc: Jeff Davis <[email protected]>; pgsql-hackers; Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]> On Thu, Jul 13, 2023 at 12:54 PM Gurjeet Singh <[email protected]> wrote: > > The approach seems good to me. My concern is with this change's > potential to cause an extended database outage. Hence sending it out > as part of v16, without any escape hatch for the DBA, is my objection. > > If this is limited to MAINT, which I'm in support of, there is no need for an "escape hatch". A prerequisite for leveraging the new feature is that you fix the code so it conforms to the new way of doing things. Tom's opinion was a general dislike for differing behavior in different situations. I dislike it too, on purist grounds, but would rather do this than not make any forward progress because we made a poor decision in the past. And I'm against simply breaking the past without any recourse as what we did for pg_dump/pg_restore still bothers me. David J. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2023-07-13 21:00 Gurjeet Singh <[email protected]> parent: David G. Johnston <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Gurjeet Singh @ 2023-07-13 21:00 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Jeff Davis <[email protected]>; pgsql-hackers; Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]> On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston <[email protected]> wrote: > > I'm against simply breaking the past without any recourse as what we did for pg_dump/pg_restore still bothers me. I'm sure this is tangential, but can you please provide some context/links to the change you're referring to here. Best regards, Gurjeet http://Gurje.et ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2023-07-13 21:07 David G. Johnston <[email protected]> parent: Gurjeet Singh <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: David G. Johnston @ 2023-07-13 21:07 UTC (permalink / raw) To: Gurjeet Singh <[email protected]>; +Cc: Jeff Davis <[email protected]>; pgsql-hackers; Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]> On Thu, Jul 13, 2023 at 2:00 PM Gurjeet Singh <[email protected]> wrote: > On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston > <[email protected]> wrote: > > > > I'm against simply breaking the past without any recourse as what we > did for pg_dump/pg_restore still bothers me. > > I'm sure this is tangential, but can you please provide some > context/links to the change you're referring to here. > > Here is the instigating issue and a discussion thread on the aftermath: https://wiki.postgresql.org/wiki/A_Guide_to_CVE-2018-1058%3A_Protect_Your_Search_Path https://www.postgresql.org/message-id/flat/13033.1531517020%40sss.pgh.pa.us#2aa2e25816d899d62f168926... David J. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2023-07-13 22:19 Jeff Davis <[email protected]> parent: David G. Johnston <[email protected]> 1 sibling, 0 replies; 11+ messages in thread From: Jeff Davis @ 2023-07-13 22:19 UTC (permalink / raw) To: David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; +Cc: pgsql-hackers; Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]> On Thu, 2023-07-13 at 13:37 -0700, David G. Johnston wrote: > If this is limited to MAINT, which I'm in support of, there is no > need for an "escape hatch". A prerequisite for leveraging the new > feature is that you fix the code so it conforms to the new way of > doing things. The current patch is not limited to exercise of the MAINTAIN privilege. > Tom's opinion was a general dislike for differing behavior in > different situations. I dislike it too, on purist grounds, but would > rather do this than not make any forward progress because we made a > poor decision in the past. I believe the opinion you're referring to is here: https://www.postgresql.org/message-id/[email protected] Which was a reaction to another version of my patch which implemented your idea to limit the changes to the MAINTAIN privilege. I agree with you that we should be practical here. The end goal is to move users away from using functions that both (a) implicitly depend on the search_path; and (b) are called implicitly as a side-effect of accessing a table. Whatever is the fastest and smoothest way to get there is fine with me. > And I'm against simply breaking the past without any recourse as what > we did for pg_dump/pg_restore still bothers me. Is a GUC the solution here? Gurjeet called it heavy-handed, and I see the point that carrying such a GUC forever would be unpleasant. But if it reduces the risk of breakage (or at least offers an escape hatch) then it may be wise, and hopefully we can remove it later. Regards, Jeff Davis ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2023-07-15 21:13 Noah Misch <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Noah Misch @ 2023-07-15 21:13 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Gurjeet Singh <[email protected]>; Jeff Davis <[email protected]>; pgsql-hackers; Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]> On Thu, Jul 13, 2023 at 02:07:27PM -0700, David G. Johnston wrote: > On Thu, Jul 13, 2023 at 2:00 PM Gurjeet Singh <[email protected]> wrote: > > On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston <[email protected]> wrote: > > > I'm against simply breaking the past without any recourse as what we > > did for pg_dump/pg_restore still bothers me. > > > > I'm sure this is tangential, but can you please provide some > > context/links to the change you're referring to here. > > Here is the instigating issue and a discussion thread on the aftermath: > https://wiki.postgresql.org/wiki/A_Guide_to_CVE-2018-1058%3A_Protect_Your_Search_Path > https://www.postgresql.org/message-id/flat/13033.1531517020%40sss.pgh.pa.us#2aa2e25816d899d62f168926... I don't blame you for feeling bothered about it. A benefit of having done it is that we gained insight into the level of pain it caused. If it had been sufficiently painful, someone would have quickly added an escape hatch. Five years later, nobody has added one. The 2018 security fixes instigated many function repairs that $SUBJECT would otherwise instigate. That wasn't too painful. The net new pain of $SUBJECT will be less, since the 2018 security fixes prepared the path. Hence, I remain +1 for the latest Davis proposal. ^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v5 1/2] pg_lfind32(): add "overlap" code for remaining elements @ 2024-03-20 19:20 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Nathan Bossart @ 2024-03-20 19:20 UTC (permalink / raw) --- src/include/port/pg_lfind.h | 102 +++++++++++++++++++++++++----------- 1 file changed, 71 insertions(+), 31 deletions(-) diff --git a/src/include/port/pg_lfind.h b/src/include/port/pg_lfind.h index b8dfa66eef..21af399dc4 100644 --- a/src/include/port/pg_lfind.h +++ b/src/include/port/pg_lfind.h @@ -80,6 +80,49 @@ pg_lfind8_le(uint8 key, uint8 *base, uint32 nelem) return false; } +/* + * pg_lfind32_helper + * + * Searches one 4-register-block of integers. The caller is responsible for + * ensuring that there are at least 4-registers-worth of integers remaining. + */ +static inline bool +pg_lfind32_helper(const Vector32 keys, uint32 *base) +{ + const uint32 nelem_per_vector = sizeof(Vector32) / sizeof(uint32); + Vector32 vals1, + vals2, + vals3, + vals4, + result1, + result2, + result3, + result4, + tmp1, + tmp2, + result; + + /* load the next block into 4 registers */ + vector32_load(&vals1, base); + vector32_load(&vals2, &base[nelem_per_vector]); + vector32_load(&vals3, &base[nelem_per_vector * 2]); + vector32_load(&vals4, &base[nelem_per_vector * 3]); + + /* compare each value to the key */ + result1 = vector32_eq(keys, vals1); + result2 = vector32_eq(keys, vals2); + result3 = vector32_eq(keys, vals3); + result4 = vector32_eq(keys, vals4); + + /* combine the results into a single variable */ + tmp1 = vector32_or(result1, result2); + tmp2 = vector32_or(result3, result4); + result = vector32_or(tmp1, tmp2); + + /* return whether there was a match */ + return vector32_is_highbit_set(result); +} + /* * pg_lfind32 * @@ -119,46 +162,43 @@ pg_lfind32(uint32 key, uint32 *base, uint32 nelem) } #endif + /* + * If there aren't enough elements for the SIMD code, jump to the standard + * one-by-one linear search code. + */ + if (nelem < nelem_per_iteration) + goto one_by_one; + + /* + * Process as many elements as possible with a block of 4 registers. + */ for (i = 0; i < tail_idx; i += nelem_per_iteration) { - Vector32 vals1, - vals2, - vals3, - vals4, - result1, - result2, - result3, - result4, - tmp1, - tmp2, - result; - - /* load the next block into 4 registers */ - vector32_load(&vals1, &base[i]); - vector32_load(&vals2, &base[i + nelem_per_vector]); - vector32_load(&vals3, &base[i + nelem_per_vector * 2]); - vector32_load(&vals4, &base[i + nelem_per_vector * 3]); - - /* compare each value to the key */ - result1 = vector32_eq(keys, vals1); - result2 = vector32_eq(keys, vals2); - result3 = vector32_eq(keys, vals3); - result4 = vector32_eq(keys, vals4); - - /* combine the results into a single variable */ - tmp1 = vector32_or(result1, result2); - tmp2 = vector32_or(result3, result4); - result = vector32_or(tmp1, tmp2); - - /* see if there was a match */ - if (vector32_is_highbit_set(result)) + if (pg_lfind32_helper(keys, &base[i])) { Assert(assert_result == true); return true; } } + + /* + * If any elements remain, process the last 'nelem_per_iteration' elements + * in the array with a 4-register block. This will cause us to check some + * elements more than once, but that won't affect correctness, and testing + * has demonstrated that this helps more cases than it harms. + */ + if (i != nelem && + pg_lfind32_helper(keys, &base[nelem - nelem_per_iteration])) + { + Assert(assert_result); + return true; + } + + Assert(!assert_result); + return false; #endif /* ! USE_NO_SIMD */ +one_by_one: /* Process the remaining elements one at a time. */ for (; i < nelem; i++) { -- 2.25.1 --EVF5PPMfhYS0aIcm Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5-0002-Add-support-for-AVX2-in-simd.h.patch" ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2024-03-20 19:20 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-07-07 01:39 Re: Fix search_path for all maintenance commands Jeff Davis <[email protected]> 2023-07-07 03:22 ` Isaac Morland <[email protected]> 2023-07-07 15:42 ` Jeff Davis <[email protected]> 2023-07-13 18:56 ` Jeff Davis <[email protected]> 2023-07-13 19:54 ` Gurjeet Singh <[email protected]> 2023-07-13 20:37 ` David G. Johnston <[email protected]> 2023-07-13 21:00 ` Gurjeet Singh <[email protected]> 2023-07-13 21:07 ` David G. Johnston <[email protected]> 2023-07-15 21:13 ` Noah Misch <[email protected]> 2023-07-13 22:19 ` Jeff Davis <[email protected]> 2024-03-20 19:20 [PATCH v5 1/2] pg_lfind32(): add "overlap" code for remaining elements Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox