AcquireRewriteLocks/acquireLocksOnSubLinks vs. rowsecurity
Hi,
The locking around rowsecurity policy expressions seems to be
insufficient:
SELECT * FROM document WHERE f_leak(dtitle) ORDER BY did;
WARNING: RelationIdGetRelation(247984) without holding lock on the relation
WARNING: relation_open(247984, NoLock) of relation "uaccount" without previously held lock
I don't know the relevant code well. But as far as I can see that's
because normally the expectation is that relevant locks have either been
taken by the parser or by AcquireRewriteLocks(). But before
static Query *
fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
{
...
/*
* Fetch any new security quals that must be applied to this RTE.
*/
get_row_security_policies(parsetree, parsetree->commandType, rte,
rt_index, &securityQuals, &withCheckOptions,
&hasRowSecurity, &hasSubLinks);
if (securityQuals != NIL || withCheckOptions != NIL)
{
...
if (hasSubLinks)
{
...
expression_tree_walker((Node *) securityQuals,
fireRIRonSubLink, (void *) activeRIRs);
...
}
rte->securityQuals = list_concat(securityQuals,
rte->securityQuals);
neither will have acquired relevant locks. The parser because it doesn't
know about rowsecurity, AcquireRewriteLocks/acquireLocksOnSubLinks
because rte->securityQuals wan't even set and range_table_walker() uses
that.
Istmt that something like
context.for_execute = true;
acquireLocksOnSubLinks((Node *) securityQuals, &context);
acquireLocksOnSubLinks((Node *) withCheckOptions, &context);
needs to be added to that code.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 27 August 2015 at 13:49, Andres Freund <andres@anarazel.de> wrote:
Hi,
The locking around rowsecurity policy expressions seems to be
insufficient:
SELECT * FROM document WHERE f_leak(dtitle) ORDER BY did;
WARNING: RelationIdGetRelation(247984) without holding lock on the relation
WARNING: relation_open(247984, NoLock) of relation "uaccount" without previously held lockI don't know the relevant code well. But as far as I can see that's
because normally the expectation is that relevant locks have either been
taken by the parser or by AcquireRewriteLocks(). But beforestatic Query *
fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
{
...
/*
* Fetch any new security quals that must be applied to this RTE.
*/
get_row_security_policies(parsetree, parsetree->commandType, rte,
rt_index, &securityQuals, &withCheckOptions,
&hasRowSecurity, &hasSubLinks);if (securityQuals != NIL || withCheckOptions != NIL)
{
...
if (hasSubLinks)
{
...
expression_tree_walker((Node *) securityQuals,
fireRIRonSubLink, (void *) activeRIRs);
...
}rte->securityQuals = list_concat(securityQuals,
rte->securityQuals);neither will have acquired relevant locks. The parser because it doesn't
know about rowsecurity, AcquireRewriteLocks/acquireLocksOnSubLinks
because rte->securityQuals wan't even set and range_table_walker() uses
that.Istmt that something like
context.for_execute = true;
acquireLocksOnSubLinks((Node *) securityQuals, &context);
acquireLocksOnSubLinks((Node *) withCheckOptions, &context);
needs to be added to that code.
Yes, I think you're right. It needs to happen before fireRIRonSubLink,
and only if hasSubLinks is true.
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
On 27 August 2015 at 13:49, Andres Freund <andres@anarazel.de> wrote:
The locking around rowsecurity policy expressions seems to be
insufficient:
SELECT * FROM document WHERE f_leak(dtitle) ORDER BY did;
WARNING: RelationIdGetRelation(247984) without holding lock on the relation
WARNING: relation_open(247984, NoLock) of relation "uaccount" without previously held lock
[...]
Istmt that something like
context.for_execute = true;
acquireLocksOnSubLinks((Node *) securityQuals, &context);
acquireLocksOnSubLinks((Node *) withCheckOptions, &context);
needs to be added to that code.Yes, I think you're right. It needs to happen before fireRIRonSubLink,
and only if hasSubLinks is true.
Attached appears to fix this for the RLS case from my testing.
Any comments?
Barring concerns, I'll push this later today and back-patch to 9.5.
Thanks!
Stephen
Attachments:
fix-rls-locking.patchtext/x-diff; charset=us-asciiDownload
From 7ac58a62338103338b6907fc7ea89f9afb9a0e53 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Fri, 28 Aug 2015 08:10:22 -0400
Subject: [PATCH] Ensure locks are acquired on RLS-added relations
During fireRIRrules(), get_row_security_policies can add to
securityQuals and withCheckOptions. Make sure to lock any relations
added at that point and before firing RIR rules on those expressions.
Back-patch to 9.5 where RLS was added.
---
src/backend/rewrite/rewriteHandler.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 1734e48..fbc0c57 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1787,6 +1787,8 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
{
if (hasSubLinks)
{
+ acquireLocksOnSubLinks_context context;
+
/*
* Recursively process the new quals, checking for infinite
* recursion.
@@ -1799,6 +1801,20 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
activeRIRs = lcons_oid(RelationGetRelid(rel), activeRIRs);
+ /*
+ * get_row_security_policies just added to securityQuals and/or
+ * withCheckOptions, and there were SubLinks, so make sure
+ * we lock any relations which were added as a result.
+ */
+ context.for_execute = true;
+ (void) acquireLocksOnSubLinks((Node *) securityQuals, &context);
+ (void) acquireLocksOnSubLinks((Node *) withCheckOptions,
+ &context);
+
+ /*
+ * Now that we have the locks on anything added by
+ * get_row_security_policies, fire any RIR rules for them.
+ */
expression_tree_walker((Node *) securityQuals,
fireRIRonSubLink, (void *) activeRIRs);
--
1.9.1
On 2015-08-28 08:49:24 -0400, Stephen Frost wrote:
+ /* + * get_row_security_policies just added to securityQuals and/or + * withCheckOptions, and there were SubLinks, so make sure + * we lock any relations which were added as a result. + */
Very minor comment: Strictly speaking the quals/wces haven't yet been
added to the Query, that happens only few lines down. I think it makes
sense to mention that we normally rely on the parser to acquire locks,
but that can't work here since sec quals/wces aren't visible to the
parser.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Andres Freund (andres@anarazel.de) wrote:
On 2015-08-28 08:49:24 -0400, Stephen Frost wrote:
+ /* + * get_row_security_policies just added to securityQuals and/or + * withCheckOptions, and there were SubLinks, so make sure + * we lock any relations which were added as a result. + */Very minor comment: Strictly speaking the quals/wces haven't yet been
added to the Query, that happens only few lines down. I think it makes
sense to mention that we normally rely on the parser to acquire locks,
but that can't work here since sec quals/wces aren't visible to the
parser.
Ok, I'll add a comment to that effect.
Thanks!
Stephen
* Andres Freund (andres@anarazel.de) wrote:
On 2015-08-28 08:49:24 -0400, Stephen Frost wrote:
+ /* + * get_row_security_policies just added to securityQuals and/or + * withCheckOptions, and there were SubLinks, so make sure + * we lock any relations which were added as a result. + */Very minor comment: Strictly speaking the quals/wces haven't yet been
added to the Query, that happens only few lines down. I think it makes
sense to mention that we normally rely on the parser to acquire locks,
but that can't work here since sec quals/wces aren't visible to the
parser.
Better?
Thanks!
Stephen
Attachments:
fix-rls-locking-v2.patchtext/x-diff; charset=us-asciiDownload
From 4cd1a52b1a869e2357f7cf0a65788883690a89b7 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Fri, 28 Aug 2015 08:10:22 -0400
Subject: [PATCH] Ensure locks are acquired on RLS-added relations
During fireRIRrules(), get_row_security_policies can add to
securityQuals and withCheckOptions. Make sure to lock any relations
added at that point and before firing RIR rules on those expressions.
Back-patch to 9.5 where RLS was added.
---
src/backend/rewrite/rewriteHandler.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 1734e48..a238cff 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1787,6 +1787,8 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
{
if (hasSubLinks)
{
+ acquireLocksOnSubLinks_context context;
+
/*
* Recursively process the new quals, checking for infinite
* recursion.
@@ -1799,6 +1801,23 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
activeRIRs = lcons_oid(RelationGetRelid(rel), activeRIRs);
+ /*
+ * get_row_security_policies just passed back securityQuals
+ * and/or withCheckOptions, and there were SubLinks, make sure
+ * we lock any relations which are referenced.
+ *
+ * These locks would normally be acquired by the parser, but
+ * securityQuals and withCheckOptions are added post-parsing.
+ */
+ context.for_execute = true;
+ (void) acquireLocksOnSubLinks((Node *) securityQuals, &context);
+ (void) acquireLocksOnSubLinks((Node *) withCheckOptions,
+ &context);
+
+ /*
+ * Now that we have the locks on anything added by
+ * get_row_security_policies, fire any RIR rules for them.
+ */
expression_tree_walker((Node *) securityQuals,
fireRIRonSubLink, (void *) activeRIRs);
--
1.9.1
* Andres Freund (andres@anarazel.de) wrote:
On 2015-08-28 08:49:24 -0400, Stephen Frost wrote:
+ /* + * get_row_security_policies just added to securityQuals and/or + * withCheckOptions, and there were SubLinks, so make sure + * we lock any relations which were added as a result. + */Very minor comment: Strictly speaking the quals/wces haven't yet been
added to the Query, that happens only few lines down. I think it makes
sense to mention that we normally rely on the parser to acquire locks,
but that can't work here since sec quals/wces aren't visible to the
parser.
Pushed.
Will work on the rewriteTargetView fix.
Thanks!
Stephen