From: Tom Lane <tgl@sss.pgh.pa.us>
To: Daniel Gustafsson <daniel@yesql.se>
cc: pm91.arapov@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18845: DEREF_OF_NULL.RET guc_malloc possibly returns NULL
In-reply-to: <E132D362-A669-4606-AFE1-B45C9DFCC141@yesql.se>
References: <18845-582c6e10247377ec@postgresql.org>
 <A664B6DF-7D34-4004-AC89-66B1517B3301@yesql.se>
 <3051193.1741961099@sss.pgh.pa.us>
 <E132D362-A669-4606-AFE1-B45C9DFCC141@yesql.se>
Comments: In-reply-to Daniel Gustafsson <daniel@yesql.se>
	message dated "Fri, 14 Mar 2025 23:10:27 +0100"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3487875.1742059377.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sat, 15 Mar 2025 13:22:57 -0400
Message-ID: <3487876.1742059377@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3487876.1742059377%40sss.pgh.pa.us>
Precedence: bulk

Daniel Gustafsson <daniel@yesql.se> writes:
> On 14 Mar 2025, at 15:04, Tom Lane <tgl@sss.pgh.pa.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 an=
d 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