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)