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

On 9/5/24 06:06, Junwang Zhao wrote:
>
> ...
>
> 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 for the fix and the idea for a function triggering issue in the
other place. I verified that it indeed triggers a valgrind report, and
the fix makes it go away.

Attached is a patch with proper commit message, explaining the issue,
and various other details. Can you please proofread it and check that I
got all the details right?

The patch also adds two regression tests, triggering the issue (without
the rest of the patch applied). It took me a while to realize the
existing tests pass simply because the objects are tiny and don't
require enlarging the buffer, and thus the repalloc.

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?

regards

--
Tomas Vondra