Postgres Pro
Downloads
Home   >   mailing lists

Thread: Report a potential bug caused by a improper call to pfree()

Report a potential bug caused by a improper call to pfree()

From
wliang@stu.xidian.edu.cn
Date:
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

Re: Report a potential bug caused by a improper call to pfree()

From
Julien Rouhaud
Date:
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.

Re: Report a potential bug caused by a improper call to pfree()

From
Tom Lane
Date:
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

Re: Report a potential bug caused by a improper call to pfree()

From
Kyotaro Horiguchi
Date:
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