From: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pm91(dot)arapov(at)gmail(dot)com, pgsql-bugs(at)lists(dot)postgresql(dot)org |
Subject: | Re: BUG #18845: DEREF_OF_NULL.RET guc_malloc possibly returns NULL |
Date: | 2025-03-16 19:59:15 |
Message-ID: | 1FD7ACAC-0EBD-47D8-98B6-CC8285C571FF@yesql.se |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs |
> On 15 Mar 2025, at 18:22, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> 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.
Those are all good points, I initially didn't think we should touch the
PGC_POSTMASTER cases but you are correct that avoiding back copy pastes to
happen is a Good Thing. The attached has all these fixes added.
--
Daniel Gustafsson
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Fix-guc_malloc-calls-consistency-and-OOM-checks.patch | application/octet-stream | 8.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2025-03-16 20:49:53 | Re: BUG #18845: DEREF_OF_NULL.RET guc_malloc possibly returns NULL |
Previous Message | Tom Lane | 2025-03-16 18:01:48 | Re: ISN extension - wrong volatility level for isn_weak() function |