pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedInsecure initialization of required_relids field
3+ messages / 2 participants
[nested] [flat]
* Insecure initialization of required_relids field
@ 2019-07-15 06:12 Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 3+ messages in thread
From: Andrey Lepikhov @ 2019-07-15 06:12 UTC (permalink / raw)
To: pgsql-hackers
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
Attachments:
0001-More-secure-initialization-of-required_relids-field.patch (text/x-patch, 947B) download
From 1dc021c0c9827ecd530ee3bb0020bad9bc5d14b6 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Date: Mon, 15 Jul 2019 11:07:20 +0500
Subject: [PATCH] More secure initialization of required_relids field
---
src/backend/optimizer/util/restrictinfo.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c
index 3b50fd29ad..450558e61b 100644
--- a/src/backend/optimizer/util/restrictinfo.c
+++ b/src/backend/optimizer/util/restrictinfo.c
@@ -172,7 +172,7 @@ make_restrictinfo_internal(Expr *clause,
if (required_relids != NULL)
restrictinfo->required_relids = required_relids;
else
- restrictinfo->required_relids = restrictinfo->clause_relids;
+ restrictinfo->required_relids = bms_copy(restrictinfo->clause_relids);
/*
* Fill in all the cacheable fields with "not yet set" markers. None of
--
2.17.1
* Re: Insecure initialization of required_relids field
@ 2019-07-15 13:48 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 3+ messages in thread
From: Tom Lane @ 2019-07-15 13:48 UTC (permalink / raw)
To: Andrey Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Insecure initialization of required_relids field
@ 2019-07-15 17:08 Andrey Lepikhov <a.lepikhov@postgrespro.ru>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 3+ messages in thread
From: Andrey Lepikhov @ 2019-07-15 17:08 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2019-07-15 17:08 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-07-15 06:12 Insecure initialization of required_relids field Andrey Lepikhov <a.lepikhov@postgrespro.ru>
2019-07-15 13:48 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-07-15 17:08 ` Andrey Lepikhov <a.lepikhov@postgrespro.ru>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox