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

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