From: Tom Lane <tgl@sss.pgh.pa.us>
To: Julien Rouhaud <julien.rouhaud@dalibo.com>
cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
        Andres Freund <andres@anarazel.de>
Subject: Re: segfault in HEAD when too many nested functions call
In-reply-to: <b0af9eaa-130c-60d0-9e4e-7a135b1e0c76@dalibo.com>
References: <b0af9eaa-130c-60d0-9e4e-7a135b1e0c76@dalibo.com>
Comments: In-reply-to Julien Rouhaud <julien.rouhaud@dalibo.com>
	message dated "Sat, 15 Jul 2017 04:07:03 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <12301.1500132157.1@sss.pgh.pa.us>
Date: Sat, 15 Jul 2017 11:22:37 -0400
Message-ID: <12302.1500132157@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

Julien Rouhaud <julien.rouhaud@dalibo.com> writes:
> Since b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755 (Andres in Cc), if you
> write queries which result in infinite recursion (or just too many
> nested function calls), execution ends with segfault instead of intended
> exhausted max_stack_depth:

Yes.  We discussed this before the patch went in [1].  I wanted to put
a stack depth check in ExecProcNode(), and still do.  Andres demurred,
claiming that that was too much overhead, but didn't really provide a
credible alternative.  The thread drifted off without any resolution,
but clearly we need to do something before 10.0 final.

> Please find attached a trivial patch to fix this.  I'm not sure
> ExecMakeTableFunctionResult() is the best or only place that needs to
> check the stack depth.

I don't think that that's adequate at all.

I experimented with a variant that doesn't go through
ExecMakeTableFunctionResult:

CREATE OR REPLACE FUNCTION so()
 RETURNS int
 LANGUAGE plpgsql
AS $$
DECLARE
 rec RECORD;
BEGIN
    FOR rec IN SELECT so() as x
    LOOP
        RETURN rec.x;
    END LOOP;
    RETURN NULL;
END;
$$;

SELECT so();

This manages not to crash, but I think the reason it does not is purely
accidental: "SELECT so()" has a nontrivial targetlist so we end up running
ExecBuildProjectionInfo on that, meaning that a fresh expression
compilation happens at every nesting depth, and there are
check_stack_depth calls in expression compilation.  Surely that's
something we'd try to improve someday.  Or it could accidentally get
broken by unrelated changes in the way plpgsql sets up queries to be
executed.

I still think that we really need to add a check in ExecProcNode().
Even if there's an argument to be made that every recursion would
somewhere go through ExecMakeTableFunctionResult, very large/complex
queries could result in substantial stack getting chewed up before
we get to that --- and we don't have an infinite amount of stack slop.

			regards, tom lane

[1] https://www.postgresql.org/message-id/22833.1490390175@sss.pgh.pa.us


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers