BUG #18598: AddressSanitizer detects use after free inside json_unique_hash_match()

The following bug has been logged on the website:

Bug reference: 18598
Logged by: Alexander Lakhin
Email address: exclusion(at)gmail(dot)com
PostgreSQL version: 17beta3
Operating system: Ubuntu 22.04
Description:

The following query:
SELECT JSON_OBJECTAGG(i: (i)::text FORMAT JSON WITH UNIQUE)
FROM generate_series(1, 100000) i;

triggers an asan-detected error:
==973230==ERROR: AddressSanitizer: heap-use-after-free on address
0x7fde473f4428 at pc 0x558af80f20a6 bp 0x7ffe6b8e2df0 sp 0x7ffe6b8e2598
READ of size 7 at 0x7fde473f4428 thread T0
#0 0x558af80f20a5 in __interceptor_strncmp.part.0
(.../usr/local/pgsql/bin/postgres+0x32d40a5)
#1 0x558af9ed5276 in json_unique_hash_match
.../src/backend/utils/adt/json.c:922
#2 0x558afa49c6ce in hash_search_with_hash_value
.../src/backend/utils/hash/dynahash.c:1021
#3 0x558afa49bfbc in hash_search
.../src/backend/utils/hash/dynahash.c:960
#4 0x558af9ed58b4 in json_unique_check_key
.../src/backend/utils/adt/json.c:967
#5 0x558af9ed6a71 in json_object_agg_transfn_worker
.../src/backend/utils/adt/json.c:1116
#6 0x558af9ed6fc5 in json_object_agg_unique_transfn
.../src/backend/utils/adt/json.c:1163
#7 0x558af8e3dcbe in ExecAggPlainTransByVal
.../src/backend/executor/execExprInterp.c:5382
...
0x7fde473f4428 is located 506920 bytes inside of 524352-byte region
[0x7fde47378800,0x7fde473f8840)
freed by thread T0 here:
#0 0x558af8114038 in realloc
(.../usr/local/pgsql/bin/postgres+0x32f6038)
#1 0x558afa52c970 in AllocSetRealloc
.../src/backend/utils/mmgr/aset.c:1226
#2 0x558afa56c0e9 in repalloc .../src/backend/utils/mmgr/mcxt.c:1566
#3 0x558afa66c94a in enlargeStringInfo .../src/common/stringinfo.c:349
#4 0x558afa66be4a in appendBinaryStringInfo
.../src/common/stringinfo.c:238
#5 0x558afa66b612 in appendStringInfoString
.../src/common/stringinfo.c:184
#6 0x558af9ed66b9 in json_object_agg_transfn_worker
.../src/backend/utils/adt/json.c:1102
#7 0x558af9ed6fc5 in json_object_agg_unique_transfn
.../src/backend/utils/adt/json.c:1163
#8 0x558af8e3dcbe in ExecAggPlainTransByVal
.../src/backend/executor/execExprInterp.c:5382
...
previously allocated by thread T0 here:
#0 0x558af8114038 in realloc
(.../usr/local/pgsql/bin/postgres+0x32f6038)
#1 0x558afa52c970 in AllocSetRealloc
.../src/backend/utils/mmgr/aset.c:1226
#2 0x558afa56c0e9 in repalloc .../src/backend/utils/mmgr/mcxt.c:1566
#3 0x558afa66c94a in enlargeStringInfo .../src/common/stringinfo.c:349
#4 0x558afa66be4a in appendBinaryStringInfo
.../src/common/stringinfo.c:238
#5 0x558afa66b612 in appendStringInfoString
.../src/common/stringinfo.c:184
#6 0x558af9ed0559 in datum_to_json_internal
.../src/backend/utils/adt/json.c:279
#7 0x558af9ed6ee3 in json_object_agg_transfn_worker
.../src/backend/utils/adt/json.c:1132
#8 0x558af9ed6fc5 in json_object_agg_unique_transfn
.../src/backend/utils/adt/json.c:1163
#9 0x558af8e3dcbe in ExecAggPlainTransByVal
.../src/backend/executor/execExprInterp.c:5382
...

Reproduced starting from 7081ac46a.

On 9/1/24 21:00, PG Bug reporting form wrote:
> The following bug has been logged on the website:
>
> Bug reference: 18598
> Logged by: Alexander Lakhin
> Email address: exclusion(at)gmail(dot)com
> PostgreSQL version: 17beta3
> Operating system: Ubuntu 22.04
> Description:
>
> The following query:
> SELECT JSON_OBJECTAGG(i: (i)::text FORMAT JSON WITH UNIQUE)
> FROM generate_series(1, 100000) i;
>
> triggers an asan-detected error:
> ==973230==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x7fde473f4428 at pc 0x558af80f20a6 bp 0x7ffe6b8e2df0 sp 0x7ffe6b8e2598
> READ of size 7 at 0x7fde473f4428 thread T0
> #0 0x558af80f20a5 in __interceptor_strncmp.part.0
> (.../usr/local/pgsql/bin/postgres+0x32d40a5)
> #1 0x558af9ed5276 in json_unique_hash_match
> ...
>
> Reproduced starting from 7081ac46a.
>

FWIW I can reproduce this using valgrind, with the same stacks reported.

This feels very much like a classical memory context bug - pointing to
memory in a short-lived memory context. I see datum_to_json_internal()
allocates the result in ExprContext, and that's bound to be reset pretty
often. But I'm not too familiar with the JSON aggregate stuff enough to
pinpoint what it does wrong.

regards

--
Tomas Vondra

On Wed, Sep 4, 2024 at 3:21 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> On 9/1/24 21:00, PG Bug reporting form wrote:
> > The following bug has been logged on the website:
> >
> > Bug reference: 18598
> > Logged by: Alexander Lakhin
> > Email address: exclusion(at)gmail(dot)com
> > PostgreSQL version: 17beta3
> > Operating system: Ubuntu 22.04
> > Description:
> >
> > The following query:
> > SELECT JSON_OBJECTAGG(i: (i)::text FORMAT JSON WITH UNIQUE)
> > FROM generate_series(1, 100000) i;
> >
> > triggers an asan-detected error:
> > ==973230==ERROR: AddressSanitizer: heap-use-after-free on address
> > 0x7fde473f4428 at pc 0x558af80f20a6 bp 0x7ffe6b8e2df0 sp 0x7ffe6b8e2598
> > READ of size 7 at 0x7fde473f4428 thread T0
> > #0 0x558af80f20a5 in __interceptor_strncmp.part.0
> > (.../usr/local/pgsql/bin/postgres+0x32d40a5)
> > #1 0x558af9ed5276 in json_unique_hash_match
> > ...
> >
> > Reproduced starting from 7081ac46a.
> >
>
> FWIW I can reproduce this using valgrind, with the same stacks reported.
>
> This feels very much like a classical memory context bug - pointing to
> memory in a short-lived memory context. I see datum_to_json_internal()
> allocates the result in ExprContext, and that's bound to be reset pretty
> often. But I'm not too familiar with the JSON aggregate stuff enough to
> pinpoint what it does wrong.
>
> regards
>
> --
> Tomas Vondra
>
>

ISTM that the JsonUniqueHashEntry.key point to an address later got
invalidated by enlargeStringInfo, we can resolve this by explicitly
pstrdup the key in the same MemoryContext of JsonAggState, like:

