Message-ID: <bef5165f-d83b-4ffd-9f7b-79fcb9ef99e1@vondra.me>
Date: Wed, 11 Sep 2024 14:08:23 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: BUG #18598: AddressSanitizer detects use after free inside
 json_unique_hash_match()
From: Tomas Vondra <tomas@vondra.me>
To: Junwang Zhao <zhjwpku@gmail.com>
Cc: exclusion@gmail.com,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <18598-3279ed972a2347c7@postgresql.org>
 <98481f61-abf6-46df-8e83-82f6ff66cefb@vondra.me>
 <CAEG8a3JraHA4bkdPjp6izqQL39zPDBAy8L4jUb63Hq4SWp3hgA@mail.gmail.com>
 <974b94bb-45a7-4472-9826-c2323f40228a@vondra.me>
 <CAEG8a3JjH0ReJF2_O7-8LuEbO69BxPhYeXs95_x7+H9AMWF1gw@mail.gmail.com>
 <4dd0cac2-d008-404c-bf03-bea532315b2e@vondra.me>
Content-Language: en-US
In-Reply-To: <4dd0cac2-d008-404c-bf03-bea532315b2e@vondra.me>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/bef5165f-d83b-4ffd-9f7b-79fcb9ef99e1%40vondra.me>
Precedence: bulk

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