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(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