@@ -1009,6 +1009,7 @@ json_object_agg_transfn_worker(FunctionCallInfo fcinfo,
Datum arg;
bool skip;
int key_offset;
+ const char *key;

if (!AggCheckCallContext(fcinfo, &aggcontext))
{
@@ -1111,7 +1112,9 @@ json_object_agg_transfn_worker(FunctionCallInfo fcinfo,

if (unique_keys)
{
- const char *key = &out->data[key_offset];
+ oldcontext = MemoryContextSwitchTo(aggcontext);
+ key = pstrdup(&out->data[key_offset]);
+ MemoryContextSwitchTo(oldcontext);

--
Regards
Junwang Zhao

On 9/4/24 11:55, Junwang Zhao wrote:
> ...
>
> ISTM that the JsonUniqueHashEntry.key point to an address later got
> invalidated by enlargeStringInfo, we can resolve this by explicitly
> pstrdup the key in the same MemoryContext of JsonAggState, like:

Yes, this fixes the issue (at least per valgrind).

> @@ -1009,6 +1009,7 @@ json_object_agg_transfn_worker(FunctionCallInfo fcinfo,
> Datum arg;
> bool skip;
> int key_offset;
> + const char *key;
>
> if (!AggCheckCallContext(fcinfo, &aggcontext))
> {
> @@ -1111,7 +1112,9 @@ json_object_agg_transfn_worker(FunctionCallInfo fcinfo,
>
> if (unique_keys)
> {
> - const char *key = &out->data[key_offset];
> + oldcontext = MemoryContextSwitchTo(aggcontext);
> + key = pstrdup(&out->data[key_offset]);
> + MemoryContextSwitchTo(oldcontext);
>

I think you don't need the new key declaration (there's already a local
one), and you can simply do just

const char *key = MemoryContextStrdup(aggcontext,
&out->data[key_offset]);

I wonder if the other json_unique_check_key() call might have a similar
issue. I've not succeeded in constructing a broken query, but perhaps
you could give it a try too?

Thanks!

--
Tomas Vondra

On Wed, Sep 4, 2024 at 7:54 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> On 9/4/24 11:55, Junwang Zhao wrote:
> > ...
> >
> > ISTM that the JsonUniqueHashEntry.key point to an address later got
> > invalidated by enlargeStringInfo, we can resolve this by explicitly
> > pstrdup the key in the same MemoryContext of JsonAggState, like:
>
> Yes, this fixes the issue (at least per valgrind).
>
> > @@ -1009,6 +1009,7 @@ json_object_agg_transfn_worker(FunctionCallInfo fcinfo,
> > Datum arg;
> > bool skip;
> > int key_offset;
> > + const char *key;
> >
> > if (!AggCheckCallContext(fcinfo, &aggcontext))
> > {
> > @@ -1111,7 +1112,9 @@ json_object_agg_transfn_worker(FunctionCallInfo fcinfo,
> >
> > if (unique_keys)
> > {
> > - const char *key = &out->data[key_offset];
> > + oldcontext = MemoryContextSwitchTo(aggcontext);
> > + key = pstrdup(&out->data[key_offset]);
> > + MemoryContextSwitchTo(oldcontext);
> >
>
> I think you don't need the new key declaration (there's already a local
> one), and you can simply do just
>
> const char *key = MemoryContextStrdup(aggcontext,
> &out->data[key_offset]);
>

Sure, I will file a patch later.

> I wonder if the other json_unique_check_key() call might have a similar
> issue. I've not succeeded in constructing a broken query, but perhaps
> you could give it a try too?

Sure, I will give it a try, thanks for the comment.
>
>
> Thanks!
>
> --
> Tomas Vondra

--
Regards
Junwang Zhao

CC'd hackers list.

On Wed, Sep 4, 2024 at 7:54 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> On 9/4/24 11:55, Junwang Zhao wrote:
> > ...
> >
> > ISTM that the JsonUniqueHashEntry.key point to an address later got
> > invalidated by enlargeStringInfo, we can resolve this by explicitly
> > pstrdup the key in the same MemoryContext of JsonAggState, like:
>
> Yes, this fixes the issue (at least per valgrind).
>
> > @@ -1009,6 +1009,7 @@ json_object_agg_transfn_worker(FunctionCallInfo fcinfo,
> > Datum arg;
> > bool skip;
> > int key_offset;
> > + const char *key;
> >
> > if (!AggCheckCallContext(fcinfo, &aggcontext))
> > {
> > @@ -1111,7 +1112,9 @@ json_object_agg_transfn_worker(FunctionCallInfo fcinfo,
> >
> > if (unique_keys)
> > {
> > - const char *key = &out->data[key_offset];
> > + oldcontext = MemoryContextSwitchTo(aggcontext);
> > + key = pstrdup(&out->data[key_offset]);
> > + MemoryContextSwitchTo(oldcontext);
> >
>
> I think you don't need the new key declaration (there's already a local
> one), and you can simply do just
>
> const char *key = MemoryContextStrdup(aggcontext,
> &out->data[key_offset]);
>
> I wonder if the other json_unique_check_key() call might have a similar
> issue. I've not succeeded in constructing a broken query, but perhaps
> you could give it a try too?

I found two other places called json_unique_check_key.

One is *json_build_object_worker*, and the usage is the same as
*json_object_agg_transfn_worker*, I fix that the same way, PSA

The following sql should trigger the problem, I haven't tried asan
but traced the *repalloc* in gdb, I will try this later when I set up my
asan building.

SELECT JSON_OBJECT(1: 1, '2': NULL, '3': 1, repeat('x', 1000): 1, 2:
repeat('a', 100) WITH UNIQUE);

The other place is json_unique_object_field_start, it is set
as a callback of JsonSemAction, and in the parse state machine,
the fname used by the callback has been correctly handled.
see [1] & [2].

[1]: https://github.com/postgres/postgres/blob/master/src/common/jsonapi.c#L1069-L1087
[2]: https://github.com/postgres/postgres/blob/master/src/common/jsonapi.c#L785-L823

>
>
> Thanks!
>
> --
> Tomas Vondra

--
Regards
Junwang Zhao

On Wed, Sep 4, 2024 at 7:54 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> On 9/4/24 11:55, Junwang Zhao wrote:
> > ...
> >
> > ISTM that the JsonUniqueHashEntry.key point to an address later got
> > invalidated by enlargeStringInfo, we can resolve this by explicitly
> > pstrdup the key in the same MemoryContext of JsonAggState, like:
>
> Yes, this fixes the issue (at least per valgrind).
>
> > @@ -1009,6 +1009,7 @@ json_object_agg_transfn_worker(FunctionCallInfo fcinfo,
> > Datum arg;
> > bool skip;
> > int key_offset;
> > + const char *key;
> >
> > if (!AggCheckCallContext(fcinfo, &aggcontext))
> > {
> > @@ -1111,7 +1112,9 @@ json_object_agg_transfn_worker(FunctionCallInfo fcinfo,
> >
> > if (unique_keys)
> > {
> > - const char *key = &out->data[key_offset];
> > + oldcontext = MemoryContextSwitchTo(aggcontext);
> > + key = pstrdup(&out->data[key_offset]);
> > + MemoryContextSwitchTo(oldcontext);
> >
>
> I think you don't need the new key declaration (there's already a local
> one), and you can simply do just
>
> const char *key = MemoryContextStrdup(aggcontext,
> &out->data[key_offset]);
>
> I wonder if the other json_unique_check_key() call might have a similar
> issue. I've not succeeded in constructing a broken query, but perhaps
> you could give it a try too?

I found two other places called json_unique_check_key.

One is *json_build_object_worker*, and the usage is the same as
*json_object_agg_transfn_worker*, I fix that the same way, PSA

The following sql should trigger the problem, I haven't tried asan
but traced the *repalloc* in gdb, I will try this later when I set up my
asan building.

SELECT JSON_OBJECT(1: 1, '2': NULL, '3': 1, repeat('x', 1000): 1, 2:
repeat('a', 100) WITH UNIQUE);

The other place is json_unique_object_field_start, it is set
as a callback of JsonSemAction, and in the parse state machine,
the fname used by the callback has been correctly handled.
see [1] & [2].

[1]: https://github.com/postgres/postgres/blob/master/src/common/jsonapi.c#L1069-L1087
[2]: https://github.com/postgres/postgres/blob/master/src/common/jsonapi.c#L785-L823

>
>
> Thanks!
>
> --
> Tomas Vondra

--
Regards
Junwang Zhao

On 9/5/24 06:06, Junwang Zhao wrote:
> ...
>
> I found two other places called json_unique_check_key.
>
> One is *json_build_object_worker*, and the usage is the same as
> *json_object_agg_transfn_worker*, I fix that the same way, PSA
>
> The following sql should trigger the problem, I haven't tried asan
> but traced the *repalloc* in gdb, I will try this later when I set up my
> asan building.
>
> SELECT JSON_OBJECT(1: 1, '2': NULL, '3': 1, repeat('x', 1000): 1, 2:
> repeat('a', 100) WITH UNIQUE);
>
> The other place is json_unique_object_field_start, it is set
> as a callback of JsonSemAction, and in the parse state machine,
> the fname used by the callback has been correctly handled.
> see [1] & [2].
>
> [1]: https://github.com/postgres/postgres/blob/master/src/common/jsonapi.c#L1069-L1087
> [2]: https://github.com/postgres/postgres/blob/master/src/common/jsonapi.c#L785-L823
>

Thanks for the fix and the idea for a function triggering issue in the
other place. I verified that it indeed triggers a valgrind report, and
the fix makes it go away.

Attached is a patch with proper commit message, explaining the issue,
and various other details. Can you please proofread it and check that I
got all the details right?

The patch also adds two regression tests, triggering the issue (without
the rest of the patch applied). It took me a while to realize the
existing tests pass simply because the objects are tiny and don't
require enlarging the buffer, and thus the repalloc.

The only question that bothers me a little bit is the possibility of a
memory leak - could it happen that we keep the copied key much longer
than needed? Or does aggcontext have with the right life span? AFAICS
that's where we allocate the aggregate state, so it seems fine.

Also, how far back do we need to backpatch this? ITSM PG15 does not have
this issue, and it was introduced with the SQL/JSON stuff in PG16. Is
that correct?

regards

--
Tomas Vondra

On 9/5/24 06:06, Junwang Zhao wrote:
>
> ...
>
> I found two other places called json_unique_check_key.
>
> One is *json_build_object_worker*, and the usage is the same as
> *json_object_agg_transfn_worker*, I fix that the same way, PSA
>
> The following sql should trigger the problem, I haven't tried asan
> but traced the *repalloc* in gdb, I will try this later when I set up my
> asan building.
>
> SELECT JSON_OBJECT(1: 1, '2': NULL, '3': 1, repeat('x', 1000): 1, 2:
> repeat('a', 100) WITH UNIQUE);
>
> The other place is json_unique_object_field_start, it is set
> as a callback of JsonSemAction, and in the parse state machine,
> the fname used by the callback has been correctly handled.
> see [1] & [2].
>
> [1]: https://github.com/postgres/postgres/blob/master/src/common/jsonapi.c#L1069-L1087
> [2]: https://github.com/postgres/postgres/blob/master/src/common/jsonapi.c#L785-L823
>
>

Thanks for the fix and the idea for a function triggering issue in the
other place. I verified that it indeed triggers a valgrind report, and
the fix makes it go away.

Attached is a patch with proper commit message, explaining the issue,
and various other details. Can you please proofread it and check that I
got all the details right?

The patch also adds two regression tests, triggering the issue (without
the rest of the patch applied). It took me a while to realize the
existing tests pass simply because the objects are tiny and don't
require enlarging the buffer, and thus the repalloc.

The only question that bothers me a little bit is the possibility of a
memory leak - could it happen that we keep the copied key much longer
than needed? Or does aggcontext have with the right life span? AFAICS
that's where we allocate the aggregate state, so it seems fine.

Also, how far back do we need to backpatch this? ITSM PG15 does not have
this issue, and it was introduced with the SQL/JSON stuff in PG16. Is
that correct?

regards

--
Tomas Vondra

On 9/10/24 21:47, Tomas Vondra wrote:
> ...
>
> The only question that bothers me a little bit is the possibility of a
> memory leak - could it happen that we keep the copied key much longer
> than needed? Or does aggcontext have with the right life span? AFAICS
> that's where we allocate the aggregate state, so it seems fine.
>
> Also, how far back do we need to backpatch this? ITSM PG15 does not have
> this issue, and it was introduced with the SQL/JSON stuff in PG16. Is
> that correct?
>

Nah, I spent a bit of time looking for a memory leak, but I don't think
there's one, or at least not a new one. We use the same memory context
as for the hash table / buffer, so that should be fine.

But this made me realize the code in json_build_object_worker() can
simply use pstrdup() to copy the key into CurrentMemoryContext, which is
where the hash table of unique keys is. In fact, using unique_check.mcxt
would not be quite right:

MemoryContext mcxt; /* context for saving skipped keys */

And this has nothing to do with skipped keys.

So I adjusted that way and pushed.

Thanks for the report / patch.

--
Tomas Vondra

Hi Tomas,

On Wed, Sep 11, 2024 at 8:08 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> On 9/10/24 21:47, Tomas Vondra wrote:
> > ...
> >
> > The only question that bothers me a little bit is the possibility of a
> > memory leak - could it happen that we keep the copied key much longer
> > than needed? Or does aggcontext have with the right life span? AFAICS
> > that's where we allocate the aggregate state, so it seems fine.
> >
> > Also, how far back do we need to backpatch this? ITSM PG15 does not have
> > this issue, and it was introduced with the SQL/JSON stuff in PG16. Is
> > that correct?
> >
>
> Nah, I spent a bit of time looking for a memory leak, but I don't think
> there's one, or at least not a new one. We use the same memory context
> as for the hash table / buffer, so that should be fine.
>
> But this made me realize the code in json_build_object_worker() can
> simply use pstrdup() to copy the key into CurrentMemoryContext, which is
> where the hash table of unique keys is. In fact, using unique_check.mcxt
> would not be quite right:
>
> MemoryContext mcxt; /* context for saving skipped keys */
>
> And this has nothing to do with skipped keys.
>
> So I adjusted that way and pushed.
>

I didn't get the time to reply to you quickly, sorry about that.
Thank you for improving the patch and appreciate your time
for working on this.

>
>
> Thanks for the report / patch.
>
> --
> Tomas Vondra

--
Regards
Junwang Zhao