Thread: 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@gmail.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@gmail.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@vondra.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@gmail.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@vondra.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