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

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@vondra.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