From: Daniel Gustafsson <daniel@yesql.se>
Message-Id: <E132D362-A669-4606-AFE1-B45C9DFCC141@yesql.se>
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_76A92A2B-3CC0-41A0-8651-BFFE471730EA"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3776.700.51.11.1\))
Subject: Re: BUG #18845: DEREF_OF_NULL.RET guc_malloc possibly returns NULL
Date: Fri, 14 Mar 2025 23:10:27 +0100
In-Reply-To: <3051193.1741961099@sss.pgh.pa.us>
Cc: pm91.arapov@gmail.com,
 pgsql-bugs@lists.postgresql.org
To: Tom Lane <tgl@sss.pgh.pa.us>
References: <18845-582c6e10247377ec@postgresql.org>
 <A664B6DF-7D34-4004-AC89-66B1517B3301@yesql.se>
 <3051193.1741961099@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/E132D362-A669-4606-AFE1-B45C9DFCC141%40yesql.se>
Precedence: bulk


--Apple-Mail=_76A92A2B-3CC0-41A0-8651-BFFE471730EA
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

> On 14 Mar 2025, at 15:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>=20
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Why would we want FATAL here?  Wouldn't it be better to return false =
like how
>> other check_ functions already do?
>=20
> 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.

The fix for check_createrole_self_grant should go down to v16 and the =
fix for
check_synchronized_standby_slots down to 17, the other ones aren't bugs =
today
so that would be a changed behaviour in backbranches.

--
Daniel Gustafsson


--Apple-Mail=_76A92A2B-3CC0-41A0-8651-BFFE471730EA
Content-Disposition: attachment;
	filename=v1-0001-Fix-guc_malloc-calls-to-check-for-OOM-and-return-.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="v1-0001-Fix-guc_malloc-calls-to-check-for-OOM-and-return-.patch"
Content-Transfer-Encoding: quoted-printable

