On Wed, Aug 08, 2018 at 03:38:57PM +0000, Bossart, Nathan wrote: > Here are some comments for the latest version of the patch.
Thanks for the review, Nathan! > -truncate_check_rel(Relation rel) > +truncate_check_rel(Oid relid, Form_pg_class reltuple) > > Could we use HeapTupleGetOid(reltuple) instead of passing the OID > separately? If that was a HeapTuple. And it seems to me that we had better make truncate_check_rel() depend directly on a Form_pg_class instead of allowing the caller pass anything and deform the tuple within the routine. > - if (rel->rd_rel->relkind != RELKIND_RELATION && > - rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) > + if (reltuple->relkind != RELKIND_RELATION && > + > + reltuple->relkind != RELKIND_PARTITIONED_TABLE) > > There appears to be an extra empty line here. Fixed. I don't know where this has come from. > +# Test for lock lookup conflicts with TRUNCATE when working on unowned > +# relations, particularly catalogs should trigger an ERROR for all the > +# scenarios present here. > > If possible, it might be worth it to check that TRUNCATE still blocks > when a role has privileges, too. Good idea. I have added more tests for that. Doing a truncate on pg_authid for installcheck was a very bad idea anyway (even if it failed all the time), so I have switched to a custom table, with a GRANT command to control the access with a custom role. > Since the only behavior this patch changes is the order of locking and > permission checks, my vote would be to back-patch. However, since the > REINDEX piece won't be back-patched, I can see why we might not here, > either. This patch is a bit more invasive than the REINDEX one to be honest, and as this is getting qualified as an improvement, at least on this thread, I would be inclined to be more restrictive and just patch HEAD, but not v11. Attached is an updated patch with all your suggestions added. -- Michael
From 4402dd2dd8b7b2672e43ebb2407013520898e8bc Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 9 Aug 2018 12:25:55 +0200
Subject: [PATCH] Refactor TRUNCATE execution to avoid too early lock lookups
A caller of TRUNCATE can perform early lookup obtention which can cause
other sessions to block on the request done, causing potentially DOS
attacks as even a non-privileged user can attempt a truncation of a
critical catalog table to block even all incoming connection attempts.
This commit fixes the case of TRUNCATE so as RangeVarGetRelidExtended is
used with a callback doing the necessary checks at an earlier stage,
avoiding locking issues at early stages, so as a failure happens for
unprivileged users instead of waiting on a lock.
---
src/backend/commands/tablecmds.c | 83 ++++++++++++----
.../isolation/expected/truncate-conflict.out | 99 +++++++++++++++++++
src/test/isolation/isolation_schedule | 1 +
.../isolation/specs/truncate-conflict.spec | 38 +++++++
4 files changed, 203 insertions(+), 18 deletions(-)
create mode 100644 src/test/isolation/expected/truncate-conflict.out
create mode 100644 src/test/isolation/specs/truncate-conflict.spec
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index eb2d33dd86..c9f157d764 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -300,7 +300,10 @@ struct DropRelationCallbackState
#define child_dependency_type(child_is_partition) \
((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
-static void truncate_check_rel(Relation rel);
+static void truncate_check_rel(Oid relid, Form_pg_class reltuple);
+static void truncate_check_activity(Relation rel);
+static void RangeVarCallbackForTruncate(const RangeVar *relation,
+ Oid relId, Oid oldRelId, void *arg);
static List *MergeAttributes(List *schema, List *supers, char relpersistence,
bool is_partition, List **supOids, List **supconstr,
int *supOidCount);
@@ -1336,15 +1339,25 @@ ExecuteTruncate(TruncateStmt *stmt)
bool recurse = rv->inh;
Oid myrelid;
- rel = heap_openrv(rv, AccessExclusiveLock);
- myrelid = RelationGetRelid(rel);
+ myrelid = RangeVarGetRelidExtended(rv, AccessExclusiveLock,
+ 0, RangeVarCallbackForTruncate,
+ NULL);
+
+ /* open the relation, we already hold a lock on it */
+ rel = heap_open(myrelid, NoLock);
+
/* don't throw error for "TRUNCATE foo, foo" */
if (list_member_oid(relids, myrelid))
{
heap_close(rel, AccessExclusiveLock);
continue;
}
- truncate_check_rel(rel);
+
+ /*
+ * RangeVarGetRelidExtended has done some basic checks with its
+ * callback, and only remain session-level checks.
+ */
+ truncate_check_activity(rel);
rels = lappend(rels, rel);
relids = lappend_oid(relids, myrelid);
/* Log this relation only if needed for logical decoding */
@@ -1367,7 +1380,9 @@ ExecuteTruncate(TruncateStmt *stmt)
/* find_all_inheritors already got lock */
rel = heap_open(childrelid, NoLock);
- truncate_check_rel(rel);
+ truncate_check_rel(RelationGetRelid(rel), rel->rd_rel);
+ truncate_check_activity(rel);
+
rels = lappend(rels, rel);
relids = lappend_oid(relids, childrelid);
/* Log this relation only if needed for logical decoding */
@@ -1450,7 +1465,8 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
ereport(NOTICE,
(errmsg("truncate cascades to table \"%s\"",
RelationGetRelationName(rel))));
- truncate_check_rel(rel);
+ truncate_check_rel(relid, rel->rd_rel);
+ truncate_check_activity(rel);
rels = lappend(rels, rel);
relids = lappend_oid(relids, relid);
/* Log this relation only if needed for logical decoding */
@@ -1700,38 +1716,47 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
}
/*
- * Check that a given rel is safe to truncate. Subroutine for ExecuteTruncate
+ * Check that a given relation is safe to truncate. Subroutine for
+ * ExecuteTruncate and RangeVarCallbackForTruncate.
*/
static void
-truncate_check_rel(Relation rel)
+truncate_check_rel(Oid relid, Form_pg_class reltuple)
{
AclResult aclresult;
+ char *relname = NameStr(reltuple->relname);
/*
* Only allow truncate on regular tables and partitioned tables (although,
* the latter are only being included here for the following checks; no
* physical truncation will occur in their case.)
*/
- if (rel->rd_rel->relkind != RELKIND_RELATION &&
- rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+ if (reltuple->relkind != RELKIND_RELATION &&
+ reltuple->relkind != RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
- RelationGetRelationName(rel))));
+ errmsg("\"%s\" is not a table", relname)));
/* Permissions checks */
- aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
- ACL_TRUNCATE);
+ aclresult = pg_class_aclcheck(relid, GetUserId(), ACL_TRUNCATE);
if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind),
- RelationGetRelationName(rel));
+ aclcheck_error(aclresult, get_relkind_objtype(reltuple->relkind),
+ relname);
- if (!allowSystemTableMods && IsSystemRelation(rel))
+ if (!allowSystemTableMods && IsSystemClass(relid, reltuple))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(rel))));
+ relname)));
+}
+/*
+ * Set of extra sanity checks to check if a given relation is safe to
+ * truncate. This is split with truncate_check_rel as
+ * RangeVarCallbackForTruncate can only call the former.
+ */
+static void
+truncate_check_activity(Relation rel)
+{
/*
* Don't allow truncate on temp tables of other backends ... their local
* buffer manager is not going to cope.
@@ -13419,6 +13444,28 @@ RangeVarCallbackOwnsTable(const RangeVar *relation,
aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relId)), relation->relname);
}
+/*
+ * Callback to RangeVarGetRelidExtended() for TRUNCATE processing.
+ */
+static void
+RangeVarCallbackForTruncate(const RangeVar *relation,
+ Oid relId, Oid oldRelId, void *arg)
+{
+ HeapTuple tuple;
+
+ /* Nothing to do if the relation was not found. */
+ if (!OidIsValid(relId))
+ return;
+
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relId));
+ if (!HeapTupleIsValid(tuple)) /* should not happen */
+ elog(ERROR, "cache lookup failed for relation %u", relId);
+
+ truncate_check_rel(relId, (Form_pg_class) GETSTRUCT(tuple));
+
+ ReleaseSysCache(tuple);
+}
+
/*
* Callback to RangeVarGetRelidExtended(), similar to
* RangeVarCallbackOwnsTable() but without checks on the type of the relation.
diff --git a/src/test/isolation/expected/truncate-conflict.out b/src/test/isolation/expected/truncate-conflict.out
new file mode 100644
index 0000000000..2c10f8d40d
--- /dev/null
+++ b/src/test/isolation/expected/truncate-conflict.out
@@ -0,0 +1,99 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_begin s1_tab_lookup s2_auth s2_truncate s1_commit s2_reset
+step s1_begin: BEGIN;
+step s1_tab_lookup: SELECT count(*) >= 0 FROM truncate_tab;
+?column?
+
+t
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s2_truncate: TRUNCATE truncate_tab;
+ERROR: permission denied for table truncate_tab
+step s1_commit: COMMIT;
+step s2_reset: RESET ROLE;
+
+starting permutation: s1_begin s2_auth s2_truncate s1_tab_lookup s1_commit s2_reset
+step s1_begin: BEGIN;
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s2_truncate: TRUNCATE truncate_tab;
+ERROR: permission denied for table truncate_tab
+step s1_tab_lookup: SELECT count(*) >= 0 FROM truncate_tab;
+?column?
+
+t
+step s1_commit: COMMIT;
+step s2_reset: RESET ROLE;
+
+starting permutation: s1_begin s2_auth s1_tab_lookup s2_truncate s1_commit s2_reset
+step s1_begin: BEGIN;
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s1_tab_lookup: SELECT count(*) >= 0 FROM truncate_tab;
+?column?
+
+t
+step s2_truncate: TRUNCATE truncate_tab;
+ERROR: permission denied for table truncate_tab
+step s1_commit: COMMIT;
+step s2_reset: RESET ROLE;
+
+starting permutation: s2_auth s2_truncate s1_begin s1_tab_lookup s1_commit s2_reset
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s2_truncate: TRUNCATE truncate_tab;
+ERROR: permission denied for table truncate_tab
+step s1_begin: BEGIN;
+step s1_tab_lookup: SELECT count(*) >= 0 FROM truncate_tab;
+?column?
+
+t
+step s1_commit: COMMIT;
+step s2_reset: RESET ROLE;
+
+starting permutation: s1_begin s1_tab_lookup s2_grant s2_auth s2_truncate s1_commit s2_reset
+step s1_begin: BEGIN;
+step s1_tab_lookup: SELECT count(*) >= 0 FROM truncate_tab;
+?column?
+
+t
+step s2_grant: GRANT TRUNCATE ON truncate_tab TO regress_truncate_conflict;
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s2_truncate: TRUNCATE truncate_tab; <waiting ...>
+step s1_commit: COMMIT;
+step s2_truncate: <... completed>
+step s2_reset: RESET ROLE;
+
+starting permutation: s1_begin s2_grant s2_auth s2_truncate s1_tab_lookup s1_commit s2_reset
+step s1_begin: BEGIN;
+step s2_grant: GRANT TRUNCATE ON truncate_tab TO regress_truncate_conflict;
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s2_truncate: TRUNCATE truncate_tab;
+step s1_tab_lookup: SELECT count(*) >= 0 FROM truncate_tab;
+?column?
+
+t
+step s1_commit: COMMIT;
+step s2_reset: RESET ROLE;
+
+starting permutation: s1_begin s2_grant s2_auth s1_tab_lookup s2_truncate s1_commit s2_reset
+step s1_begin: BEGIN;
+step s2_grant: GRANT TRUNCATE ON truncate_tab TO regress_truncate_conflict;
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s1_tab_lookup: SELECT count(*) >= 0 FROM truncate_tab;
+?column?
+
+t
+step s2_truncate: TRUNCATE truncate_tab; <waiting ...>
+step s1_commit: COMMIT;
+step s2_truncate: <... completed>
+step s2_reset: RESET ROLE;
+
+starting permutation: s2_grant s2_auth s2_truncate s1_begin s1_tab_lookup s1_commit s2_reset
+step s2_grant: GRANT TRUNCATE ON truncate_tab TO regress_truncate_conflict;
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s2_truncate: TRUNCATE truncate_tab;
+step s1_begin: BEGIN;
+step s1_tab_lookup: SELECT count(*) >= 0 FROM truncate_tab;
+?column?
+
+t
+step s1_commit: COMMIT;
+step s2_reset: RESET ROLE;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index d5594e80e2..48ae740739 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -76,3 +76,4 @@ test: partition-key-update-2
test: partition-key-update-3
test: partition-key-update-4
test: plpgsql-toast
+test: truncate-conflict
diff --git a/src/test/isolation/specs/truncate-conflict.spec b/src/test/isolation/specs/truncate-conflict.spec
new file mode 100644
index 0000000000..bbacd7aa3e
--- /dev/null
+++ b/src/test/isolation/specs/truncate-conflict.spec
@@ -0,0 +1,38 @@
+# Test for lock lookup conflicts with TRUNCATE when working on unowned
+# relations, particularly catalogs should trigger an ERROR for all the
+# scenarios present here.
+
+setup
+{
+ CREATE ROLE regress_truncate_conflict;
+ CREATE TABLE truncate_tab (a int);
+}
+
+teardown
+{
+ DROP TABLE truncate_tab;
+ DROP ROLE regress_truncate_conflict;
+}
+
+session "s1"
+step "s1_begin" { BEGIN; }
+step "s1_tab_lookup" { SELECT count(*) >= 0 FROM truncate_tab; }
+step "s1_commit" { COMMIT; }
+
+session "s2"
+step "s2_grant" { GRANT TRUNCATE ON truncate_tab TO regress_truncate_conflict; }
+step "s2_auth" { SET ROLE regress_truncate_conflict; }
+step "s2_truncate" { TRUNCATE truncate_tab; }
+step "s2_reset" { RESET ROLE; }
+
+# TRUNCATE will directly fail
+permutation "s1_begin" "s1_tab_lookup" "s2_auth" "s2_truncate" "s1_commit" "s2_reset"
+permutation "s1_begin" "s2_auth" "s2_truncate" "s1_tab_lookup" "s1_commit" "s2_reset"
+permutation "s1_begin" "s2_auth" "s1_tab_lookup" "s2_truncate" "s1_commit" "s2_reset"
+permutation "s2_auth" "s2_truncate" "s1_begin" "s1_tab_lookup" "s1_commit" "s2_reset"
+
+# TRUNCATE will block
+permutation "s1_begin" "s1_tab_lookup" "s2_grant" "s2_auth" "s2_truncate" "s1_commit" "s2_reset"
+permutation "s1_begin" "s2_grant" "s2_auth" "s2_truncate" "s1_tab_lookup" "s1_commit" "s2_reset"
+permutation "s1_begin" "s2_grant" "s2_auth" "s1_tab_lookup" "s2_truncate" "s1_commit" "s2_reset"
+permutation "s2_grant" "s2_auth" "s2_truncate" "s1_begin" "s1_tab_lookup" "s1_commit" "s2_reset"
--
2.18.0
signature.asc
Description: PGP signature
