Subject: Re: jsonapi: scary new warnings with LTO enabled

From: Tom Lane <[email-masked]>
Date: Thu, 17 Apr 2025 11:20:33 -0400

Daniel Gustafsson <[email-masked]> writes:
> Agreed, moving to heap allocated structures for these callsites seem much
> better. Something like the attached should be enough I think?

I confirm this silences those warnings on my Fedora 41 box.

I'm content to do it like this, but maybe Jacob wants to
investigate alternatives?

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Thu, 17 Apr 2025 08:48:05 -0700

> I confirm this silences those warnings on my Fedora 41 box.

Instead of doing

    lex = calloc(...);
    /* (error out on NULL return) */
    makeJsonLexContextCstringLen(lex, ...);

we need to do

    lex = makeJsonLexContextCstringLen(NULL, ...);
    /* (error out on NULL return) */

so that JSONLEX_FREE_STRUCT is set correctly. Otherwise we'll leak the
main allocation:

    ==12550==ERROR: LeakSanitizer: detected memory leaks
    Direct leak of 120 byte(s) in 1 object(s) allocated from:
        #0 0xaaaae34d2a84 in __interceptor_calloc
(/home/jacob/src/postgres/worktree-oauth/build-clang/src/interfaces/libpq/fuzz_libpq_handle_oauth_sasl_error+0x112a84)
(BuildId: 359bf20b63a97771ccb3bd2c238485920485521f)
        #1 0xaaaae3510ff0 in handle_oauth_sasl_error
/home/jacob/src/postgres/worktree-oauth/build-clang/../src/interfaces/libpq/fe-auth-oauth.c:511:8

> investigate alternatives?

I was more worried about it when you said you wanted to get rid of the
stack allocation API. (I like having the flexibility to choose between
the two forms, not just for performance but also for struct
embedding.) But I'm perfectly happy with just adjusting these sites.

Thanks!
--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Sat, 19 Apr 2025 23:15:24 +0200

> On 17 Apr 2025, at 17:48, Jacob Champion <[email-masked]> wrote:
> 

Since there is no way to determine if the allocation succeeded from outside of
the JSON api it might be better to keep the calloc and explicitly free it?

(Could a JsonLexContextBroken() function be useful perhaps?)

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Mon, 21 Apr 2025 08:33:28 -0700

> the JSON api it might be better to keep the calloc and explicitly free it?

I don't think so; pg_parse_json() will error out quickly, so I don't
see much advantage to the extra code. Raw performance isn't much of a
concern for the out-of-memory case, IMO.

> (Could a JsonLexContextBroken() function be useful perhaps?)

Maybe if there's ever a client that absolutely must initialize a
context before doing a bunch of independent work? But I don't think
that's true here. (Any callers we're converting from the stack API are
going to have short-lived contexts.)

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 21 Apr 2025 20:20:00 +0200

> On 21 Apr 2025, at 17:33, Jacob Champion <[email-masked]> wrote:
> 

Sure, but I fear we'll get an endless stream of static analysis reports for the
allocation leaking if we don't free it.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Mon, 21 Apr 2025 11:28:43 -0700

> allocation leaking if we don't free it.

But we do free it, in freeJsonLexContext(). That usage of the API goes
back to 2023, with 1c99cde2f344. Or am I misunderstanding?

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 21 Apr 2025 20:46:01 +0200

> On 21 Apr 2025, at 20:28, Jacob Champion <[email-masked]> wrote:
> 

We do, but with the current coding we call setJsonLexContextOwnsTokens
immediately after creation which derefences the pointer without checkinf for
allocation failure.  This means we dereference the pointer before we can check
for an OOM return from pg_parse_json which even if safe seems to violate code
readability no?

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Mon, 21 Apr 2025 11:58:34 -0700

On Mon, Apr 21, 2025 at 11:46 AM Daniel Gustafsson <[email-masked]> wrote:
> allocation failure.

Right. (The flag does nothing on the OOM sentinel.)

> This means we dereference the pointer before we can check

Personally, I'm fine with can't-fail APIs, as long as they're
documented as such. (I think the deferred error API was probably
chosen so that src/common JSON clients could be written without a lot
of pain?)

--Jacob