Re: BUG #18845: DEREF_OF_NULL.RET guc_malloc possibly returns NULL

Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> On 14 Mar 2025, at 15:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Indeed. Also, a quick survey shows a lot of inconsistency in
>> guc_malloc callers --- some are lazy and just use ERROR rather
>> than LOG-and-return. That's probably all right for PGC_POSTMASTER
>> variables (since there's no chance of continuing anyway) but
>> perhaps it's worth improving elsewhere.

> Turns out there was one more guc_malloc(LOG.. which didn't inspect the
> returned allocation in check_synchronized_standby_slots. On top of that there
> were a few non PGC_POSTMASTER check functions that could return false and let
> the GUC machinery handle it if we want to be consistent.

Patch looks good as far as it goes, but I nosed around and found
a few other things:

* After sleeping on it, I think we ought to change the
guc_malloc(ERROR,...) calls in the PGC_POSTMASTER cases too.
While this won't have any functional effect, what it does do
is remove examples of bad practice that are likely to get
copied-and-pasted to somewhere where it matters. The affected
functions seem to be
check_recovery_target_lsn
check_recovery_target_timeline
check_recovery_target_xid
check_debug_io_direct

* check_application_name and check_cluster_name are using WARNING
not LOG in their guc_strdup calls. That seems to have been
decided by flipping a coin; let's sync them with everyplace else.

* init_custom_variable uses ERROR in a guc_malloc and a guc_strdup
call. These should probably be changed to FATAL, on the same grounds
that the error levels earlier in that function are FATAL: we are
partway through initializing a newly-loaded extension, so it's
unlikely that trying to continue the session is going to end well.

I agree that in the back branches it's sufficient to fix
check_createrole_self_grant and check_synchronized_standby_slots.
The rest of this is mostly about setting good examples.

regards, tom lane