Skip to content

Commit ea5ae3a

Browse files
tglsfdctglsfdc
committed
Fix misbehavior of DROP OWNED BY with duplicate polroles entries.
Ordinarily, a pg_policy.polroles array wouldn't list the same role more than once; but CREATE POLICY does not prevent that. If we perform DROP OWNED BY on a role that is listed more than once, RemoveRoleFromObjectPolicy either suffered an assertion failure or encountered a tuple-updated-by-self error. Rewrite it to cope correctly with duplicate entries, and add a CommandCounterIncrement call to prevent the other problem. Per discussion, there's other cleanup that ought to happen here, but this seems like the minimum essential fix. Per bug #17062 from Alexander Lakhin. It's been broken all along, so back-patch to all supported branches. Discussion: https://postgr.es/m/17062-11f471ae3199ca23@postgresql.org
1 parent 4b8b356 commit ea5ae3a

File tree

3 files changed

+46
-23
lines changed

3 files changed

+46
-23
lines changed

‎src/backend/commands/policy.c

Lines changed: 27 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "access/htup.h"
1818
#include "access/htup_details.h"
1919
#include "access/sysattr.h"
20+
#include "access/xact.h"
2021
#include "catalog/catalog.h"
2122
#include "catalog/dependency.h"
2223
#include "catalog/indexing.h"
@@ -424,10 +425,9 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
424425
Oid relid;
425426
Relation rel;
426427
ArrayType *policy_roles;
427-
int num_roles;
428428
Datum roles_datum;
429429
bool attr_isnull;
430-
bool noperm = true;
430+
bool keep_policy = true;
431431

432432
Assert(classid == PolicyRelationId);
433433

@@ -480,31 +480,20 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
480480

481481
policy_roles = DatumGetArrayTypePCopy(roles_datum);
482482

483-
/* We should be removing exactly one entry from the roles array */
484-
num_roles = ARR_DIMS(policy_roles)[0] - 1;
485-
486-
Assert(num_roles >= 0);
487-
488483
/* Must own relation. */
489-
if (pg_class_ownercheck(relid, GetUserId()))
490-
noperm = false; /* user is allowed to modify this policy */
491-
else
484+
if (!pg_class_ownercheck(relid, GetUserId()))
492485
ereport(WARNING,
493486
(errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
494487
errmsg("role \"%s\" could not be removed from policy \"%s\" on \"%s\"",
495488
GetUserNameFromId(roleid, false),
496489
NameStr(((Form_pg_policy) GETSTRUCT(tuple))->polname),
497490
RelationGetRelationName(rel))));
498-
499-
/*
500-
* If multiple roles exist on this policy, then remove the one we were
501-
* asked to and leave the rest.
502-
*/
503-
if (!noperm && num_roles > 0)
491+
else
504492
{
505493
int i,
506494
j;
507495
Oid *roles = (Oid *) ARR_DATA_PTR(policy_roles);
496+
int num_roles = ARR_DIMS(policy_roles)[0];
508497
Datum *role_oids;
509498
char *qual_value;
510499
Node *qual_expr;
@@ -578,16 +567,22 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
578567
else
579568
with_check_qual = NULL;
580569

581-
/* Rebuild the roles array to then update the pg_policy tuple with */
570+
/*
571+
* Rebuild the roles array, without any mentions of the target role.
572+
* Ordinarily there'd be exactly one, but we must cope with duplicate
573+
* mentions, since CREATE/ALTER POLICY historically have allowed that.
574+
*/
582575
role_oids = (Datum *) palloc(num_roles * sizeof(Datum));
583-
for (i = 0, j = 0; i < ARR_DIMS(policy_roles)[0]; i++)
584-
/* Copy over all of the roles which are not the one being removed */
576+
for (i = 0, j = 0; i < num_roles; i++)
577+
{
585578
if (roles[i] != roleid)
586579
role_oids[j++] = ObjectIdGetDatum(roles[i]);
580+
}
581+
num_roles = j;
587582

588-
/* We should have only removed the one role */
589-
Assert(j == num_roles);
590-
583+
/* If any roles remain, update the policy entry. */
584+
if (num_roles > 0)
585+
{
591586
/* This is the array for the new tuple */
592587
role_ids = construct_array(role_oids, num_roles, OIDOID,
593588
sizeof(Oid), true, 'i');
@@ -642,8 +637,17 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
642637

643638
heap_freetuple(new_tuple);
644639

640+
/* Make updates visible */
641+
CommandCounterIncrement();
642+
645643
/* Invalidate Relation Cache */
646644
CacheInvalidateRelcache(rel);
645+
}
646+
else
647+
{
648+
/* No roles would remain, so drop the policy instead */
649+
keep_policy = false;
650+
}
647651
}
648652

649653
/* Clean up. */
@@ -653,7 +657,7 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
653657

654658
heap_close(pg_policy_rel, RowExclusiveLock);
655659

656-
return (noperm || num_roles > 0);
660+
return keep_policy;
657661
}
658662

659663
/*

‎src/test/regress/expected/rowsecurity.out

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3906,6 +3906,15 @@ ERROR: policy "p1" for table "dob_t1" does not exist
39063906
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
39073907
DROP OWNED BY regress_rls_dob_role1;
39083908
DROP POLICY p1 ON dob_t1; -- should succeed
3909+
-- same cases with duplicate polroles entries
3910+
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1 USING (true);
3911+
DROP OWNED BY regress_rls_dob_role1;
3912+
DROP POLICY p1 ON dob_t1; -- should fail, already gone
3913+
ERROR: policy "p1" for table "dob_t1" does not exist
3914+
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
3915+
DROP OWNED BY regress_rls_dob_role1;
3916+
DROP POLICY p1 ON dob_t1; -- should succeed
3917+
-- partitioned target
39093918
CREATE POLICY p1 ON dob_t2 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
39103919
DROP OWNED BY regress_rls_dob_role1;
39113920
DROP POLICY p1 ON dob_t2; -- should succeed

‎src/test/regress/sql/rowsecurity.sql

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1760,6 +1760,16 @@ CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role2 USING
17601760
DROP OWNED BY regress_rls_dob_role1;
17611761
DROP POLICY p1 ON dob_t1; -- should succeed
17621762

1763+
-- same cases with duplicate polroles entries
1764+
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1 USING (true);
1765+
DROP OWNED BY regress_rls_dob_role1;
1766+
DROP POLICY p1 ON dob_t1; -- should fail, already gone
1767+
1768+
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
1769+
DROP OWNED BY regress_rls_dob_role1;
1770+
DROP POLICY p1 ON dob_t1; -- should succeed
1771+
1772+
-- partitioned target
17631773
CREATE POLICY p1 ON dob_t2 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
17641774
DROP OWNED BY regress_rls_dob_role1;
17651775
DROP POLICY p1 ON dob_t2; -- should succeed

0 commit comments

Comments
 (0)