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

Andrey Lepikhov <a(dot)lepikhov(at)postgrespro(dot)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(dot)lepikhov(at)postgrespro(dot)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