=46rom=2039de8e1567ff0631ab30ac010f6393a028ecdac4=20Mon=20Sep=2017=20=
00:00:00=202001=0AFrom:=20Daniel=20Gustafsson=20=
<dgustafsson@postgresql.org>=0ADate:=20Fri,=2014=20Mar=202025=2022:56:58=20=
+0100=0ASubject:=20[PATCH=20v1]=20Fix=20guc_malloc=20calls=20to=20check=20=
for=20OOM=20and=20return=20false=0A=0Acheck_createrole_self_grant=20and=20=
check_synchronized_standby_slots=0Awere=20allocating=20memory=20on=20a=20=
LOG=20elevel=20without=20checking=20if=20the=0Aallocation=20succeeded=20=
or=20not,=20which=20would=20have=20led=20to=20a=20segfault=0Aif=20it=20=
didn't.=20=20The=20remaining=20calls=20were=20using=20the=20ERROR=20=
level=0Aand=20relied=20on=20erroring=20out=20rather=20than=20returnin=20=
false=20to=20let=0Athe=20GUC=20machinery=20handle=20it=20gracefully.=0A=0A=
Author:=20Daniel=20Gustafsson=20<daniel@yesql.se>=0AReported-by:=20=
Nikita=20<pm91.arapov@gmail.com>=0ABug:=20#18845=0ADiscussion:=20=
https://postgr.es/m/18845-582c6e10247377ec@postgresql.org=0A---=0A=20=
src/backend/access/transam/xlog.c=20=20|=204=20+++-=0A=20=
src/backend/commands/user.c=20=20=20=20=20=20=20=20|=202=20++=0A=20=
src/backend/replication/slot.c=20=20=20=20=20|=202=20++=0A=20=
src/backend/tcop/backend_startup.c=20|=204=20+++-=0A=20=
src/backend/tcop/postgres.c=20=20=20=20=20=20=20=20|=204=20+++-=0A=20=
src/backend/utils/error/elog.c=20=20=20=20=20|=208=20++++++--=0A=206=20=
files=20changed,=2019=20insertions(+),=205=20deletions(-)=0A=0Adiff=20=
--git=20a/src/backend/access/transam/xlog.c=20=
b/src/backend/access/transam/xlog.c=0Aindex=20799fc739e18..0c4c5b9a368=20=
100644=0A---=20a/src/backend/access/transam/xlog.c=0A+++=20=
b/src/backend/access/transam/xlog.c=0A@@=20-4791,7=20+4791,9=20@@=20=
check_wal_consistency_checking(char=20**newval,=20void=20**extra,=20=
GucSource=20source)=0A=20=09list_free(elemlist);=0A=20=0A=20=09/*=20=
assign=20new=20value=20*/=0A-=09*extra=20=3D=20guc_malloc(ERROR,=20=
(RM_MAX_ID=20+=201)=20*=20sizeof(bool));=0A+=09*extra=20=3D=20=
guc_malloc(LOG,=20(RM_MAX_ID=20+=201)=20*=20sizeof(bool));=0A+=09if=20=
(!*extra)=0A+=09=09return=20false;=0A=20=09memcpy(*extra,=20=
newwalconsistency,=20(RM_MAX_ID=20+=201)=20*=20sizeof(bool));=0A=20=09=
return=20true;=0A=20}=0Adiff=20--git=20a/src/backend/commands/user.c=20=
b/src/backend/commands/user.c=0Aindex=208ae510c623b..0d638e29d00=20=
100644=0A---=20a/src/backend/commands/user.c=0A+++=20=
b/src/backend/commands/user.c=0A@@=20-2566,6=20+2566,8=20@@=20=
check_createrole_self_grant(char=20**newval,=20void=20**extra,=20=
GucSource=20source)=0A=20=09list_free(elemlist);=0A=20=0A=20=09result=20=
=3D=20(unsigned=20*)=20guc_malloc(LOG,=20sizeof(unsigned));=0A+=09if=20=
(!result)=0A+=09=09return=20false;=0A=20=09*result=20=3D=20options;=0A=20=
=09*extra=20=3D=20result;=0A=20=0Adiff=20--git=20=
a/src/backend/replication/slot.c=20b/src/backend/replication/slot.c=0A=
index=20719e531eb90..646ba2a78fa=20100644=0A---=20=
a/src/backend/replication/slot.c=0A+++=20=
b/src/backend/replication/slot.c=0A@@=20-2730,6=20+2730,8=20@@=20=
check_synchronized_standby_slots(char=20**newval,=20void=20**extra,=20=
GucSource=20source)=0A=20=0A=20=09/*=20GUC=20extra=20value=20must=20be=20=
guc_malloc'd,=20not=20palloc'd=20*/=0A=20=09config=20=3D=20=
(SyncStandbySlotsConfigData=20*)=20guc_malloc(LOG,=20size);=0A+=09if=20=
(!config)=0A+=09=09return=20false;=0A=20=0A=20=09/*=20Transform=20the=20=
data=20into=20SyncStandbySlotsConfigData=20*/=0A=20=09config->nslotnames=20=
=3D=20list_length(elemlist);=0Adiff=20--git=20=
a/src/backend/tcop/backend_startup.c=20=
b/src/backend/tcop/backend_startup.c=0Aindex=2027c0b3c2b04..a07c59ece01=20=
100644=0A---=20a/src/backend/tcop/backend_startup.c=0A+++=20=
b/src/backend/tcop/backend_startup.c=0A@@=20-1078,7=20+1078,9=20@@=20=
check_log_connections(char=20**newval,=20void=20**extra,=20GucSource=20=
source)=0A=20=09=20*=20We=20succeeded,=20so=20allocate=20`extra`=20and=20=
save=20the=20flags=20there=20for=20use=20by=0A=20=09=20*=20=
assign_log_connections().=0A=20=09=20*/=0A-=09*extra=20=3D=20=
guc_malloc(ERROR,=20sizeof(int));=0A+=09*extra=20=3D=20guc_malloc(LOG,=20=
sizeof(int));=0A+=09if=20(!*extra)=0A+=09=09return=20false;=0A=20=09=
*((int=20*)=20*extra)=20=3D=20flags;=0A=20=0A=20=09return=20true;=0Adiff=20=
--git=20a/src/backend/tcop/postgres.c=20b/src/backend/tcop/postgres.c=0A=
index=2055ab2da299b..0950a7bd1a2=20100644=0A---=20=
a/src/backend/tcop/postgres.c=0A+++=20b/src/backend/tcop/postgres.c=0A@@=20=
-3617,7=20+3617,9=20@@=20check_restrict_nonsystem_relation_kind(char=20=
**newval,=20void=20**extra,=20GucSource=20so=0A=20=09=
list_free(elemlist);=0A=20=0A=20=09/*=20Save=20the=20flags=20in=20=
*extra,=20for=20use=20by=20the=20assign=20function=20*/=0A-=09*extra=20=3D=
=20guc_malloc(ERROR,=20sizeof(int));=0A+=09*extra=20=3D=20=
guc_malloc(LOG,=20sizeof(int));=0A+=09if=20(!*extra)=0A+=09=09return=20=
false;=0A=20=09*((int=20*)=20*extra)=20=3D=20flags;=0A=20=0A=20=09return=20=
true;=0Adiff=20--git=20a/src/backend/utils/error/elog.c=20=
b/src/backend/utils/error/elog.c=0Aindex=20860bbd40d42..b891dab3bf6=20=
100644=0A---=20a/src/backend/utils/error/elog.c=0A+++=20=
b/src/backend/utils/error/elog.c=0A@@=20-2198,7=20+2198,9=20@@=20=
check_backtrace_functions(char=20**newval,=20void=20**extra,=20GucSource=20=
source)=0A=20=09=20*=20whitespace=20chars=20to=20save=20some=20memory,=20=
but=20it=20doesn't=20seem=20worth=20the=0A=20=09=20*=20trouble.=0A=20=09=20=
*/=0A-=09someval=20=3D=20guc_malloc(ERROR,=20newvallen=20+=201=20+=201);=0A=
+=09someval=20=3D=20guc_malloc(LOG,=20newvallen=20+=201=20+=201);=0A+=09=
if=20(!someval)=0A+=09=09return=20false;=0A=20=09for=20(i=20=3D=200,=20j=20=
=3D=200;=20i=20<=20newvallen;=20i++)=0A=20=09{=0A=20=09=09if=20=
((*newval)[i]=20=3D=3D=20',')=0A@@=20-2283,7=20+2285,9=20@@=20=
check_log_destination(char=20**newval,=20void=20**extra,=20GucSource=20=
source)=0A=20=09pfree(rawstring);=0A=20=09list_free(elemlist);=0A=20=0A-=09=
myextra=20=3D=20(int=20*)=20guc_malloc(ERROR,=20sizeof(int));=0A+=09=
myextra=20=3D=20(int=20*)=20guc_malloc(LOG,=20sizeof(int));=0A+=09if=20=
(!myextra)=0A+=09=09return=20false;=0A=20=09*myextra=20=3D=20newlogdest;=0A=
=20=09*extra=20=3D=20myextra;=0A=20=0A--=20=0A2.39.3=20(Apple=20Git-146)=0A=
=0A=

--Apple-Mail=_76A92A2B-3CC0-41A0-8651-BFFE471730EA--