Bug in DefineRange() with multiranges
Hi,
My colleague, Alex Kozhemyakin, stumbled upon a bug in DefineRange().
The problem is here:
@@ -1707,7 +1707,6 @@ DefineRange(ParseState *pstate,
CreateRangeStmt *stmt)
/* Create cast from the range type to its multirange type */
CastCreate(typoid, multirangeOid, castFuncOid, 'e', 'f',
DEPENDENCY_INTERNAL);
- pfree(multirangeTypeName);
pfree(multirangeArrayName);
return address;
Given a query
create type textrange1 as range(subtype=text,
multirange_type_name=multirange_of_text, collation="C");
the string "multirange_of_text" in the parse tree is erroneously
pfree'd. The corrupted parse tree is then passed to event triggers.
There is another branch in DefineRange() that genereates a multirange
type name which is fine to free.
I wonder what is the proper fix. Just drop pfree() altogether or add
pstrdup() instead? I see that makeMultirangeTypeName() doesn't bother
freeing its buf.
Here is a gdb session demonstating the bug:
Breakpoint 1, ProcessUtilitySlow (pstate=0x5652e80c7730,
pstmt=0x5652e80a6a40, queryString=0x5652e80a5790 "create type textrange1
as range(subtype=text, multirange_type_name=multirange_of_text,
collation=\"C\");",
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
qc=0x7ffe835b4be0, dest=<optimized out>) at
/pgwork/REL_14_STABLE/src/src/backend/tcop/utility.c:1621
1621 address = DefineRange((CreateRangeStmt *)
parsetree);
(gdb) p *(Value *)((TypeName *)((DefElem *)((CreateRangeStmt
*)parsetree)->params->elements[1].ptr_value)->arg)->names->elements[0].ptr_value
$1 = {type = T_String, val = {ival = -401972176, str = 0x5652e80a6430
"multirange_of_text"}}
(gdb) n
1900 if (!commandCollected)
(gdb) p *(Value *)((TypeName *)((DefElem *)((CreateRangeStmt
*)parsetree)->params->elements[1].ptr_value)->arg)->names->elements[0].ptr_value
$2 = {type = T_String, val = {ival = -401972176, str = 0x5652e80a6430
'\177' <repeats 32 times>, "\020"}}
Regards,
--
Sergey Shinderuk https://postgrespro.com/
On 04.10.21 19:09, Sergey Shinderuk wrote:
I wonder what is the proper fix. Just drop pfree() altogether or add
pstrdup() instead? I see that makeMultirangeTypeName() doesn't bother
freeing its buf.
I think removing the pfree()s is a correct fix.
On 10.10.2021 20:12, Peter Eisentraut wrote:
On 04.10.21 19:09, Sergey Shinderuk wrote:
I wonder what is the proper fix. Just drop pfree() altogether or add
pstrdup() instead? I see that makeMultirangeTypeName() doesn't bother
freeing its buf.I think removing the pfree()s is a correct fix.
Thanks, here is a patch.
--
Sergey Shinderuk https://postgrespro.com/
Attachments:
0001-Fix-premature-pfree-of-multirange_type_name-in-Defin.patchtext/plain; charset=UTF-8; name=0001-Fix-premature-pfree-of-multirange_type_name-in-Defin.patch; x-mac-creator=0; x-mac-type=0Download
From 6c548f07d2a254da46cd0b6f6e99b7ed24a6b811 Mon Sep 17 00:00:00 2001
From: Sergey Shinderuk <s.shinderuk@postgrespro.ru>
Date: Tue, 12 Oct 2021 07:57:42 +0300
Subject: [PATCH] Fix premature pfree() of multirange_type_name in
DefineRange()
If the mutlirange_type_name parameter is given in the query, this would
erroneously pfree() the string in the parse tree. Oversight in 6df7a9698bb0.
---
src/backend/commands/typecmds.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index b290629a450..9ab40341793 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1707,7 +1707,6 @@ DefineRange(ParseState *pstate, CreateRangeStmt *stmt)
/* Create cast from the range type to its multirange type */
CastCreate(typoid, multirangeOid, castFuncOid, 'e', 'f', DEPENDENCY_INTERNAL);
- pfree(multirangeTypeName);
pfree(multirangeArrayName);
return address;
--
2.24.3 (Apple Git-128)
On Tue, Oct 12, 2021 at 08:52:29AM +0300, Sergey Shinderuk wrote:
Thanks, here is a patch.
Looks fine seen from here, so I'll apply shortly. I was initially
tempted to do pstrdup() on the object name returned by
QualifiedNameGetCreationNamespace(), but just removing the pfree() is
simpler.
I got to wonder about similar mistakes from the other callers of
QualifiedNameGetCreationNamespace(), so I have double-checked but
nothing looks wrong.
--
Michael
On 13.10.2021 07:21, Michael Paquier wrote:
Looks fine seen from here, so I'll apply shortly.
Thank you!
--
Sergey Shinderuk https://postgrespro.com/