> On 14 Dec 2022, at 13:58, Mats Kindahl <mats@timescale.com> wrote:
> If strdup() fails to allocate memory for the strings, it will return NULL and the dereference at the later lines will
causea segmentation fault, which will bring down the server. There seems to be code that reads the start packet between
thesetwo lines of code, but I can trigger a segmentation fault without having a correct user. This seems unnecessary.
Ugh.
> A tentative patch to fix this just to check the allocation and exit the process if there is not enough memory, taking
careto 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);
> + }
> +
Something like this (with the comment in the right syntax) seems like an
appropriate fix.
> However, an alternative patch is to not use strdup() or any other functions that allocate memory on the heap and
insteaduse pstrdup(), pmalloc() and friends. Not sure if there are any reasons to not use those in this function, but
itseems like the memory context is correctly set up when BackendInitialize() is called. I have attached a patch for
thatas well. In this case, it is not necessary to check the return value since MemoryContextAlloc() will report an
errorif it fails to allocate memory and not return NULL.
I think using pstrdup would require changing over to PostmasterContext (or
TopMemoryContext even?) to ensure these allocations are around for the lifetime
of the backend.
A related issue is that we in PostmasterMain strdup into output_config_variable
during option parsing, but we don't check for success. When we then go about
checking if -C was set, we can't tell if it wasn't at all set or if the
strdup() call failed. Another one is the -D option where a failure to strdup
will silently fall back to $PGDATA. Both of these should IMO check for strdup
returning NULL and exit in case it does.
--
Daniel Gustafsson https://vmware.com/
