Report a potential bug caused by a improper call to pfree()
Hi all,
I find a potential bug caused by a improper call to pfree in PostgresSQL 14.1, which is in backend/utils/adt/jsonb_gin.c
Specifically, at line 1116, the pointer 'stack' is assigned with the address of a local variable 'tail'.
At line 1163, pfree() is called to free 'stack'. However, pfree is designed to free the memory in heap rather than stack.
1090 Datum
1091 gin_extract_jsonb_path(PG_FUNCTION_ARGS)
1092 {
1093 Jsonb *jb = PG_GETARG_JSONB_P(0);
1094 int32 *nentries = (int32 *) PG_GETARG_POINTER(1);
1095 int total = JB_ROOT_COUNT(jb);
1096 JsonbIterator *it;
1097 JsonbValue v;
1098 JsonbIteratorToken r;
1099 PathHashStack tail;
1100 PathHashStack *stack;
1101 GinEntries entries;
...
1113 /* We keep a stack of partial hashes corresponding to parent key levels */
1114 tail.parent = NULL;
1115 tail.hash = 0;
1116 stack = &tail;
1117
1118 it = JsonbIteratorInit(&jb->root);
1119
1120 while ((r = JsonbIteratorNext(&it, &v, false)) != WJB_DONE)
1121 {
1122 PathHashStack *parent;
1123
1124 switch (r)
1125 {
...
1158 case WJB_END_ARRAY:
1159 case WJB_END_OBJECT:
1160 /* Pop the stack */
1161 parent = stack->parent;
1162 pfree(stack);
--------------------
I think it may be a potential bug and can be fixed without any side-effect as:
++ if (stack != &tail)
1162 pfree(stack);
Thanks!
Wentao Liang
Hi,
On Sun, Jan 30, 2022 at 10:47:18AM +0800, wliang@stu.xidian.edu.cn wrote:
I find a potential bug caused by a improper call to pfree in PostgresSQL 14.1, which is in backend/utils/adt/jsonb_gin.c
Specifically, at line 1116, the pointer 'stack' is assigned with the address of a local variable 'tail'.
At line 1163, pfree() is called to free 'stack'. However, pfree is designed to free the memory in heap rather than stack.1158 case WJB_END_ARRAY:
1159 case WJB_END_OBJECT:
1160 /* Pop the stack */
1161 parent = stack->parent;
1162 pfree(stack);I think it may be a potential bug and can be fixed without any side-effect as:
++ if (stack != &tail)
1162 pfree(stack);
I don't think it's necessary, it should be guaranteed that something as been
pushed on the tail, ie. there shouldn't be a WJB_END_* before a corresponding
begin.
Note that the tail also can't have a parent, so even if that scenario could
happen, it would crash in the previous instruction anyway, trying to
dereference a NULL pointer.
Julien Rouhaud <rjuju123@gmail.com> writes:
On Sun, Jan 30, 2022 at 10:47:18AM +0800, wliang@stu.xidian.edu.cn wrote:
1160 /* Pop the stack */
1161 parent = stack->parent;
1162 pfree(stack);I think it may be a potential bug and can be fixed without any side-effect as:
++ if (stack != &tail)
1162 pfree(stack);
I don't think it's necessary, it should be guaranteed that something as been
pushed on the tail, ie. there shouldn't be a WJB_END_* before a corresponding
begin.
I've not checked the logic, but the lack of any reported crashes here
seems to confirm that there's no bug.
regards, tom lane
At Sun, 30 Jan 2022 10:29:27 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in
Julien Rouhaud <rjuju123@gmail.com> writes:
On Sun, Jan 30, 2022 at 10:47:18AM +0800, wliang@stu.xidian.edu.cn wrote:
1160 /* Pop the stack */
1161 parent = stack->parent;
1162 pfree(stack);I think it may be a potential bug and can be fixed without any side-effect as:
++ if (stack != &tail)
1162 pfree(stack);I don't think it's necessary, it should be guaranteed that something as been
pushed on the tail, ie. there shouldn't be a WJB_END_* before a corresponding
begin.I've not checked the logic, but the lack of any reported crashes here
seems to confirm that there's no bug.
As a cross-check, I agree to Julien. The parser starts reading from
OBJECT_START or ARRAY_START (or bare scalar) so if we had the stack
empty there, we *should properly crash* instead of pretending that a
problem were not exitsting at all.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center