Re: Stack overflow issue

On 21/06/2023 16:45, Egor Chindyaskin wrote:
> Hello! In continuation of the topic I would like to suggest solution.
> This patch adds several checks to the vulnerable functions above.

I looked at this last patch. The depth checks are clearly better than
segfaulting, but I think we can also avoid the recursions and having to
error out. That seems nice especially for MemoryContextDelete(), which
is called at transaction cleanup.

1. CommitTransactionCommand

This is just tail recursion. The compiler will almost certainly optimize
it away unless you're using -O0. We can easily turn it into iteration
ourselves to avoid that hazard, per attached
0001-Turn-tail-recursion-into-iteration-in-CommitTransact.patch.

2. ShowTransactionStateRec

Since this is just a debugging aid, I think we can just stop recursing
if we're about to run out of stack space. Seems nicer than erroring out,
although it can still error if you run out of memory. See
0002-Avoid-stack-overflow-in-ShowTransactionStateRec.patch.

3. All the MemoryContext functions

I'm reluctant to add stack checks to these, because they are called in
places like cleaning up after transaction abort. MemoryContextDelete()
in particular. If you ereport an error, it's not clear that you can
recover cleanly; you'll leak memory if nothing else.

Fortunately MemoryContext contains pointers to parent and siblings, so
we can traverse a tree of MemoryContexts iteratively, without using stack.

MemoryContextStats() is a bit tricky, but we can put a limit on how much
it recurses, and just print a summary line if the limit is reached.
That's what we already do if a memory context has a lot of children.
(Actually, if we didn't try keep track of the # of children at each
level, to trigger the summarization, we could traverse the tree without
using stack. But a limit seems useful.)

What do you think?

--
Heikki Linnakangas
Neon (https://neon.tech)