pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedReport a potential bug caused by a improper call to pfree()
4+ messages / 4 participants
[nested] [flat]
* Report a potential bug caused by a improper call to pfree()
@ 2022-01-30 02:47 wliang@stu.xidian.edu.cn
2022-01-30 13:36 ` Re: Report a potential bug caused by a improper call to pfree() Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: wliang@stu.xidian.edu.cn @ 2022-01-30 02:47 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Report a potential bug caused by a improper call to pfree()
2022-01-30 02:47 Report a potential bug caused by a improper call to pfree() wliang@stu.xidian.edu.cn
@ 2022-01-30 13:36 ` Julien Rouhaud <rjuju123@gmail.com>
2022-01-30 15:29 ` Re: Report a potential bug caused by a improper call to pfree() Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 4+ messages in thread
From: Julien Rouhaud @ 2022-01-30 13:36 UTC (permalink / raw)
To: wliang@stu.xidian.edu.cn; +Cc: pgsql-bugs@lists.postgresql.org
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.
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Report a potential bug caused by a improper call to pfree()
2022-01-30 02:47 Report a potential bug caused by a improper call to pfree() wliang@stu.xidian.edu.cn
2022-01-30 13:36 ` Re: Report a potential bug caused by a improper call to pfree() Julien Rouhaud <rjuju123@gmail.com>
@ 2022-01-30 15:29 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-01-31 02:41 ` Re: Report a potential bug caused by a improper call to pfree() Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: Tom Lane @ 2022-01-30 15:29 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: wliang@stu.xidian.edu.cn; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Report a potential bug caused by a improper call to pfree()
2022-01-30 02:47 Report a potential bug caused by a improper call to pfree() wliang@stu.xidian.edu.cn
2022-01-30 13:36 ` Re: Report a potential bug caused by a improper call to pfree() Julien Rouhaud <rjuju123@gmail.com>
2022-01-30 15:29 ` Re: Report a potential bug caused by a improper call to pfree() Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-01-31 02:41 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 0 replies; 4+ messages in thread
From: Kyotaro Horiguchi @ 2022-01-31 02:41 UTC (permalink / raw)
To: tgl@sss.pgh.pa.us; +Cc: rjuju123@gmail.com; wliang@stu.xidian.edu.cn; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2022-01-31 02:41 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-01-30 02:47 Report a potential bug caused by a improper call to pfree() wliang@stu.xidian.edu.cn
2022-01-30 13:36 ` Julien Rouhaud <rjuju123@gmail.com>
2022-01-30 15:29 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-01-31 02:41 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
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