Small bugs regarding resowner handling in aio.c, catcache.c
Hi Heikki, Andres,
Whilst looking through catcache.c's code I noticed this piece of code:
ReleaseCatCacheListWithOwner(CatCList *list, ResourceOwner resowner)
{
[...]
if (resowner)
ResourceOwnerForgetCatCacheRef(CurrentResourceOwner, &ct->tuple);
Note how the resowner argument is ignored in favour of
CurrentResourceOwner; and that probably wasn't what the author
intended.
So I looked around a bit, and found 2 more similar instances: one more
in catcache.c, and one in aio.c. I can't guarantee that there are no
other comparable issues, but at least I could not immediately find any
functions that ignore their ResourceOwner argument in favour of
CurrentResourceOwner.
The issue in aio.c is externally visible, so it might trigger issues.
However, every caller in the tree uses CurrentResourceOwner, so only
extensions could trigger this bug.
The two issues in catcache.c are benign: No external code can trigger
it with a different resource owner; and while no internal code would
trigger it either, it's better to use the provided resowner, so that
future callers won't activate the bug.
Attached a fix for all 3 cases.
Kind regards,
Matthias van de Meent
Databricks (https://www.databricks.com)
Attachments:
v1-0001-Fix-some-near-bugs-related-to-ResOwner-function-a.patchapplication/octet-stream; name=v1-0001-Fix-some-near-bugs-related-to-ResOwner-function-a.patchDownload
From d8344cd804a2af68c22cb3afcf74fa7eeeda2110 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Wed, 10 Dec 2025 00:03:10 +0100
Subject: [PATCH v1] Fix some near-bugs related to ResOwner function arguments
Whilst all users of these functions were providing
CurrentResourceOwner (CRO) as resowner argument, it's not good form
to ignore the argument and manually pull CRO, a future user (or
extension) may want to use this functionality to register using
a different ResOwner than CRO.
---
src/backend/storage/aio/aio.c | 12 ++++++------
src/backend/utils/cache/catcache.c | 4 ++--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/backend/storage/aio/aio.c b/src/backend/storage/aio/aio.c
index a12b785ade6..c4c2d8cc4b1 100644
--- a/src/backend/storage/aio/aio.c
+++ b/src/backend/storage/aio/aio.c
@@ -53,7 +53,7 @@
static inline void pgaio_io_update_state(PgAioHandle *ioh, PgAioHandleState new_state);
static void pgaio_io_reclaim(PgAioHandle *ioh);
-static void pgaio_io_resowner_register(PgAioHandle *ioh);
+static void pgaio_io_resowner_register(PgAioHandle *ioh, struct ResourceOwnerData *resowner);
static void pgaio_io_wait_for_free(void);
static PgAioHandle *pgaio_io_from_wref(PgAioWaitRef *iow, uint64 *ref_generation);
static const char *pgaio_io_state_get_name(PgAioHandleState s);
@@ -217,7 +217,7 @@ pgaio_io_acquire_nb(struct ResourceOwnerData *resowner, PgAioReturn *ret)
pgaio_my_backend->handed_out_io = ioh;
if (resowner)
- pgaio_io_resowner_register(ioh);
+ pgaio_io_resowner_register(ioh, resowner);
if (ret)
{
@@ -406,13 +406,13 @@ pgaio_io_update_state(PgAioHandle *ioh, PgAioHandleState new_state)
}
static void
-pgaio_io_resowner_register(PgAioHandle *ioh)
+pgaio_io_resowner_register(PgAioHandle *ioh, struct ResourceOwnerData *resowner)
{
Assert(!ioh->resowner);
- Assert(CurrentResourceOwner);
+ Assert(resowner);
- ResourceOwnerRememberAioHandle(CurrentResourceOwner, &ioh->resowner_node);
- ioh->resowner = CurrentResourceOwner;
+ ResourceOwnerRememberAioHandle(resowner, &ioh->resowner_node);
+ ioh->resowner = resowner;
}
/*
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 02ae7d5a831..2d8a31d6895 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -1668,7 +1668,7 @@ ReleaseCatCacheWithOwner(HeapTuple tuple, ResourceOwner resowner)
ct->refcount--;
if (resowner)
- ResourceOwnerForgetCatCacheRef(CurrentResourceOwner, &ct->tuple);
+ ResourceOwnerForgetCatCacheRef(resowner, &ct->tuple);
if (
#ifndef CATCACHE_FORCE_RELEASE
@@ -2110,7 +2110,7 @@ ReleaseCatCacheListWithOwner(CatCList *list, ResourceOwner resowner)
Assert(list->refcount > 0);
list->refcount--;
if (resowner)
- ResourceOwnerForgetCatCacheListRef(CurrentResourceOwner, list);
+ ResourceOwnerForgetCatCacheListRef(resowner, list);
if (
#ifndef CATCACHE_FORCE_RELEASE
--
2.50.1 (Apple Git-155)
On 10/12/2025 01:33, Matthias van de Meent wrote:
Hi Heikki, Andres,
Whilst looking through catcache.c's code I noticed this piece of code:
ReleaseCatCacheListWithOwner(CatCList *list, ResourceOwner resowner)
{
[...]
if (resowner)
ResourceOwnerForgetCatCacheRef(CurrentResourceOwner, &ct->tuple);Note how the resowner argument is ignored in favour of
CurrentResourceOwner; and that probably wasn't what the author
intended.So I looked around a bit, and found 2 more similar instances: one more
in catcache.c, and one in aio.c. I can't guarantee that there are no
other comparable issues, but at least I could not immediately find any
functions that ignore their ResourceOwner argument in favour of
CurrentResourceOwner.The issue in aio.c is externally visible, so it might trigger issues.
However, every caller in the tree uses CurrentResourceOwner, so only
extensions could trigger this bug.
The two issues in catcache.c are benign: No external code can trigger
it with a different resource owner; and while no internal code would
trigger it either, it's better to use the provided resowner, so that
future callers won't activate the bug.Attached a fix for all 3 cases.
Pushed and backpatched, thanks!
- Heikki
Hi,
On 2025-12-10 11:51:35 +0200, Heikki Linnakangas wrote:
On 10/12/2025 01:33, Matthias van de Meent wrote:
Hi Heikki, Andres,
Whilst looking through catcache.c's code I noticed this piece of code:
ReleaseCatCacheListWithOwner(CatCList *list, ResourceOwner resowner)
{
[...]
if (resowner)
ResourceOwnerForgetCatCacheRef(CurrentResourceOwner, &ct->tuple);Note how the resowner argument is ignored in favour of
CurrentResourceOwner; and that probably wasn't what the author
intended.So I looked around a bit, and found 2 more similar instances: one more
in catcache.c, and one in aio.c. I can't guarantee that there are no
other comparable issues, but at least I could not immediately find any
functions that ignore their ResourceOwner argument in favour of
CurrentResourceOwner.The issue in aio.c is externally visible, so it might trigger issues.
However, every caller in the tree uses CurrentResourceOwner, so only
extensions could trigger this bug.
The two issues in catcache.c are benign: No external code can trigger
it with a different resource owner; and while no internal code would
trigger it either, it's better to use the provided resowner, so that
future callers won't activate the bug.Attached a fix for all 3 cases.
Pushed and backpatched, thanks!
Thanks for finding/fixing this and merging the change!
Greetings,
Andres Freund