public inbox for [email protected]  

help / color / mirror / Atom feed

From: Andres Freund <[email protected]>
To: Jacob Champion <[email protected]>
Cc: Andreas Karlsson <[email protected]>
Cc: Галкин Сергей <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529
Date: Mon, 6 Apr 2026 17:40:26 -0400
Message-ID: <mazv3o3ua5pbvy5xwrz6n6zu6gyxkqtohbdohu3o73xyhtwa26@xcbb7ro2o42k> (raw)
In-Reply-To: <CAOYmi+mneT-m52GhWdA=XqH5SE6f5jJbAmjwL+wp+xk4j9VVpg@mail.gmail.com>
References: <[email protected]>
	<[email protected]>
	<CAOYmi+muy=Wtk0dv6J5HmcFTMvrPMe2MGN2X+hjZL7DKSUEXLQ@mail.gmail.com>
	<leyiwkml7kyrdxwjzhcmmi6rtyxxwofokfxhlkpbagfprmbqw2@jxdckvomcmug>
	<CAOYmi+mneT-m52GhWdA=XqH5SE6f5jJbAmjwL+wp+xk4j9VVpg@mail.gmail.com>

Hi,

On 2026-04-06 14:18:59 -0700, Jacob Champion wrote:
> On Mon, Apr 6, 2026 at 11:46 AM Andres Freund <[email protected]> wrote:
> > I guess getting rid of the stringinfo/pqexpbuffer split is not that easy, but
> > at least the common memory allocation stuff seems like it should be doable to
> > put through through the same wrappers for both FE/BE, handling whether we want
> > to error out or not by passing MCXT_ALLOC_NO_OOM or not.
> 
> We can spell the abstraction layer differently, but how does that help
> us hide the complexity of the OOM behavior? IMO the difference in
> returning NULLs is the entire reason this code is difficult; the
> abstraction layer must necessarily leak [1].

It's not all the complexity, but I think the various indirections do add to
making it hard to understand.


> > How is the only sane answer here not to avoid ever freeing NULLs?
> 
> Maybe I didn't parse this question correctly, but I don't want to
> avoid freeing NULLs. It's entirely reasonable and normal to write code
> that frees NULLs.

I think it's a bad idea ever call free(), realloc() etc with a NULL.  It's imo
quite the code smell indicating that code lost of track of whether something
was allocated or not.


> > Particularly because this code started out as backend code. Yea, yea, we
> > probably didn't have NULLs due to erroring out on allocation failure, but
> > still.
> >
> > Kinda seems like the fe_memutils.c pfree() should assert that the argument is
> > not null.
> 
> Maybe... but if we want to change this, I hope that we'll instead
> consider not naming a function "pfree" when sometimes it is actually
> "free"?

The whole point of having pfree() in FE code is to make it possible to write
common code that doesn't need ifdef around every allocation.  I don't see what
the gain of this would be.


> Or else make pfree() behave like free() [2] so that we don't
> have to have that particular papercut at all anymore?

-many

It's also not a path I want to add any unnecessary instructions to.


> It still doesn't help the OOM abstraction leak between libpq and the
> backend, as far as I can tell.

If the code were to use a JsonLexContext field to decide whether to pass
MCXT_ALLOC_NO_OOM to the allocation functions etc it'd at least would make the
code more similar between FE/BE due to both having to deal with NULLs.

That would require adding some optionally OOM safe functions to stringinfo,
but I suspect that would be a good thing anyway (might not be able to do it
with the existing functions, some paths that use stringinfo are quite perf
sensitive, and it does make some code nontrivially more complicated).


Greetings,

Andres Freund

view thread (10+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529
  In-Reply-To: <mazv3o3ua5pbvy5xwrz6n6zu6gyxkqtohbdohu3o73xyhtwa26@xcbb7ro2o42k>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox