Postgres fdw: Bug in InitPgFdwOptions initialization can cause an esoteric crash

Hi hackers,
After doing some research I found a bug in the initialization of the
postgres_fdw_options static object.
The bug itself is that the static pointer postgres_fdw_options is
assigned when the memory is allocated
instead of getting assigned when the full initialization loop is completed.

This is roughly what the function does (with my comments explaining the bug):

static void
InitPgFdwOptions(void)
{
...
//1: checks if already initialized
/* Prevent redundant initialization. */
if (postgres_fdw_options)
return;

...
//2: allocates memory for postgres_fdw_options
postgres_fdw_options = (PgFdwOption *)
MemoryContextAlloc(TopMemoryContext,
sizeof(PgFdwOption) * num_libpq_opts +
sizeof(non_libpq_options));

popt = postgres_fdw_options;

//3: iterates over libpq_options and copies them into postgres_fdw_options:
for (lopt = libpq_options; lopt->keyword; lopt++)
{
...
// 3.1: This strdup may throw an OOM error if allocation fails (error
not well handled)
popt->keyword = MemoryContextStrdup(TopMemoryContext,
lopt->keyword);
...
}

...
}

the problem with this function is:
if this first allocation succeeds: MemoryContextAlloc
but this allocation fails: MemoryContextStrdup
it leaves a partially allocated object which will cause trubbles later.
The problem exacerbates in the first check (1: checks if already initialized)
it will not get re-initialized and checks to that pointer will cause troubles.

I instrumented the code to force the behavior and I was able to found a crasher:
postgres=# BEGIN;
BEGIN
postgres=*# CREATE EXTENSION postgres_fdw;
ERROR: out of memory
DETAIL: FAULT INJECTION: Simulated out of memory error for testing.
(this is to test allocation after failing)
CONTEXT: SQL statement "CREATE FOREIGN DATA WRAPPER postgres_fdw
HANDLER postgres_fdw_handler
VALIDATOR postgres_fdw_validator"
extension script file "postgres_fdw--1.0.sql", near line 16
postgres=!# END;
ROLLBACK
postgres=# BEGIN;
BEGIN
postgres=*# CREATE EXTENSION postgres_fdw;
CREATE EXTENSION
postgres=*# CREATE SERVER test_server
FOREIGN DATA WRAPPER postgres_fdw
OPTIONS (
host 'localhost',
dbname 'postgres'
);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
!?>

