agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Álvaro Herrera <alvherre@kurilemu.de>
Subject: [PATCH v3] Tighten ACL check in repack_is_permitted_for_relation()
Date: Mon, 17 Aug 2026 17:53:34 +0200
repack_is_permitted_for_relation() uses pg_class_aclcheck_ext()
to silently skip a concurrently-dropped relation. That's wrong
for a caller that may already hold a lock on the relation whose
ACL is checked, where missing a relation is not fine, and it
makes the single-relation REPACK and CLUSTER cases more brittle.
So only detect a missing relation where that's expected,
following the fix for vacuum_is_permitted_for_relation() in
commit 824d5f6241ea.
The new already_locked behavior is limited to get_tables_to_repack()
and get_tables_to_repack_partitioned(). All other callers of
repack_is_permitted_for_relation() hold a lock on the relation
that prevents it from being concurrently dropped, so this commit
also adds an assertion to that effect.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://www.postgresql.org/message-id/CALj2ACX3pyuRS8%2B%2B6L20cJUMRTf_qbbVp69J1btJ3y6%3D77e5gw%40ma...
---
src/backend/commands/repack.c | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index edff54e734e..66b88e28c2e 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -173,7 +173,8 @@ static List *get_tables_to_repack_partitioned(RepackStmt *stmt,
Relation rel,
MemoryContext permcxt);
static bool repack_is_permitted_for_relation(RepackCommand cmd,
- Oid relid, Oid userid);
+ Oid relid, Oid userid,
+ bool already_locked);
static void apply_concurrent_changes(BufFile *file, ChangeContext *chgcxt);
static void apply_concurrent_insert(Relation rel, TupleTableSlot *slot,
@@ -681,7 +682,7 @@ cluster_rel_recheck(RepackCommand cmd, Relation OldHeap, Oid indexOid,
Assert(CheckRelationLockedByMe(OldHeap, lmode, false));
/* Check that the user still has privileges for the relation */
- if (!repack_is_permitted_for_relation(cmd, tableOid, userid))
+ if (!repack_is_permitted_for_relation(cmd, tableOid, userid, true))
{
relation_close(OldHeap, lmode);
return false;
@@ -2155,7 +2156,7 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
/* noisily skip rels which the user can't process */
if (!repack_is_permitted_for_relation(cmd, index->indrelid,
- GetUserId()))
+ GetUserId(), false))
continue;
/* Use a permanent memory context for the result list */
@@ -2192,7 +2193,7 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
/* noisily skip rels which the user can't process */
if (!repack_is_permitted_for_relation(cmd, class->oid,
- GetUserId()))
+ GetUserId(), false))
continue;
/* Use a permanent memory context for the result list */
@@ -2321,7 +2322,7 @@ get_tables_to_repack_partitioned(RepackStmt *stmt, Relation rel,
* if so.
*/
if (!repack_is_permitted_for_relation(stmt->command, table_oid,
- GetUserId()))
+ GetUserId(), false))
continue;
/* Use a permanent memory context for the result list */
@@ -2341,26 +2342,38 @@ get_tables_to_repack_partitioned(RepackStmt *stmt, Relation rel,
/*
- * Return whether userid has privileges to execute REPACK on relid.
+ * Return whether userid has privileges to execute REPACK/CLUSTER on relid.
*
- * Caller may not have a lock on the relation, so it could have been
- * dropped concurrently. In that case, silently return false.
+ * The relation may already be locked by caller, in which case it cannot
+ * possibly go missing; otherwise it can have been removed recently. If
+ * it's been removed, silently return false. If the relation does exist but
+ * the user doesn't have the required privs, emit a WARNING and return false.
*
- * If the relation does exist but the user doesn't have the required
- * privs, emit a WARNING and return false. Otherwise, return true.
+ * Otherwise, return true.
*/
static bool
-repack_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid)
+repack_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid,
+ bool already_locked)
{
bool is_missing = false;
AclResult result;
char *relname;
Assert(cmd == REPACK_COMMAND_CLUSTER || cmd == REPACK_COMMAND_REPACK);
+ Assert(!already_locked ||
+ CheckRelationOidLockedByMe(relid, AccessShareLock, true));
result = pg_class_aclcheck_ext(relid, userid, ACL_MAINTAIN, &is_missing);
+
+ /*
+ * If the relation was concurrently dropped, nothing to do. This is only
+ * reachable when the caller doesn't already have a lock on the relation.
+ */
if (is_missing)
+ {
+ Assert(!already_locked);
return false;
+ }
if (result == ACLCHECK_OK)
return true;
--
2.47.3
--lagmlhwoe2cxax2o--
view thread (136+ messages) latest in thread
Message-ID: <no-message-id-1372448@localhost>
Permalink: ../../no-message-id-1372448@localhost/
Also on: postgresql.org/message-id/no-message-id-1372448@localhost
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@kurilemu.de
Subject: Re: [PATCH v3] Tighten ACL check in repack_is_permitted_for_relation()
In-Reply-To: <no-message-id-1372448@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox