MIME-Version: 1.0
References: <0b32e30f2fb94ae3b7f4ee15bbb072c0@rutoken.ru>
 <107eb23a-8ebb-42bc-99c0-ca551733e94e@proxel.se>
 <CAOYmi+muy=Wtk0dv6J5HmcFTMvrPMe2MGN2X+hjZL7DKSUEXLQ@mail.gmail.com>
 <leyiwkml7kyrdxwjzhcmmi6rtyxxwofokfxhlkpbagfprmbqw2@jxdckvomcmug>
In-Reply-To: <leyiwkml7kyrdxwjzhcmmi6rtyxxwofokfxhlkpbagfprmbqw2@jxdckvomcmug>
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Mon, 6 Apr 2026 14:18:59 -0700
Message-ID: 
 <CAOYmi+mneT-m52GhWdA=XqH5SE6f5jJbAmjwL+wp+xk4j9VVpg@mail.gmail.com>
Subject: Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529
To: Andres Freund <andres@anarazel.de>
Cc: Andreas Karlsson <andreas@proxel.se>,
 =?UTF-8?B?0JPQsNC70LrQuNC9INCh0LXRgNCz0LXQuQ==?= <galkin@rutoken.ru>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAOYmi%2BmneT-m52GhWdA%3DXqH5SE6f5jJbAmjwL%2Bwp%2Bxk4j9VVpg%40mail.gmail.com>
Precedence: bulk

On Mon, Apr 6, 2026 at 11:46=E2=80=AFAM Andres Freund <andres@anarazel.de> =
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 doabl=
e 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].

> 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 understand from the server perspective that's not currently how it
works, but I'm working on all of this from the client side: the unit
tests run FRONTEND code rather than BACKEND, so I tripped over this
hazard over and over again, and I filled it in so that hopefully no
one else would have to.

> 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 argumen=
t 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"? Or else make pfree() behave like free() [2] so that we don't
have to have that particular papercut at all anymore?

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

> FWIW, it can be silenced by marking makeStringInfoExt() with
> __attribute__((returns_nonnull)). Whether that's worth doing is a differe=
nt
> question.

I wouldn't mind doing that too, necessarily, if it's still helpful
(after fixing the core issue).

Thanks!
--Jacob

[1] https://postgr.es/m/CAOYmi%2BmyshCL_yaWQiu54Kj5in93D5nPyw7yXj2jZnDKi73S=
HQ%40mail.gmail.com
[2] https://postgr.es/m/1074830.1655442689%40sss.pgh.pa.us