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 14:46:14 -0400
Message-ID: <leyiwkml7kyrdxwjzhcmmi6rtyxxwofokfxhlkpbagfprmbqw2@jxdckvomcmug> (raw)
In-Reply-To: <CAOYmi+muy=Wtk0dv6J5HmcFTMvrPMe2MGN2X+hjZL7DKSUEXLQ@mail.gmail.com>
References: <[email protected]>
	<[email protected]>
	<CAOYmi+muy=Wtk0dv6J5HmcFTMvrPMe2MGN2X+hjZL7DKSUEXLQ@mail.gmail.com>

Hi,

On 2026-04-06 10:57:22 -0700, Jacob Champion wrote:
> On Mon, Apr 6, 2026 at 4:59 AM Andreas Karlsson <[email protected]> wrote:
> > The code is correct but a bit confusing.
> 
> Yeah, it's not great. The need for this (security-critical!) code to
> wrangle three separate allocation conventions is error-prone, to say
> the least.

Indeed, that's quite terrible.

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.

That would require something like pstrdup_extended() to be added to both FE &
BE, but that seems doable?

I don't understand why that code needs stuff like

/*
 * Backend pfree() doesn't handle NULL pointers like the frontend's does; smooth
 * that over to reduce mental gymnastics. Avoid multiple evaluation of the macro
 * argument to avoid future hair-pulling.
 */
#define FREE(s) do {	\
	void *__v = (s);	\
	if (__v)			\
		pfree(__v);		\
} while (0)

How is the only sane answer here not to avoid ever freeing NULLs?
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.




> > adding this noop NULL check to silence a false positive from a
> > static analyzer does not seem like an improvement.
> 
> We do occasionally merge code to silence false positives, and we could
> maybe do something with pg_assume() here, but I agree that it'd be
> better to refactor it so that it's obviously correct.

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


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: <leyiwkml7kyrdxwjzhcmmi6rtyxxwofokfxhlkpbagfprmbqw2@jxdckvomcmug>

* 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