Insecure initialization of required_relids field
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:t40927psql -h localhost -U postgresBuilt from patchset v1 (message #1), September 20, 2026 at 08:25 AM.
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 t40927_1 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 t40927_1 && git checkout t40927_1Patchset v1 (message #1) is on t40927_1
Hi,
commit a31ad27fc5d introduced required_relids field. By default, it
links to the clause_relids.
It works good while we do not modify clause_relids or required_relids.
But in the case of modification such initialization demands us to
remember, that this field is shared. And we need to do bms_copy() before
making any changes (see [1]commit 4e97631e6a9, analyzejoins.c, line 434,435: rinfo->required_relids = bms_copy(rinfo->required_relids); rinfo->required_relids = bms_del_member(rinfo->required_relids, relid); for example).
Also, we make some changes of the RestrictInfo fields (see patch [2]https://commitfest.postgresql.org/23/1712/)
during removing of unneeded self joins.
I propose to do more secure initialization way of required_relids (see
patch in attachment).
[1]: commit 4e97631e6a9, analyzejoins.c, line 434,435: rinfo->required_relids = bms_copy(rinfo->required_relids); rinfo->required_relids = bms_del_member(rinfo->required_relids, relid);
rinfo->required_relids = bms_copy(rinfo->required_relids);
rinfo->required_relids = bms_del_member(rinfo->required_relids, relid);
[2]: https://commitfest.postgresql.org/23/1712/
--
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company
Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes:
commit a31ad27fc5d introduced required_relids field. By default, it
links to the clause_relids.
It works good while we do not modify clause_relids or required_relids.
But in the case of modification such initialization demands us to
remember, that this field is shared. And we need to do bms_copy() before
making any changes (see [1] for example).
Also, we make some changes of the RestrictInfo fields (see patch [2])
during removing of unneeded self joins.
I propose to do more secure initialization way of required_relids (see
patch in attachment).
This seems fairly expensive (which is why it wasn't done like that
to start with) and you've pointed to no specific bug that it fixes.
Seeing that (a) the original commit is 14 years old, and (b) changing
either of these fields after-the-fact is at most a very niche usage,
I don't think we really have a problem here.
regards, tom lane
On 15/07/2019 18:48, Tom Lane wrote:
Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes:
commit a31ad27fc5d introduced required_relids field. By default, it
links to the clause_relids.
It works good while we do not modify clause_relids or required_relids.
But in the case of modification such initialization demands us to
remember, that this field is shared. And we need to do bms_copy() before
making any changes (see [1] for example).
Also, we make some changes of the RestrictInfo fields (see patch [2])
during removing of unneeded self joins.
I propose to do more secure initialization way of required_relids (see
patch in attachment).This seems fairly expensive (which is why it wasn't done like that
to start with) and you've pointed to no specific bug that it fixes.
Seeing that (a) the original commit is 14 years old, and (b) changing
either of these fields after-the-fact is at most a very niche usage,
I don't think we really have a problem here.
In the patch 'Removing unneeded self joins' [1]https://commitfest.postgresql.org/23/1712/ -- Andrey Lepikhov Postgres Professional https://postgrespro.com The Russian Postgres Company we modify both
clause_relids and required_relids. Valgrind detected a problem: during
the required_relids change routine repalloc() was executed. In this
case, clause_relids will point to free memory block.
In accordance to your answer do you recommend me to make the bms_copy()
call before changing any of clause_relids and required_relids fields?
[1]: https://commitfest.postgresql.org/23/1712/ -- Andrey Lepikhov Postgres Professional https://postgrespro.com The Russian Postgres Company
--
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company