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

CC'd hackers list.

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]);
>
> 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