Horton
Menu

Thread

  1. DEREF_AFTER_NULL: src/common/jsonapi.c:2529

    Галкин Сергей <galkin@rutoken.ru> — 2026-04-06T08:26:18Z

    Hello, a static analyzer pointed out a possible NULL dereference at the end of json_errdetail() (src/common/jsonapi.c):

return lex->errormsg->data;

That seemed plausible to me, since there is a comment just above saying that lex->errormsg can be NULL in shlib code. I also checked PQExpBufferBroken(), and it does handle NULL, but that call is under #ifdef, while the final access to lex->errormsg->data is unconditional.

I may be missing some invariant here, but it seems worth adding an explicit NULL check. I prepared a corresponding patch and am attaching it below in case you agree that this is a real issue.

diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 1145d93945f..192040b5443 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -2525,6 +2525,9 @@ json_errdetail(JsonParseErrorType error, JsonLexContext *lex)
     if (PQExpBufferBroken(lex->errormsg))
         return _("out of memory while constructing error description");
 #endif
+
+    if (!lex->errormsg)
+        return _("out of memory while constructing error description");

     return lex->errormsg->data;
 }

Best regards, Galkin Sergey

  2. Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529

    Andreas Karlsson <andreas@proxel.se> — 2026-04-06T11:59:01Z

    On 4/6/26 10:26 AM, Галкин Сергей wrote:
> That seemed plausible to me, since there is a comment just above saying 
> that lex->errormsg can be NULL in shlib code. I also checked 
> PQExpBufferBroken(), and it does handle NULL, but that call is under 
> #ifdef, while the final access to lex->errormsg->data is unconditional.
> 
> I may be missing some invariant here, but it seems worth adding an 
> explicit NULL check. I prepared a corresponding patch and am attaching 
> it below in case you agree that this is a real issue.

The code is correct but a bit confusing. When JSONAPI_USE_PQEXPBUFFER is 
not defined jsonapi_makeStringInfo() will call palloc() which cannot 
return NULL so the NULL check (currently done by PQExpBufferBroken()) is 
only necessary when JSONAPI_USE_PQEXPBUFFER is defined.

If someone has a patch improving readability I would personally before 
merging this since this code feels more complex than it ideally should 
be but adding this noop NULL check to silence a false positive from a 
static analyzer does not seem like an improvement.

-- 
Andreas Karlsson
Percona

  3. Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529

    Andreas Karlsson <andreas@proxel.se> — 2026-04-06T12:01:41Z

    On 4/6/26 1:59 PM, Andreas Karlsson wrote:
> If someone has a patch improving readability I would personally before 
> merging this since this code feels more complex than it ideally should 
> be but adding this noop NULL check to silence a false positive from a 
> static analyzer does not seem like an improvement.

Whops, accidentally forgot to type some words there. That should have been:

"[...] I would personally be all for merging it since this code [...]"

-- 
Andreas Karlsson
Percona

  4. Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529

    Jacob Champion <jacob.champion@enterprisedb.com> — 2026-04-06T17:57:22Z

    On Mon, Apr 6, 2026 at 4:59 AM Andreas Karlsson <andreas@proxel.se> 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.

> If someone has a patch improving readability

Suggestions?

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

--Jacob

  5. Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529

    Andres Freund <andres@anarazel.de> — 2026-04-06T18:46:14Z

    Hi,

On 2026-04-06 10:57:22 -0700, Jacob Champion wrote:
> On Mon, Apr 6, 2026 at 4:59 AM Andreas Karlsson <andreas@proxel.se> 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

  6. Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529

    Jacob Champion <jacob.champion@enterprisedb.com> — 2026-04-06T21:18:59Z

    On Mon, Apr 6, 2026 at 11:46 AM 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 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].

> 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 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"? 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 different
> 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_yaWQiu54Kj5in93D5nPyw7yXj2jZnDKi73SHQ%40mail.gmail.com
[2] https://postgr.es/m/1074830.1655442689%40sss.pgh.pa.us

  7. Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529

    Andres Freund <andres@anarazel.de> — 2026-04-06T21:40:26Z

    Hi,

On 2026-04-06 14:18:59 -0700, Jacob Champion wrote:
> On Mon, Apr 6, 2026 at 11:46 AM 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 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

  8. Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529

    Jacob Champion <jacob.champion@enterprisedb.com> — 2026-04-06T22:01:14Z

    On Mon, Apr 6, 2026 at 2:40 PM Andres Freund <andres@anarazel.de> wrote:
> > 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.

I could not disagree more strongly:
- the alternative for many developers in practice is going to be a
unilateral `if (ptr) free(ptr);` anyway, losing any potential
"benefit", and
- I'm not even convinced that "lose track of whether something was
allocated" is a thing. If it was NULL, it was not allocated. If it is
not NULL, it is either allocated, or you're about to double-free
something, which has nothing to do with free(NULL). What's to "lose
track" of?

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

Which didn't work out for us, in my humble opinion, as soon as libpq
entered the equation. We don't have to name the abstraction layer the
same thing as only one of the branches of the abstraction.

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

Okay, but I'd be curious to know how widespread this position is.

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

I'm talking about libpq here; we don't link fe_memutils.c at all.

--Jacob

  9. Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529

    Peter Eisentraut <peter@eisentraut.org> — 2026-04-08T21:25:02Z

    On 07.04.26 00:01, Jacob Champion wrote:
>>> 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.
> 
> Okay, but I'd be curious to know how widespread this position is.

This was recently (as in: I still remember it) discussed:

https://www.postgresql.org/message-id/flat/cf26e970-8e92-59f1-247a-aa265235075b%40enterprisedb.com

Probably not worth opening up again.

But it seems to me that the pfree() in fe_memutils.c should 
Assert(pointer != NULL), to be consistent with the backend version.

I've also been thinking sometimes about a pfree_if_nonnull() (which 
would do { if (ptr) pfree(ptr)) }.  That would in some cases make the 
notation more compact and robust.  Maybe it could help here too?

  10. Re: DEREF_AFTER_NULL: src/common/jsonapi.c:2529

    Jacob Champion <jacob.champion@enterprisedb.com> — 2026-04-09T15:59:35Z

    On Wed, Apr 8, 2026 at 2:25 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> This was recently (as in: I still remember it) discussed:
>
> https://www.postgresql.org/message-id/flat/cf26e970-8e92-59f1-247a-aa265235075b%40enterprisedb.com
>
> Probably not worth opening up again.

Fair enough.

> But it seems to me that the pfree() in fe_memutils.c should
> Assert(pointer != NULL), to be consistent with the backend version.

From a quick meson run, it looks like my FREE() macro would be the
only thing that trips that assertion. So yeah, I guess so.

> I've also been thinking sometimes about a pfree_if_nonnull() (which
> would do { if (ptr) pfree(ptr)) }.  That would in some cases make the
> notation more compact and robust.  Maybe it could help here too?

No strong opinion there.

--Jacob