Re: RangeVarGetRelid()

On Tue, Dec 06, 2011 at 04:02:31PM -0500, Robert Haas wrote:
> On Mon, Dec 5, 2011 at 2:09 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> >> ? ? ? ? ? ? ? AcceptInvalidationMessages();
> >
> > The above call can go away, now.
>
> Doesn't that still protect us against namespace-shadowing issues?
> RangeVarGetRelid doesn't actually AcceptInvalidationMessages() at the
> top.

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.

FWIW, this is fairly independent of your latest patches -- I just happened to
notice it due to the incidental quotation.

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

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 like how this patch reduces the arbitrary division of authorization checks
between alter.c and tablecmds.c.

Thanks,
nm