| From: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
|---|---|
| To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
| Cc: | Jeff Davis <pgsql(at)j-davis(dot)com>, Roman Eskin <r(dot)eskin(at)arenadata(dot)io>, Michael Paquier <michael(at)paquier(dot)xyz>, Alexander Lakhin <exclusion(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Subject: | Re: Avoid orphaned objects dependencies, take 3 |
| Date: | 2026-05-18 12:14:34 |
| Message-ID: | agsCqlLTytZCudMv@bdtpg |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
On Wed, May 13, 2026 at 04:20:21PM -0400, Robert Haas wrote:
> On Tue, Apr 28, 2026 at 7:17 AM Bertrand Drouvot
> <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> > 0003: Add Assert guard to detect permission check before lock regressions
> >
> > Add instrumentation under USE_ASSERT_CHECKING to detect cases where object_aclcheck()
> > is called on a referenced object before a lock is held on it, which would widen
> > the TOCTOU window between the permission check and the dependency recording.
>
> I really like the idea of having some kind of cross-check system that
> can detect future (or current) coding mistakes.
Thanks for the feedback! BTW, it detected a new one due to 4793fc41f82, so v21
attached does fix it to make the CI green.
> But what I wonder
> about this mechanism is: should we instead be insisting that we take a
> lock and check permissions on every dependency? Is it an error to
> record a dependency on an object without any sort of permissions
> check?
I'm not sure. For example, currently, without execute privilege on myfunc(), one
could create a view like:
CREATE VIEW v1 AS SELECT myfunc(a) FROM t1;
so that the dependency is recorded.
The execution permission is checked when the view is queried. I don't think this
example is a bug, so that I'm doubtful about insisting that we take a lock and
check permissions on every dependency.
> Also, I think the mechanism might not be entirely safe. ProcessUtility
> can result in executing user-defined functions which could
> theoretically run other DDL and then it seems like this code would get
> confused.
The assert fires when an aclcheck was tracked and the lock is not held. Since
the locks are transaction-scoped, any lock acquired during the statement persists,
so the assert passes regardless of nesting. So that looks not possible to get false
positives (assert fires) due to user-defined functions running other DDL.
The concern would be false negatives (missed detection) due to the reset wiping
the outer DDL's entries.
I believe, that's theoretically possible only if:
- The outer DDL has a P1 bug (aclcheck without lock on object X)
- A user-defined function happens to run DDL that also aclchecks and locks the same
object X
- The user-defined function DDL's lock satisfies the assert for the outer DDL
But then, the bug would be caught in normal testing without the user-defined
function being present.
So I'm not sure how this code could get confused. Do you have an example of what
you have in mind?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
| Attachment | Content-Type | Size |
|---|---|---|
| v21-0001-Avoid-orphaned-objects-dependencies.patch | text/x-diff | 22.7 KB |
| v21-0002-Lock-referenced-objects-before-permission-checks.patch | text/x-diff | 33.6 KB |
| v21-0003-Add-Assert-guard-to-detect-permission-check-befo.patch | text/x-diff | 9.5 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Thom Brown | 2026-05-18 12:29:23 | Re: [PATCH] Fix psql tab completion for REPACK boolean options |
| Previous Message | Alexander Korotkov | 2026-05-18 12:04:22 | Re: Fix SPLIT PARTITION bound-overlap bug and other improvements |