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
