diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d3fca47..d4a15faf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,20 @@ pinned at `1.0-dev` or `1.0-alpha`, each true until the next version shipped. ### Fixed +- Five entry points requested a relation lock before checking that the caller + owns the table. `pgcolumnar.add_projection` takes a `ShareLock`, and + `drop_projection`, `recluster`, `compact_rewrite` and `compact` take a + `ShareUpdateExclusiveLock`; each opened the relation first and validated + ownership after. An unprivileged caller could therefore queue in the lock + manager on a table it has no rights to, blocking readers and writers until it + was refused. Measured against a held `AccessExclusiveLock`: the call reached a + four second lock timeout before the ownership error, where it now returns the + ownership error immediately. Ownership is now checked before the lock, which + is the ordering `pgcolumnar.vacuum`, `vacuum_sorted` and `cluster` already + used. `drop_projection` also reported whether an arbitrary relation was + columnar to a caller who does not own it; it now reports only that they are + not the owner, matching the four entry points beside it. (#749) + - The columnar scan resolved `pgcolumnar.zone_map` once per chunk group per predicate column instead of once per scan. Every group a predicate could exclude ran a relation open with a lock and two catalog name lookups, then diff --git a/src/columnar.h b/src/columnar.h index 8b96f135..51a8ceca 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -445,6 +445,7 @@ extern bool pgcolumnar_enable_end_truncation; /* error unless the current user owns the relation (maintenance/DDL gate) */ extern void PgColumnarRequireTableOwner(Relation rel); +extern void PgColumnarRequireTableOwnerByOid(Oid relid); extern void PgColumnarRequireNoRowSecurity(Oid relid); /* diff --git a/src/columnar_projection.c b/src/columnar_projection.c index a36b05b6..6051ec80 100644 --- a/src/columnar_projection.c +++ b/src/columnar_projection.c @@ -187,6 +187,8 @@ pgcolumnar_add_projection(PG_FUNCTION_ARGS) (errcode(ERRCODE_NAME_TOO_LONG), errmsg("projection name \"%s\" is too long", projname))); + PgColumnarRequireTableOwnerByOid(relid); + /* * ShareLock: block concurrent INSERT/UPDATE/DELETE (RowExclusiveLock) while * we back-fill the projection from existing rows, so no concurrently written @@ -194,7 +196,6 @@ pgcolumnar_add_projection(PG_FUNCTION_ARGS) * unaffected. (A CONCURRENTLY variant is future work.) */ rel = table_open(relid, ShareLock); - PgColumnarRequireTableOwner(rel); storageId = PgColumnarStorageId(rel); existing = PgColumnarListProjections(storageId); @@ -291,6 +292,16 @@ pgcolumnar_drop_projection(PG_FUNCTION_ARGS) relid = PG_GETARG_OID(0); projname = text_to_cstring(PG_GETARG_TEXT_PP(1)); + /* + * Ownership FIRST, then the relation type. Both precede table_open so a + * non-owner never reaches the lock manager, and doing them in this order + * means a non-owner is told only that they are not the owner: asking about + * an arbitrary relation must not report back whether it is columnar. The + * sibling entry points in columnar_vacuum.c and columnar_visibilitymap.c + * are ordered the same way. + */ + PgColumnarRequireTableOwnerByOid(relid); + if (!PgColumnarIsColumnarRelation(relid)) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), @@ -298,7 +309,6 @@ pgcolumnar_drop_projection(PG_FUNCTION_ARGS) get_rel_name(relid)))); rel = table_open(relid, ShareUpdateExclusiveLock); - PgColumnarRequireTableOwner(rel); storageId = PgColumnarStorageId(rel); existing = PgColumnarListProjections(storageId); diff --git a/src/columnar_vacuum.c b/src/columnar_vacuum.c index 925d0613..f97fb9bb 100644 --- a/src/columnar_vacuum.c +++ b/src/columnar_vacuum.c @@ -191,7 +191,7 @@ PgColumnarRequireTableOwner(Relation rel) * not own until its own lock request was resolved, so it has to be turned away * before the lock is requested rather than after it is held. */ -static void +void PgColumnarRequireTableOwnerByOid(Oid relid) { if (!COLUMNAR_TABLE_OWNERCHECK(relid)) @@ -776,6 +776,8 @@ pgcolumnar_recluster(PG_FUNCTION_ARGS) (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("Z-order clustering supports at most 8 columns"))); + PgColumnarRequireTableOwnerByOid(relid); + /* the lazy lock: concurrent reads and writes during the recluster */ rel = table_open(relid, ShareUpdateExclusiveLock); @@ -788,8 +790,6 @@ pgcolumnar_recluster(PG_FUNCTION_ARGS) RelationGetRelationName(rel)))); } - PgColumnarRequireTableOwner(rel); - tupdesc = RelationGetDescr(rel); atts = palloc(ncols * sizeof(AttrNumber)); for (i = 0; i < ncols; i++) @@ -882,6 +882,8 @@ pgcolumnar_compact_rewrite(PG_FUNCTION_ARGS) (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("min_deleted_fraction must be between 0 and 1"))); + PgColumnarRequireTableOwnerByOid(relid); + rel = table_open(relid, ShareUpdateExclusiveLock); if (!PgColumnarIsColumnarRelation(relid)) @@ -893,8 +895,6 @@ pgcolumnar_compact_rewrite(PG_FUNCTION_ARGS) RelationGetRelationName(rel)))); } - PgColumnarRequireTableOwner(rel); - /* dev/test: hold SUEL so the daemon's yield is observable (#415) */ pgcolumnar_maintenance_hold(); @@ -1859,6 +1859,8 @@ pgcolumnar_compact(PG_FUNCTION_ARGS) (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED), errmsg("table name cannot be null"))); + PgColumnarRequireTableOwnerByOid(relid); + /* the lazy lock: concurrent reads and writes are allowed during compaction */ rel = table_open(relid, ShareUpdateExclusiveLock); @@ -1871,8 +1873,6 @@ pgcolumnar_compact(PG_FUNCTION_ARGS) RelationGetRelationName(rel)))); } - PgColumnarRequireTableOwner(rel); - /* self-heal a truncate crash-residual so the no-overlap assert holds eagerly, * even though compact does not reuse (see PgColumnarReconcileFreeList) */ PgColumnarReconcileFreeList(rel); diff --git a/test/vacuum_lock_privilege.sh b/test/vacuum_lock_privilege.sh index c716a34a..e87e93d0 100755 --- a/test/vacuum_lock_privilege.sh +++ b/test/vacuum_lock_privilege.sh @@ -81,6 +81,46 @@ check "vacuum_sorted refuses a non-owner before taking the exclusive lock" \ check "cluster refuses a non-owner before taking the exclusive lock" \ "$(outcome_under_contention "SELECT pgcolumnar.cluster('victim'::regclass, 'id');")" "owner" +# ---- #749: the same ordering for the five maintenance/projection entries ----- +# +# add_projection takes ShareLock and the other four take +# ShareUpdateExclusiveLock, all of which conflict with the AccessExclusiveLock +# held above, so an unfixed caller queues exactly as vacuum did. Asserting only +# the SQLSTATE would NOT be a removal proof: on unfixed main these calls are +# also eventually refused, just after the lock request. Contention is what +# separates the two orderings, which is the argument this file already makes. +check "add_projection refuses a non-owner before taking its lock (#749)" \ + "$(outcome_under_contention "SELECT pgcolumnar.add_projection('victim'::regclass, 'p1', ARRAY['id']);")" "owner" +check "drop_projection refuses a non-owner before taking its lock (#749)" \ + "$(outcome_under_contention "SELECT pgcolumnar.drop_projection('victim'::regclass, 'p1');")" "owner" +check "recluster refuses a non-owner before taking its lock (#749)" \ + "$(outcome_under_contention "SELECT pgcolumnar.recluster('victim'::regclass, 'id');")" "owner" +check "compact_rewrite refuses a non-owner before taking its lock (#749)" \ + "$(outcome_under_contention "SELECT pgcolumnar.compact_rewrite('victim'::regclass, 0.2);")" "owner" +check "compact refuses a non-owner before taking its lock (#749)" \ + "$(outcome_under_contention "SELECT pgcolumnar.compact('victim'::regclass);")" "owner" + +# A non-owner asking about a relation they do not own must learn only that they +# are not the owner. drop_projection checked the relation TYPE first, so it +# reported "is not a columnar table" for an arbitrary heap relation to a caller +# with no rights to it, while the four entries beside it did not (#749 review). +psql_run "CREATE TABLE heapvictim (id int);" >/dev/null 2>&1 +psql_run "REVOKE ALL ON heapvictim FROM PUBLIC;" >/dev/null 2>&1 +notowner_heap() { # sql -> owner | typedisclosed | other + local out + out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U t_unpriv \ + -d "$PGC_DB" -Atq -v ON_ERROR_STOP=0 -c "$1" 2>&1)" + case "$out" in + *"must be owner"*|*"permission denied for"*) echo owner ;; + *"is not a columnar table"*) echo typedisclosed ;; + *) echo "other:${out:0:40}" ;; + esac +} +check "drop_projection tells a non-owner nothing about the relation type (#749)" \ + "$(notowner_heap "SELECT pgcolumnar.drop_projection('heapvictim'::regclass, 'p1');")" "owner" +check "compact does the same, which is the behaviour drop_projection now matches" \ + "$(notowner_heap "SELECT pgcolumnar.compact('heapvictim'::regclass);")" "owner" + # The caller must never have entered the lock queue: no ungranted request from it. check "the refused caller left no lock request queued on victim" \ "$(q "SELECT count(*) FROM pg_locks l JOIN pg_class c ON c.oid=l.relation JOIN pg_stat_activity a ON a.pid=l.pid WHERE c.relname='victim' AND a.usename='t_unpriv' AND NOT l.granted;")" "0" @@ -93,4 +133,12 @@ wait "$holder_pid" 2>/dev/null || true check "the owner can still vacuum the table" \ "$(as_super -c "SELECT pgcolumnar.vacuum('victim'::regclass);" | grep -c 'ERROR')" "0" +# The positive controls for the #749 arms. Without these, a guard that refused +# every caller would satisfy every deny arm above. +check "the owner can still compact the table (#749 control)" \ + "$(as_super -c "SELECT pgcolumnar.compact('victim'::regclass);" | grep -c 'ERROR')" "0" +check "the owner can still add and drop a projection (#749 control)" \ + "$(as_super -c "SELECT pgcolumnar.add_projection('victim'::regclass, 'p1', ARRAY['id']);" \ + -c "SELECT pgcolumnar.drop_projection('victim'::regclass, 'p1');" | grep -c 'ERROR')" "0" + pgc_summary