Skip to content

Commit d21fca0

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 84bee96 commit d21fca0

File tree

3 files changed

+46
-23
lines changed

3 files changed

+46
-23
lines changed

‎src/backend/commands/policy.c

+27-23
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "access/relation.h"
1919
#include "access/sysattr.h"
2020
#include "access/table.h"
21+
#include "access/xact.h"
2122
#include "catalog/catalog.h"
2223
#include "catalog/dependency.h"
2324
#include "catalog/indexing.h"
@@ -425,10 +426,9 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
425426
Oid relid;
426427
Relation rel;
427428
ArrayType *policy_roles;
428-
int num_roles;
429429
Datum roles_datum;
430430
bool attr_isnull;
431-
bool noperm = true;
431+
bool keep_policy = true;
432432

433433
Assert(classid == PolicyRelationId);
434434

@@ -481,31 +481,20 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
481481

482482
policy_roles = DatumGetArrayTypePCopy(roles_datum);
483483

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

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

592-
/* We should have only removed the one role */
593-
Assert(j == num_roles);
594-
587+
/* If any roles remain, update the policy entry. */
588+
if (num_roles > 0)
589+
{
595590
/* This is the array for the new tuple */
596591
role_ids = construct_array(role_oids, num_roles, OIDOID,
597592
sizeof(Oid), true, TYPALIGN_INT);
@@ -646,8 +641,17 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
646641

647642
heap_freetuple(new_tuple);
648643

644+
/* Make updates visible */
645+
CommandCounterIncrement();
646+
649647
/* Invalidate Relation Cache */
650648
CacheInvalidateRelcache(rel);
649+
}
650+
else
651+
{
652+
/* No roles would remain, so drop the policy instead */
653+
keep_policy = false;
654+
}
651655
}
652656

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

658662
table_close(pg_policy_rel, RowExclusiveLock);
659663

660-
return (noperm || num_roles > 0);
664+
return keep_policy;
661665
}
662666

663667
/*

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

+9
Original file line numberDiff line numberDiff line change
@@ -3886,6 +3886,15 @@ ERROR: policy "p1" for table "dob_t1" does not exist
38863886
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
38873887
DROP OWNED BY regress_rls_dob_role1;
38883888
DROP POLICY p1 ON dob_t1; -- should succeed
3889+
-- same cases with duplicate polroles entries
3890+
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1 USING (true);
3891+
DROP OWNED BY regress_rls_dob_role1;
3892+
DROP POLICY p1 ON dob_t1; -- should fail, already gone
3893+
ERROR: policy "p1" for table "dob_t1" does not exist
3894+
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
3895+
DROP OWNED BY regress_rls_dob_role1;
3896+
DROP POLICY p1 ON dob_t1; -- should succeed
3897+
-- partitioned target
38893898
CREATE POLICY p1 ON dob_t2 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
38903899
DROP OWNED BY regress_rls_dob_role1;
38913900
DROP POLICY p1 ON dob_t2; -- should succeed

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

+10
Original file line numberDiff line numberDiff line change
@@ -1757,6 +1757,16 @@ CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role2 USING
17571757
DROP OWNED BY regress_rls_dob_role1;
17581758
DROP POLICY p1 ON dob_t1; -- should succeed
17591759

1760+
-- same cases with duplicate polroles entries
1761+
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1 USING (true);
1762+
DROP OWNED BY regress_rls_dob_role1;
1763+
DROP POLICY p1 ON dob_t1; -- should fail, already gone
1764+
1765+
CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
1766+
DROP OWNED BY regress_rls_dob_role1;
1767+
DROP POLICY p1 ON dob_t1; -- should succeed
1768+
1769+
-- partitioned target
17601770
CREATE POLICY p1 ON dob_t2 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
17611771
DROP OWNED BY regress_rls_dob_role1;
17621772
DROP POLICY p1 ON dob_t2; -- should succeed

0 commit comments

Comments
 (0)