Обсуждение: 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

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 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.
>

Yes, I think you're right. It needs to happen before fireRIRonSubLink,
and only if hasSubLinks is true.

Regards,
Dean

* 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

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

* 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

* 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