Re: Crash during backend start when low on memory

On Wed, Dec 14, 2022 at 2:23 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

> > On 14 Dec 2022, at 13:58, Mats Kindahl <mats(at)timescale(dot)com> wrote:
>
> > 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.
>
> Ugh.
>
> > 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);
> > + }
> > +
>
> Something like this (with the comment in the right syntax) seems like an
> appropriate fix.
>

Yeah, I fixed the comments. Patch attached. I am actually sceptical of this
approach and think using pstrdup() and friends is better. With this
approach, there are two different mechanisms for allocating memory, so
there is an inconsistency in the code that is just confusing and raises
questions rather than solves problems. Better to have a consistent approach
and use pstrdup() everywhere.

>
> > 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.
>
> 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.
>

PostmasterContext is set quite early in the procedure so it should not be a
problem to use it. I think we can use PostmasterContext rather than
TopMemoryContext. The memory for the allocations are passed down to
PostmasterMain() and used inside InitPostgres() but as far as I can tell,
they are not used after the InitPostgres() call. The PostmasterContex is
destroyed right after the call to InitPostgres() inside PostmasterMain()
and then the rest is allocated in other contexts.

>
> 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.
>

For the strdup() in PostmasterMain() we have this code quite early in the
function, so it should be safe to use pstrdup() consistently.

/*
* By default, palloc() requests in the postmaster will be allocated in
* the PostmasterContext, which is space that can be recycled by backends.
* Allocated data that needs to be available to backends should be
* allocated in TopMemoryContext.
*/
PostmasterContext = AllocSetContextCreate(TopMemoryContext,
"Postmaster",
ALLOCSET_DEFAULT_SIZES);
MemoryContextSwitchTo(PostmasterContext);

I have attached a patch for that as well, which is the approach I think is
better.

Best wishes,
Mats Kindahl, Timescale

>
> --
> Daniel Gustafsson https://vmware.com/
>
>