Program received signal SIGSEGV, Segmentation fault.
__strcmp_avx2 () at ../sysdeps/x86_64/multiarch/strcmp-avx2.S:994
warning: 994 ../sysdeps/x86_64/multiarch/strcmp-avx2.S: No such
file or directory
(gdb) bt
#0 __strcmp_avx2 () at ../sysdeps/x86_64/multiarch/strcmp-avx2.S:994
#1 0x00007735787705ce in is_valid_option (context=<optimized out>,
keyword=0x57b237f2ffb8 "host") at option.c:412
#2 postgres_fdw_validator (fcinfo=<optimized out>) at option.c:83
#3 0x000057b1f9b1c2fe in FunctionCall2Coll (flinfo=0x7ffca4bd0530,
collation=<optimized out>, arg1=<optimized out>, arg2=<optimized out>)
at fmgr.c:1163
#4 0x000057b1f9b1ca4a in OidFunctionCall2Coll
(functionId=functionId(at)entry=24622, collation=collation(at)entry=0,
arg1=96422953671832, arg2=arg2(at)entry=1417) at fmgr.c:1429
#5 0x000057b1f9722d12 in transformGenericOptions
(catalogId=catalogId(at)entry=1417, oldOptions=oldOptions(at)entry=0,
options=<optimized out>, fdwvalidator=24622) at foreigncmds.c:202
#6 0x000057b1f97239b6 in CreateForeignServer
(stmt=stmt(at)entry=0x57b237e357d8) at foreigncmds.c:1036
#7 0x000057b1f99ae53d in ProcessUtilitySlow
(pstate=pstate(at)entry=0x57b237e5e120, pstmt=pstmt(at)entry=0x57b237e358a8,
queryString=queryString(at)entry=0x57b237e34bd0 "CREATE SERVER
test_server\nFOREIGN DATA WRAPPER postgres_fdw\nOPTIONS (\n host
'localhost',\n dbname 'postgres'\n);",
context=context(at)entry=PROCESS_UTILITY_TOPLEVEL,
params=params(at)entry=0x0, queryEnv=queryEnv(at)entry=0x0,
qc=0x7ffca4bd0c50, dest=<optimized out>) at utility.c:1600
#8 0x000057b1f99ad6c9 in standard_ProcessUtility
(pstmt=0x57b237e358a8, queryString=0x57b237e34bd0 "CREATE SERVER
test_server\nFOREIGN DATA WRAPPER postgres_fdw\nOPTIONS (\n host
'localhost',\n dbname 'postgres'\n);",
readOnlyTree=<optimized out>, context=PROCESS_UTILITY_TOPLEVEL,
params=0x0, queryEnv=0x0, dest=0x57b237e35c68, qc=0x7ffca4bd0c50) at
utility.c:1069
#9 0x000057b1f99abdcc in PortalRunUtility
(portal=portal(at)entry=0x57b237ebaa90, pstmt=pstmt(at)entry=0x57b237e358a8,
isTopLevel=isTopLevel(at)entry=true,
setHoldSnapshot=setHoldSnapshot(at)entry=false,
dest=dest(at)entry=0x57b237e35c68,
qc=qc(at)entry=0x7ffca4bd0c50) at pquery.c:1256
#10 0x000057b1f99abf0d in PortalRunMulti
(portal=portal(at)entry=0x57b237ebaa90, isTopLevel=isTopLevel(at)entry=true,
setHoldSnapshot=setHoldSnapshot(at)entry=false,
dest=dest(at)entry=0x57b237e35c68, altdest=altdest(at)entry=0x57b237e35c68,
qc=qc(at)entry=0x7ffca4bd0c50) at pquery.c:1414
#11 0x000057b1f99ac452 in PortalRun
(portal=portal(at)entry=0x57b237ebaa90,
count=count(at)entry=9223372036854775807,
isTopLevel=isTopLevel(at)entry=true, dest=dest(at)entry=0x57b237e35c68,
altdest=altdest(at)entry=0x57b237e35c68,
qc=qc(at)entry=0x7ffca4bd0c50) at pquery.c:891
#12 0x000057b1f99a7ded in exec_simple_query
(query_string=0x57b237e34bd0 "CREATE SERVER test_server\nFOREIGN DATA
WRAPPER postgres_fdw\nOPTIONS (\n host 'localhost',\n dbname
'postgres'\n);") at postgres.c:1297
#13 0x000057b1f99a9a96 in PostgresMain (dbname=<optimized out>,
username=<optimized out>) at postgres.c:4946
#14 0x000057b1f99a3a53 in BackendMain (startup_data=<optimized out>,
startup_data_len=<optimized out>) at backend_startup.c:124
#15 0x000057b1f98e0be1 in postmaster_child_launch
(child_type=<optimized out>, child_slot=1,
startup_data=startup_data(at)entry=0x7ffca4bd1120,
startup_data_len=startup_data_len(at)entry=24,
client_sock=client_sock(at)entry=0x7ffca4bd1140)
at launch_backend.c:268
#16 0x000057b1f98e4892 in BackendStartup (client_sock=0x7ffca4bd1140)
at postmaster.c:3650
#17 ServerLoop () at postmaster.c:1727
#18 0x000057b1f98e648d in PostmasterMain (argc=argc(at)entry=5,
argv=argv(at)entry=0x57b237e2efb0) at postmaster.c:1414
#19 0x000057b1f9586c0e in main (argc=5, argv=0x57b237e2efb0) at main.c:227
(gdb) p postgres_fdw_options[0]
$1 = {keyword = 0x57b237e77bc0 "service", optcontext = 1417,
is_libpq_opt = true}
(gdb) p postgres_fdw_options[1]
$2 = {keyword = 0x57b237e784c8 "user", optcontext = 1418, is_libpq_opt = true}
(gdb) p postgres_fdw_options[2]
$3 = {keyword = 0x58900000589 <error: Cannot access memory at address
0x58900000589>, optcontext = 1417, is_libpq_opt = 137}
(gdb) p postgres_fdw_options[3]
$4 = {keyword = 0x58900000589 <error: Cannot access memory at address
0x58900000589>, optcontext = 1417, is_libpq_opt = 137}

And this is the instrumentation I added to force the crash:
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 5b539c4eeef..fa3f340809a 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -332,7 +332,22 @@ InitPgFdwOptions(void)
sizeof(PgFdwOption)
* num_libpq_opts +
sizeof(non_libpq_options));

+ /*
+ * LJEFFREY: Instrumentation to test some crafted garbage
memory allocation that could cause a crash.
+ * This is to test the error handling of the code that calls
this function.
+ * NOTE: This happens before initialization of the
postgres_fdw_options object, so good error handling
+ * shouldn't crash
+ */
+ for (int i=0; i < (sizeof(PgFdwOption) * num_libpq_opts +
+
sizeof(non_libpq_options)) / sizeof(Oid); i++)
+ {
+ ((Oid*)postgres_fdw_options)[i] = 1417;
+ }
+
popt = postgres_fdw_options;
+
+ // Instrumentation to induce error in the middle of the loop
+ int alloc_count = 0;
for (lopt = libpq_options; lopt->keyword; lopt++)
{
/* Hide debug options, as well as settings we override
internally. */
@@ -348,6 +363,13 @@ InitPgFdwOptions(void)
if (strncmp(lopt->keyword, "oauth_", strlen("oauth_")) == 0)
continue;

+ // LJEFFREY: Instrumentacion para probar out of memory
+ if (++alloc_count == 3)
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("FAULT INJECTION: Simulated
out of memory error for testing. (this is to test allocation after
failing)")));
+
popt->keyword = MemoryContextStrdup(TopMemoryContext,

lopt->keyword);

NOTE: this instrumentation is not meant to be something that may
happen in production environments, or at least the chance of happening
is extremely small
I'll send a patch in this week to see if this can be fixed easily.
Regards,
Lucas Jeffrey