Crash during backend start when low on memory

Hi all,

We have a crash when starting up a backend that can happen when the system
is low on memory. Even though the system is probably not in good shape, it
seems unnecessary to trigger a crash and it would be preferable to just
deny creating the backend and throw an error.

Inside BackendInitialize, there is this code:

/*
* Save remote_host and remote_port in port structure (after this, they
* will appear in log_line_prefix data for log messages).
*/
port->remote_host = strdup(remote_host);
port->remote_port = strdup(remote_port);
.
.
.
appendStringInfoString(&ps_data, port->remote_host);
if (port->remote_port[0] != '\0')
appendStringInfo(&ps_data, "(%s)", port->remote_port);

There is no assignment to remote_port or remote_host in the lines between.
If strdup() fails to allocate memory for the strings, it will return NULL
and the dereference at the later lines will cause a segmentation fault,
which will bring down the server. There seems to be code that reads the
start packet between these two lines of code, but I can trigger a
segmentation fault without having a correct user. This seems unnecessary.

I can trigger the crash using a debugger:

1. Connect a debugger to the postmaster
2. Set a breakpoint on BackendStartup and continue.
3. Try to connect to the server using psql and use a non-existing user
name.
4. Set follow-fork-mode to child and set a breakpoint on
BackendInitialize and continue executing.
5. Step to the first lines above and set port->remote_port and
port->remote_host to NULL simulating a failed strdup.
6. Continue executing, and the backend will get a segfault and crash the
server.

This is probably not a big problem in normal installations. It requires you
to be out of memory to trigger this and you will run out of backends long
before you run out of memory, but if you're running in constrained
environments or in a container that is pushing the limits of the amount of
memory to allocate, this can trigger the crash.

A tentative patch to fix this just to check the allocation and exit the
process if there is not enough memory, taking care to not try to allocate
more memory. I used this patch (attached as well).

diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index 1da5752047..e4b3d1bd59 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4327,6 +4327,14 @@ BackendInitialize(Port *port)
port->remote_host = strdup(remote_host);
port->remote_port = strdup(remote_port);

+ if (port->remote_host == NULL || port->remote_port == NULL) {
+ /* Since we are out of memory, we use strerror_r and
write_stderr
+ here. They do not allocate memory and just use the
stack. */
+ char sebuf[PG_STRERROR_R_BUFLEN];
+ write_stderr("out of memory: %s", strerror_r(errno, sebuf,
sizeof(sebuf)));
+ proc_exit(1);
+ }
+
/* And now we can issue the Log_connections message, if wanted */
if (Log_connections)
{

However, an alternative patch is to not use strdup() or any other functions
that allocate memory on the heap and instead use pstrdup(), pmalloc() and
friends. Not sure if there are any reasons to not use those in this
function, but it seems like the memory context is correctly set up when
BackendInitialize() is called. I have attached a patch for that as well. In
this case, it is not necessary to check the return value since
MemoryContextAlloc() will report an error if it fails to allocate memory
and not return NULL.

Best wishes,
Mats Kindahl, Timescale