Thread: Insecure initialization of required_relids field
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] 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). [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); [2] https://commitfest.postgresql.org/23/1712/ -- Andrey Lepikhov Postgres Professional https://postgrespro.com The Russian Postgres Company
Attachment
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] 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