Обсуждение: 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
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