Re: RangeVarGetRelid() - Mailing list pgsql-hackers

On Wed, Dec 7, 2011 at 8:42 AM, Noah Misch <noah@leadboat.com> wrote:
> It narrows the window for race conditions of that genesis, but isn't doing so
> an anti-feature?  Even if not, doing that _only_ in RemoveRelations() is odd.

I dunno.  I was just reluctant to change things without a clear reason
for doing so, and "it's not what we do in other places" didn't seem
like enough.  Really, I'd like to *always* do
AcceptInvalidationMessages() before a name lookup, but I'm not
convinced it's cheap enough for that.

>> Attached please find a patch with some more fixes on this same general
>> theme.  This one tackles renaming of relations, columns, and triggers;
>> and changing the schema of relations.  In these cases, the current
>> code does a permissions check before locking the table (which is good)
>> and uses RangeVarGetRelid() to guard against "cache lookup failure"
>> errors caused by concurrent DDL (also good).  However, if the referent
>> of the name changes during the lock wait, we don't recheck
>> permissions; we just allow the rename or schema change on the basis
>> that the user had permission to do it to the relation that formerly
>> had that name.  While this is pretty minor as security concerns go, it
>> seems best to clean it up, so this patch does that.
>
> I'd suggest limiting callback functions to checks that benefit greatly from
> preceding the lock acquisition.  In these cases, that's only the target object
> ownership check; all other checks can wait for the lock.  Writing correct
> callback code is relatively tricky; we have no lock, so the available catalog
> entries can change arbitrarily often as the function operates.  It's worth the
> trouble of navigating that hazard to keep the mob from freely locking all
> objects.  However, if the owner of "some_table" writes "ALTER VIEW some_table
> RENAME TO foo", I see no commensurate benefit from reporting the relkind
> mismatch before locking the relation.  This would also permit sharing a
> callback among almost all the call sites you've improved so far.  Offhand, I
> think only ReindexIndex() would retain a bespoke callback.

No, I don't think so.  REINDEX INDEX needs special heap locking and
does not reject operations on system tables, REINDEX TABLE does not
reject operations on system tables, LOCK TABLE has special permissions
requirements and does not reject operations on system tables, DROP
TABLE also has special permissions requirements, RENAME ATTRIBUTE is
"normal" (i.e. must own relation, must not apply to system tables),
and RENAME RELATION has an extra permission check.  It might not be
worth having separate callbacks *just* to check the relkind, but I
don't think we have any instances of that in what's already committed
or in this patch.  It's an issue that is on my mind, though; and
perhaps as I keep cranking through the call sites some things will
turn up that can be merged.  It's amazing how many subtly different
permissions requirements there are here, but none of them seem
terribly ill-founded, and, in any case, changing them is a job for
another day if it's to be done at all.

> This patch removes two of the three callers of CheckRelationOwnership(),
> copying some of its logic to two other sites.  I'd suggest fixing the third
> caller (standard_ProcessUtility() for CREATE INDEX) in this same patch.  That
> way, we can either delete the function now or adjust it to permit continued
> sharing of that code under the new regime.

I had the same thought, but wasn't quite sure how to do it.  That code
seems to be making the rather optimistic assumption that you can look
up the same name as many times as you want and get the same answer.
CheckRelationOwnership() looks it up, and then transformIndexStmt()
looks it up again, and then DefineIndex() looks it up again ... twice,
in the case of a concurrent build.  If someone renames a relation with
the same name into a namespace earlier in our search path during all
this, the chances of totally nutty behavior seem pretty good; what
happens if we do phase 2 of the concurrent index build on a different
relation than phase 1?  So I didn't attempt to tackle the problem just
because it looked to me like the code needed a little more TLC than I
had time to give it, but maybe it deserves a second look.

> I like how this patch reduces the arbitrary division of authorization checks
> between alter.c and tablecmds.c.

Thanks.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company