Fix unsafe coding in ResourceOwnerReleaseAll()
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t248609psql -h localhost -U postgresBuilt from patchset v4 (message #4), August 23, 2026 at 01:52 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t248609_4 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t248609_4 && git checkout t248609_4Patchset v4 (message #4) is on t248609_4
I wondered why the example shown in [1]/messages/by-id/19527-6e7686960c6dce78@postgresql.org led to a double-free crash
rather than a clean failure. On investigation, the problem is that
ResourceOwnerReleaseAll() calls the resource kind's ReleaseResource
method before deleting the resource from the owner, rather than
afterwards as the code in ResourceOwnerReleaseAllOfKind() does.
So if we throw an error inside ReleaseResource, the subsequent abort
cleanup comes back and does resource owner cleanup again, and we
try to delete the item again. Kaboom.
The attached patch converts the bug example into a clean failure,
even with the recent bug fix in pgcrypto undone:
regression=# SELECT encrypt_iv(
repeat('A', 1073741308)::bytea,
decode('00112233445566778899aabbccddeeff', 'hex'),
decode('000102030405060708090a0b0c0d0e0f', 'hex'),
'aes'
);
ERROR: invalid memory alloc request size 1073741824
WARNING: AbortTransaction while in ABORT state
ERROR: ResourceOwnerForget called for pgcrypto OpenSSL cipher handle after release started
regression=#
Of course, this might lead to leaking the resource we wished to free.
But that's better than crashing, or at least that's the value judgment
we made long ago in the original ResourceOwner code.
regards, tom lane
Attachments:
make-ResourceOwnerReleaseAll-safer.patchtext/x-diff; charset=us-ascii; name=make-ResourceOwnerReleaseAll-safer.patchDownload+15-5
Hi Tom,
Thank you for proposing this change.
Of course, this might lead to leaking the resource we wished to free.
But that's better than crashing, or at least that's the value judgment
we made long ago in the original ResourceOwner code.
Another approach would be to remove the resource from the resource
owner's list directly within the ResourceRelease callbacks, just
before the resource is released.
This makes the fix specific to a resource type rather than applying it
across all resource types. This would be similar to how it's done in
ReleaseCatCacheWithOwner.
That said, the patch looks good because it makes the code consistent
with ResourceOwnerReleaseAllOfKind() and avoids the crash.
Unrelated to the patch but I noticed a typo in the ERROR message in
ResourceOwnerReleaseAllOfKind(). If we
want a check similar to ResourceOwnerForget in this function, we
should edit the ERROR message with the correct
function name.
I can propose a patch for this in a separate thread, if you'd prefer.
The error message is as follows:
void
ResourceOwnerReleaseAllOfKind(ResourceOwner owner, const ResourceOwnerDesc *kind)
{
/* Mustn't call this after we have already started releasing resources. */
if (owner->releasing)
elog(ERROR, "ResourceOwnerForget called for %s after release started", kind->name);
Thank you,
Rahila Syed
Rahila Syed <rahilasyed90@gmail.com> writes:
Unrelated to the patch but I noticed a typo in the ERROR message in
ResourceOwnerReleaseAllOfKind().
/* Mustn't call this after we have already started releasing resources. */
if (owner->releasing)
elog(ERROR, "ResourceOwnerForget called for %s after release started", kind->name);
Hmm, that definitely looks like a message that was transposed from
someplace else without much thought. It might be worth tracing the
git history to see how it got to be like that.
More: unless I'm missing something, ResourceOwnerReleaseAllOfKind
is called only from plancache.c's ReleaseAllPlanCacheRefsInOwner,
which is called only in some very random-looking ways in plpgsql.
I wonder whether there's not a bigger cleanup project indicated here.
When I posted before, I thought that ResourceOwnerReleaseAllOfKind had
a direct lineage to the old ResourceOwner code, but now I'm thinking
maybe it shouldn't exist at all. Why should plpgsql be taking special
care for particular kinds of resource entries, and why should it
suppose that it owns all instances of that kind within that resowner?
regards, tom lane
Hi,
More: unless I'm missing something, ResourceOwnerReleaseAllOfKind
is called only from plancache.c's ReleaseAllPlanCacheRefsInOwner,
which is called only in some very random-looking ways in plpgsql.
I wonder whether there's not a bigger cleanup project indicated here.
When I posted before, I thought that ResourceOwnerReleaseAllOfKind had
a direct lineage to the old ResourceOwner code, but now I'm thinking
maybe it shouldn't exist at all. Why should plpgsql be taking special
care for particular kinds of resource entries, and why should it
suppose that it owns all instances of that kind within that resowner?
I agree that ResourceOwnerReleaseAllOfKind can be removed
After digging into the resource owner code, it looks like a reason for
having the ResourceOwnerReleaseAllOfKind variant is to allow releasing
all of an owner's resources without going through the three-phase
release mechanism that ResourceOwnerRelease normally enforces
(BEFORE_LOCKS -> LOCKS -> AFTER_LOCKS). That phase ordering isn't
needed if a resource owner only ever holds one kind of resource and
has no children holding other kinds.
The resource owners that ResourceOwnerReleaseAllOfKind is actually
called on (plpgsql's procedure-lifespan owner, the DO-block
simple-expression owner, and the shared simple-expression owner) are
all created specifically to retain resources across transaction
COMMIT/ROLLBACK boundaries. In practice they only ever hold plan-cache
refcounts, so a three-phase release is not required for them. These
owners are deleted immediately after the ResourceOwnerReleaseAllOfKind
call, which shows the intent at each call site is "release everything
this owner holds," rather than to "release only resources of this one
kind."
I tested this by replacing all four call sites with three explicit
ResourceOwnerRelease() calls (one per phase) instead of the single
ResourceOwnerReleaseAllOfKind() call. This passes the plpgsql tests
and the regression suite without crashes or assertion failures. (patch
attached).
The drawback with this approach is that it takes three calls to
release the owners.
Since plan-cache refs are registered at RESOURCE_RELEASE_AFTER_LOCKS,
the BEFORE_LOCKS and LOCKS calls are no-ops for these owners, but are
still required to satisfy ResourceOwnerRelease's internal
phase-ordering assertions.
One possible refactoring: pass a flag to ResourceOwnerRelease (or
ResourceOwnerReleaseAll) indicating it does not need to respect phase
ordering and can just release everything the owner holds in one pass.
That would let a caller drain a standalone, single-kind owner in one
call instead of three.
If we had that, I don't think we would need
ResourceOwnerReleaseAllOfKind — unless a future use case requires
releasing resources of one particular kind from an owner that contains
different kinds of resources or has children holding different kinds
of resources belonging to different phases.
Removing ResourceOwnerReleaseAllOfKind will also help get rid of one
of the flags
"releasing" or "sorted" in ResourceOwnerData.
Thank you,
Rahila Syed
On 12/08/2026 11:27, Rahila Syed wrote:
More: unless I'm missing something, ResourceOwnerReleaseAllOfKind
is called only from plancache.c's ReleaseAllPlanCacheRefsInOwner,
which is called only in some very random-looking ways in plpgsql.
I wonder whether there's not a bigger cleanup project indicated here.
When I posted before, I thought that ResourceOwnerReleaseAllOfKind had
a direct lineage to the old ResourceOwner code, but now I'm thinking
maybe it shouldn't exist at all. Why should plpgsql be taking special
care for particular kinds of resource entries, and why should it
suppose that it owns all instances of that kind within that resowner?I agree that ResourceOwnerReleaseAllOfKind can be removed
After digging into the resource owner code, it looks like a reason for
having the ResourceOwnerReleaseAllOfKind variant is to allow releasing
all of an owner's resources without going through the three-phase
release mechanism that ResourceOwnerRelease normally enforces
(BEFORE_LOCKS -> LOCKS -> AFTER_LOCKS). That phase ordering isn't
needed if a resource owner only ever holds one kind of resource and
has no children holding other kinds.The resource owners that ResourceOwnerReleaseAllOfKind is actually
called on (plpgsql's procedure-lifespan owner, the DO-block
simple-expression owner, and the shared simple-expression owner) are
all created specifically to retain resources across transaction
COMMIT/ROLLBACK boundaries. In practice they only ever hold plan-cache
refcounts, so a three-phase release is not required for them. These
owners are deleted immediately after the ResourceOwnerReleaseAllOfKind
call, which shows the intent at each call site is "release everything
this owner holds," rather than to "release only resources of this one
kind."I tested this by replacing all four call sites with three explicit
ResourceOwnerRelease() calls (one per phase) instead of the single
ResourceOwnerReleaseAllOfKind() call. This passes the plpgsql tests
and the regression suite without crashes or assertion failures. (patch
attached).
The drawback with this approach is that it takes three calls to
release the owners.
Since plan-cache refs are registered at RESOURCE_RELEASE_AFTER_LOCKS,
the BEFORE_LOCKS and LOCKS calls are no-ops for these owners, but are
still required to satisfy ResourceOwnerRelease's internal
phase-ordering assertions.One possible refactoring: pass a flag to ResourceOwnerRelease (or
ResourceOwnerReleaseAll) indicating it does not need to respect phase
ordering and can just release everything the owner holds in one pass.
That would let a caller drain a standalone, single-kind owner in one
call instead of three.
+1 for having a function that just releases all resources in a resource
owner in one call. I don't think it's performance critical so it could
just call ResourceOwnerRelease() three times.
ReleaseAuxProcessResources() could make use of it too.
If we had that, I don't think we would need
ResourceOwnerReleaseAllOfKind — unless a future use case requires
releasing resources of one particular kind from an owner that contains
different kinds of resources or has children holding different kinds
of resources belonging to different phases.
+1 for removing it. I hope we don't need it in the future, it feels like
a ugly wart in the first place.
Removing ResourceOwnerReleaseAllOfKind will also help get rid of one
of the flags "releasing" or "sorted" in ResourceOwnerData.
You still need those flags when you release in phases. I don't see us
getting rid of the three phases in the usual transaction-scoped resource
owners any time soon, even if some resource owners don't need them.
- Heikki