Horton

Thread

Commits

  1. Allocate JsonLexContexts on the heap to avoid warnings

  1. jsonapi: scary new warnings with LTO enabled

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-16T21:42:41Z

    I noticed some new warnings from buildfarm member chafer,
which I'm able to reproduce locally on a Fedora 41 box
by building with "meson setup build -Db_lto=true":

ninja: Entering directory `build'
[1515/2472] Linking target src/interfaces/libpq/libpq.so.5.18
In function 'freeJsonLexContext',
    inlined from 'freeJsonLexContext' at ../src/common/jsonapi.c:688:1,
    inlined from 'handle_oauth_sasl_error' at ../src/interfaces/libpq/fe-auth-oauth.c:547:2:
../src/common/jsonapi.c:723:17: warning: 'free' called on unallocated object 'lex' [-Wfree-nonheap-object]
  723 |                 FREE(lex);
      |                 ^
../src/interfaces/libpq/fe-auth-oauth.c: In function 'handle_oauth_sasl_error':
../src/interfaces/libpq/fe-auth-oauth.c:479:24: note: declared here
  479 |         JsonLexContext lex = {0};
      |                        ^
[2407/2472] Linking target src/test/modules/test_json_parser/test_json_parser_incremental_shlib
In function 'freeJsonLexContext',
    inlined from 'freeJsonLexContext' at ../src/common/jsonapi.c:688:1,
    inlined from 'main' at ../src/test/modules/test_json_parser/test_json_parser_incremental.c:198:2:
../src/common/jsonapi.c:723:17: warning: 'free' called on unallocated object 'lex' [-Wfree-nonheap-object]
  723 |                 FREE(lex);
      |                 ^
../src/test/modules/test_json_parser/test_json_parser_incremental.c: In function 'main':
../src/test/modules/test_json_parser/test_json_parser_incremental.c:87:24: note: declared here
   87 |         JsonLexContext lex;
      |                        ^
[2426/2472] Linking target src/test/modules/test_json_parser/test_json_parser_incremental
In function 'pg_free',
    inlined from 'pfree' at ../src/common/fe_memutils.c:135:2,
    inlined from 'freeJsonLexContext' at ../src/common/jsonapi.c:723:3,
    inlined from 'freeJsonLexContext' at ../src/common/jsonapi.c:688:1,
    inlined from 'main' at ../src/test/modules/test_json_parser/test_json_parser_incremental.c:198:2:
../src/common/fe_memutils.c:107:9: warning: 'free' called on unallocated object 'lex' [-Wfree-nonheap-object]
  107 |         free(ptr);
      |         ^
../src/test/modules/test_json_parser/test_json_parser_incremental.c: In function 'main':
../src/test/modules/test_json_parser/test_json_parser_incremental.c:87:24: note: declared here
   87 |         JsonLexContext lex;
      |                        ^

AFAICT there is no actual bug here: the FREE() call is reached only if
the JSONLEX_FREE_STRUCT flag is set, which it should not be for these
call sites.  But evidently the LTO optimizer is not quite smart enough
to realize that.

It seems fairly dangerous to ignore -Wfree-nonheap-object warnings.
I feel like we ought to move to prevent these somehow.  I'm not sure
how other than giving up on stack allocation of JsonLexContexts,
though, especially if we consider the jsonapi API frozen.  But seeing
that there are only three such call sites and none of them seem in the
least performance-critical, maybe we should just do that?

			regards, tom lane

  2. Re: jsonapi: scary new warnings with LTO enabled

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-16T21:52:42Z

    > On 16 Apr 2025, at 23:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> It seems fairly dangerous to ignore -Wfree-nonheap-object warnings.
> I feel like we ought to move to prevent these somehow.

Absolutely agree.

> I'm not sure
> how other than giving up on stack allocation of JsonLexContexts,
> though, especially if we consider the jsonapi API frozen.  But seeing
> that there are only three such call sites and none of them seem in the
> least performance-critical, maybe we should just do that?

I can't see any other option really, and there is no performance angle really
so that should be safe.  Since I committed at least one of these, let me know
if you want me to tackle it.

--
Daniel Gustafsson

  3. Re: jsonapi: scary new warnings with LTO enabled

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-16T22:12:44Z

    Daniel Gustafsson <daniel@yesql.se> writes:
>> On 16 Apr 2025, at 23:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm not sure
>> how other than giving up on stack allocation of JsonLexContexts,
>> though, especially if we consider the jsonapi API frozen.  But seeing
>> that there are only three such call sites and none of them seem in the
>> least performance-critical, maybe we should just do that?

> I can't see any other option really, and there is no performance angle really
> so that should be safe.  Since I committed at least one of these, let me know
> if you want me to tackle it.

The only alternative I can see that might stop the warning is if we
can find a way to make it clearer to the optimizer that the FREE()
isn't reached.  But I'm not sure about a trustworthy way to make that
happen.  Maybe it'd work to change the signature of freeJsonLexContext
(or perhaps better, add a separate entry point) so that the caller is
passing a bool constant that controls whether to free the struct.
We could have an Assert that compares that to the state of the
JSONLEX_FREE_STRUCT flag to catch mistakes.  This seems kind of messy
though.

			regards, tom lane

  4. Re: jsonapi: scary new warnings with LTO enabled

    Ranier Vilela <ranier.vf@gmail.com> — 2025-04-16T22:17:34Z

    Em qua., 16 de abr. de 2025 às 18:42, Tom Lane <tgl@sss.pgh.pa.us> escreveu:

> I noticed some new warnings from buildfarm member chafer,
> which I'm able to reproduce locally on a Fedora 41 box
> by building with "meson setup build -Db_lto=true":
>
> ninja: Entering directory `build'
> [1515/2472] Linking target src/interfaces/libpq/libpq.so.5.18
> In function 'freeJsonLexContext',
>     inlined from 'freeJsonLexContext' at ../src/common/jsonapi.c:688:1,
>     inlined from 'handle_oauth_sasl_error' at
> ../src/interfaces/libpq/fe-auth-oauth.c:547:2:
> ../src/common/jsonapi.c:723:17: warning: 'free' called on unallocated
> object 'lex' [-Wfree-nonheap-object]
>   723 |                 FREE(lex);
>       |                 ^
> ../src/interfaces/libpq/fe-auth-oauth.c: In function
> 'handle_oauth_sasl_error':
> ../src/interfaces/libpq/fe-auth-oauth.c:479:24: note: declared here
>   479 |         JsonLexContext lex = {0};
>       |                        ^
> [2407/2472] Linking target
> src/test/modules/test_json_parser/test_json_parser_incremental_shlib
> In function 'freeJsonLexContext',
>     inlined from 'freeJsonLexContext' at ../src/common/jsonapi.c:688:1,
>     inlined from 'main' at
> ../src/test/modules/test_json_parser/test_json_parser_incremental.c:198:2:
> ../src/common/jsonapi.c:723:17: warning: 'free' called on unallocated
> object 'lex' [-Wfree-nonheap-object]
>   723 |                 FREE(lex);
>       |                 ^
> ../src/test/modules/test_json_parser/test_json_parser_incremental.c: In
> function 'main':
> ../src/test/modules/test_json_parser/test_json_parser_incremental.c:87:24:
> note: declared here
>    87 |         JsonLexContext lex;
>       |                        ^
> [2426/2472] Linking target
> src/test/modules/test_json_parser/test_json_parser_incremental
> In function 'pg_free',
>     inlined from 'pfree' at ../src/common/fe_memutils.c:135:2,
>     inlined from 'freeJsonLexContext' at ../src/common/jsonapi.c:723:3,
>     inlined from 'freeJsonLexContext' at ../src/common/jsonapi.c:688:1,
>     inlined from 'main' at
> ../src/test/modules/test_json_parser/test_json_parser_incremental.c:198:2:
> ../src/common/fe_memutils.c:107:9: warning: 'free' called on unallocated
> object 'lex' [-Wfree-nonheap-object]
>   107 |         free(ptr);
>       |         ^
> ../src/test/modules/test_json_parser/test_json_parser_incremental.c: In
> function 'main':
> ../src/test/modules/test_json_parser/test_json_parser_incremental.c:87:24:
> note: declared here
>    87 |         JsonLexContext lex;
>       |                        ^
>
> AFAICT there is no actual bug here: the FREE() call is reached only if
> the JSONLEX_FREE_STRUCT flag is set, which it should not be for these
> call sites.

See the function *makeJsonLexContextCstringLen* (line 400)
The JSONLEX_FREE_STRUCT  is enabled, no?

fe-auth-oauth.c (line 507)
makeJsonLexContextCstringLen(&lex, msg, msglen, PG_UTF8, true);

Worst, on a second call, with lex not NULL, the flags is reseted
and the struct will no longer be released?

best regards,
Ranier Vilela

  5. Re: jsonapi: scary new warnings with LTO enabled

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-16T22:18:23Z

    > On 17 Apr 2025, at 00:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 16 Apr 2025, at 23:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I'm not sure
>>> how other than giving up on stack allocation of JsonLexContexts,
>>> though, especially if we consider the jsonapi API frozen.  But seeing
>>> that there are only three such call sites and none of them seem in the
>>> least performance-critical, maybe we should just do that?
> 
>> I can't see any other option really, and there is no performance angle really
>> so that should be safe.  Since I committed at least one of these, let me know
>> if you want me to tackle it.
> 
> The only alternative I can see that might stop the warning is if we
> can find a way to make it clearer to the optimizer that the FREE()
> isn't reached.  But I'm not sure about a trustworthy way to make that
> happen.  Maybe it'd work to change the signature of freeJsonLexContext
> (or perhaps better, add a separate entry point) so that the caller is
> passing a bool constant that controls whether to free the struct.
> We could have an Assert that compares that to the state of the
> JSONLEX_FREE_STRUCT flag to catch mistakes.  This seems kind of messy
> though.

Yeah, that seems messy enough that someone down the line will go "why on earth"
and we'll have to revisit this discussion.  It can probably be made to work but
I doubt it will be worth it compared to allocating on the heap.

--
Daniel Gustafsson

  6. Re: jsonapi: scary new warnings with LTO enabled

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-16T23:04:50Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 17 Apr 2025, at 00:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The only alternative I can see that might stop the warning is if we
>> can find a way to make it clearer to the optimizer that the FREE()
>> isn't reached.  But I'm not sure about a trustworthy way to make that
>> happen.

> Yeah, that seems messy enough that someone down the line will go "why on earth"
> and we'll have to revisit this discussion.  It can probably be made to work but
> I doubt it will be worth it compared to allocating on the heap.

Looking through all of the callers of freeJsonLexContext, quite
a lot of them use local JsonLexContext structs, and probably some
of them are more performance-critical than these.  So that raises
the question of why are we seeing warnings for only these call
sites?  Maybe there is a more elegant way to suppress them.

Still, I think that observation refutes my initial thought that
we should rip out support for local JsonLexContext structs
altogether.  I'm inclined now to just do the minimal thing of
changing these callers to use an allocated struct, and call it
a day.  (BTW, there seem to be only 2 places to change not 3;
two of the warnings are pointing at the same variable.)

			regards, tom lane

  7. Re: jsonapi: scary new warnings with LTO enabled

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-04-16T23:09:40Z

    On Wed, Apr 16, 2025 at 4:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Looking through all of the callers of freeJsonLexContext, quite
> a lot of them use local JsonLexContext structs, and probably some
> of them are more performance-critical than these.  So that raises
> the question of why are we seeing warnings for only these call
> sites?

Yeah, I had the same question...

> Maybe there is a more elegant way to suppress them.

Can we brute-force ignore this particular warning site with a #pragma
(suggested in [1])?

--Jacob

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98753

  8. Re: jsonapi: scary new warnings with LTO enabled

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-16T23:28:44Z

    Jacob Champion <jacob.champion@enterprisedb.com> writes:
> On Wed, Apr 16, 2025 at 4:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Looking through all of the callers of freeJsonLexContext, quite
>> a lot of them use local JsonLexContext structs, and probably some
>> of them are more performance-critical than these.  So that raises
>> the question of why are we seeing warnings for only these call
>> sites?

> Yeah, I had the same question...

After making another pass through the callers of freeJsonLexContext,
I observe that the warnings appear in callers that use a local
variable *and* contain goto statements.  So I'm betting that the
presence of goto's causes the LTO optimizer to pull in its horns
quite a bit and thereby fail to detect the flag correlation.

>> Maybe there is a more elegant way to suppress them.

> Can we brute-force ignore this particular warning site with a #pragma
> (suggested in [1])?

That's surely not elegant :-(.  However, I don't especially want to
rewrite away the goto's in these callers ...

			regards, tom lane

  9. Re: jsonapi: scary new warnings with LTO enabled

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-17T11:43:18Z

    > On 17 Apr 2025, at 01:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Jacob Champion <jacob.champion@enterprisedb.com> writes:
>> On Wed, Apr 16, 2025 at 4:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Looking through all of the callers of freeJsonLexContext, quite
>>> a lot of them use local JsonLexContext structs, and probably some
>>> of them are more performance-critical than these.  So that raises
>>> the question of why are we seeing warnings for only these call
>>> sites?
> 
>> Yeah, I had the same question...
> 
> After making another pass through the callers of freeJsonLexContext,
> I observe that the warnings appear in callers that use a local
> variable *and* contain goto statements.  So I'm betting that the
> presence of goto's causes the LTO optimizer to pull in its horns
> quite a bit and thereby fail to detect the flag correlation.

That seems plausible given the selective warnings.

>>> Maybe there is a more elegant way to suppress them.
> 
>> Can we brute-force ignore this particular warning site with a #pragma
>> (suggested in [1])?
> 
> That's surely not elegant :-(.  However, I don't especially want to
> rewrite away the goto's in these callers ...

Agreed, moving to heap allocated structures for these callsites seem much
better. Something like the attached should be enough I think?

--
Daniel Gustafsson

  10. Re: jsonapi: scary new warnings with LTO enabled

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-17T15:20:33Z

    Daniel Gustafsson <daniel@yesql.se> 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

  11. Re: jsonapi: scary new warnings with LTO enabled

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-04-17T15:48:05Z

    On Thu, Apr 17, 2025 at 8:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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

> I'm content to do it like this, but maybe Jacob wants to
> 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

  12. Re: jsonapi: scary new warnings with LTO enabled

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-19T21:15:24Z

    > On 17 Apr 2025, at 17:48, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Thu, Apr 17, 2025 at 8:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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:

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

  13. Re: jsonapi: scary new warnings with LTO enabled

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-04-21T15:33:28Z

    On Sat, Apr 19, 2025 at 2:15 PM Daniel Gustafsson <daniel@yesql.se> 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?

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

  14. Re: jsonapi: scary new warnings with LTO enabled

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-21T18:20:00Z

    > On 21 Apr 2025, at 17:33, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Sat, Apr 19, 2025 at 2:15 PM Daniel Gustafsson <daniel@yesql.se> 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?
> 
> 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.

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

  15. Re: jsonapi: scary new warnings with LTO enabled

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-04-21T18:28:43Z

    On Mon, Apr 21, 2025 at 11:20 AM Daniel Gustafsson <daniel@yesql.se> 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.

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

--Jacob

  16. Re: jsonapi: scary new warnings with LTO enabled

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-21T18:46:01Z

    > On 21 Apr 2025, at 20:28, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Mon, Apr 21, 2025 at 11:20 AM Daniel Gustafsson <daniel@yesql.se> 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.
> 
> But we do free it, in freeJsonLexContext(). That usage of the API goes
> back to 2023, with 1c99cde2f344. Or am I misunderstanding?

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

  17. Re: jsonapi: scary new warnings with LTO enabled

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-04-21T18:58:34Z

    On Mon, Apr 21, 2025 at 11:46 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> We do, but with the current coding we call setJsonLexContextOwnsTokens
> immediately after creation which derefences the pointer without checkinf for
> allocation failure.

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

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

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

  18. Re: jsonapi: scary new warnings with LTO enabled

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-22T10:10:29Z

    > On 21 Apr 2025, at 20:58, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

> 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?)

My preference is that no operation can silently work on a failed object, but
it's not a hill (even more so given where we are in the cycle).  The attached
v3 allocates via the JSON api, no specific error handling should be required as
it's already handled today.

--
Daniel Gustafsson

  19. Re: jsonapi: scary new warnings with LTO enabled

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-04-23T00:01:44Z

    On Tue, Apr 22, 2025 at 3:10 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> My preference is that no operation can silently work on a failed object, but
> it's not a hill (even more so given where we are in the cycle).

Hm, okay. Something to talk about with Andrew and Peter, maybe.

> The attached
> v3 allocates via the JSON api, no specific error handling should be required as
> it's already handled today.

pgindent shows one whitespace change on my machine (comment
indentation), but other than that, LGTM! Fuzzers are happy too.

Thanks,
--Jacob

  20. Re: jsonapi: scary new warnings with LTO enabled

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-23T09:35:15Z

    > On 23 Apr 2025, at 02:01, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> On Tue, Apr 22, 2025 at 3:10 AM Daniel Gustafsson <daniel@yesql.se> wrote:

>> The attached
>> v3 allocates via the JSON api, no specific error handling should be required as
>> it's already handled today.
> 
> pgindent shows one whitespace change on my machine (comment
> indentation), but other than that, LGTM! Fuzzers are happy too.

Thanks for confirming, I've pushed this now.

--
Daniel Gustafsson