Hi,
On Thu, May 22, 2025 at 09:40:47AM -0400, Robert Haas wrote:
> On Thu, May 22, 2025 at 8:15 AM Bertrand Drouvot
> <bertranddrouvot...@gmail.com> wrote:
> > That would probably address Robert's concern [1] "acquiring two locks on
> > the same
> > object with different lock modes where we should really only acquire one"
> > but
> > probably not this one "I think it might result in acquiring the
> > locks on those other objects at a subtly wrong time (leading to race
> > conditions)".
> >
> > For the latter I'm not sure how that could be a subtly wrong time or how
> > could
> > we determine what a subtly wrong time is (to avoid taking the lock).
>
> Well, I admit I'm not quite sure there is any timing problem here. But
> I think it's possible. For example, consider RangeVarGetRelidExtended,
> and in particular the callback argument. If we do permissions checking
> before locking the relation, the results can change before we actually
> lock the relation; if we do it afterward, users can contrive to hold
> locks on relations for which they don't have permissions. That rather
> ornate callback system allows us to have the best of both worlds. That
> system is also concerned with the possibility that we do a name -> OID
> translation before holding a lock and by the time we acquire the lock,
> concurrent DDL has happened and the answer we got is no longer the
> right answer. We've had security holes due to such things.
Yeah I just looked at 2ad36c4e44c and see what issues it solved.
>
> Now I'm not entirely sure that either of those things are issues here.
> My first guess is that name lookup races are not an issue here,
> because we're following OID links,
Yeah, I think the same and think that checking that the object(s) still exist
once we tried to get the lock is probably enough (as done in the patch versions
that have been shared).
> but permissions checks seem like
> they might be an issue.
I agree that we might end up locking an object we don't have the permission
on.
I just looked at 2 examples.
1.
CREATE MATERIALIZED VIEW test_maint_mv2 AS SELECT fn(i) FROM test_maint;
Would call recordMultipleDependencies() with the function as a referenced
object:
Breakpoint 3, recordMultipleDependencies
123
(gdb) p *depender
$3 = {classId = 2618, objectId = 16468, objectSubId = 0}
(gdb) p *referenced
$4 = {classId = 1255, objectId = 16388, objectSubId = 0}
postgres=# select * from pg_identify_object(1255,16388,0);
type | schema | name | identity
----------+--------+------+--------------------
function | public | | public.fn(integer)
(1 row)
But would check the permissions after:
Breakpoint 1, object_aclcheck (classid=1255, objectid=16388, roleid=16384,
mode=128)
3823 return object_aclcheck_ext(classid, objectid, roleid, mode,
NULL);
Means that in that case we'd put a lock on the function *before* checking for
the permissions.
2.
OTOH it's also possible the other way around:
CREATE FUNCTION myschema1.foo() RETURNS int AS 'select 1' LANGUAGE sql;
Would call object_aclcheck() on the schema:
Breakpoint 1, object_aclcheck (classid=2615, objectid=16435, roleid=10,
mode=512)
3823 return object_aclcheck_ext(classid, objectid, roleid, mode,
NULL);
postgres=# select * from pg_identify_object(2615,16435,0);
type | schema | name | identity
--------+--------+-----------+-----------
schema | | myschema1 | myschema1
(1 row)
before adding the dependency:
Breakpoint 3, recordMultipleDependencies
123
(gdb) p *referenced
$1 = {classId = 2615, objectId = 16435, objectSubId = 0}
> We might not decide to do something as
> elaborate as we did with RangeVarGetRelidExtended
So, we currently have 2 patterns:
P1: permission checks on a referenced object is done before we call
recordMultipleDependencies()
P2: permission checks on a referenced object is done after we call
recordMultipleDependencies()
So, if we add locking in recordMultipleDependencies() then I think that P2 is
worst
than P1 (there is still the risk that the permissions changed by the time
we reach recordMultipleDependencies() though).
As also stated in RangeVarGetRelidExtended()'s comment:
"
and it's really best to check permissions * before locking anything!
"
We could imagine doing the permissions check in recordMultipleDependencies() (in
more or less the same way as RangeVarGetRelidExtended() is doing with callback)
because only the recordMultipleDependencies()'s "caller" could know what kind of
check is needed. But that would not work for the same reasons as Jeff's proposal
does not fly (we may manipulate multiple referenced objects that would need
distinct callbacks...).
So, it looks like that we may need some refactoring of the existing code to
ensure that the permissions checks on the referenced objects are done before
the lock is acquired (before recordMultipleDependencies() is called?).
I propose to first try to list the cases where P2 is in action (and I think
those
will be the ones that need special attention that Jeff is asking for in [1]).
And then discuss once we have the cases in hand. Thoughts?
[1]:
https://www.postgresql.org/message-id/d721011cd3ec3aedd57b193ef10cf541f50df325.camel%40j-davis.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
