Re: Insecure initialization of required_relids field

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