Subject: Re: SQL-standard function body
To: Julien Rouhaud <rjuju123@gmail.com>, Noah Misch <noah@leadboat.com>
Cc: Jaime Casanova <jcasanov@systemguards.com.ec>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: 
 <CAJKUy5j3N23=SuF=S=xX6mu8VX3N9qSbxiej2wMsAqZ3FB2Pow@mail.gmail.com>
 <20210315080344.wed62yy54fuse7jg@nol> <20210315081448.63vagqr3ly45l74i@nol>
 <c17ea953-0837-70ab-b408-522421697d54@2ndquadrant.com>
 <CAJKUy5giffMO31KfBxgSgcaXRC48HfhqwWB6fewAE_8cgOGe2w@mail.gmail.com>
 <20210331101256.d7h7d74o4celskt4@nol>
 <9277ef6a-bfa8-9c06-8e91-16c2892120cc@2ndquadrant.com>
 <20210403033943.dp6um65shvrovoj5@nol>
 <01c65ea6-e5bf-22f4-cacc-6d0f6853a965@2ndquadrant.com>
 <20210606044418.GA297923@rfd.leadboat.com>
 <20210606073220.42ytzr6r5bbhqh3q@nol>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Message-ID: <993a2fe6-93b9-3364-21bb-6e2d8c0fde13@enterprisedb.com>
Date: Mon, 7 Jun 2021 10:52:10 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.10.2
MIME-Version: 1.0
In-Reply-To: <20210606073220.42ytzr6r5bbhqh3q@nol>
Content-Type: multipart/mixed;
 boundary="------------F62BEB39F73530B25088A8E5"
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/993a2fe6-93b9-3364-21bb-6e2d8c0fde13%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------F62BEB39F73530B25088A8E5
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit

On 06.06.21 09:32, Julien Rouhaud wrote:
> On Sat, Jun 05, 2021 at 09:44:18PM -0700, Noah Misch wrote:
>> I get a NULL pointer dereference if the function body has a doubled semicolon:
>>
>>    create function f() returns int language sql begin atomic select 1;; end;
> 
> You don't even need a statements to reproduce the problem, a body containing
> only semi-colon(s) will behave the same.
> 
> Attached patch should fix the problem.

Your patch filters out empty statements at the parse transformation 
phase, so they are no longer present when you dump the body back out. 
So your edits in the test expected files don't fit.

I suggest we just prohibit empty statements at the parse stage.  I don't 
see a strong reason to allow them, and if we wanted to, we'd have to do 
more work, e.g., in ruleutils.c to print them back out correctly.

--------------F62BEB39F73530B25088A8E5
Content-Type: text/plain; charset=UTF-8; x-mac-type="0"; x-mac-creator="0";
 name="0001-Prevent-empty-statement-in-unquoted-SQL-function-bod.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="0001-Prevent-empty-statement-in-unquoted-SQL-function-bod.pa";
 filename*1="tch"

From 19817fa97da9afa4fd35365c6ecb968b53aff7fe Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 7 Jun 2021 10:49:36 +0200
Subject: [PATCH] Prevent empty statement in unquoted SQL function body

This currently crashes.  It was not meant to be supported, and there
is no support for example in ruleutils.c, so just prohibit it in the
parser.

Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://www.postgresql.org/message-id/20210606044418.GA297923@rfd.leadboat.com
---
 src/backend/parser/gram.y                       | 7 ++++---
 src/test/regress/expected/create_function_3.out | 8 ++++++++
 src/test/regress/sql/create_function_3.sql      | 6 ++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 9ee90e3f13..3cd30c15ec 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -893,11 +893,14 @@ stmtmulti:	stmtmulti ';' toplevel_stmt
 
 /*
  * toplevel_stmt includes BEGIN and END.  stmt does not include them, because
- * those words have different meanings in function bodys.
+ * those words have different meanings in function bodys.  Also, empty
+ * statements are allowed at the top level but not in function bodies.
  */
 toplevel_stmt:
 			stmt
 			| TransactionStmtLegacy
+			| /*EMPTY*/
+				{ $$ = NULL; }
 		;
 
 stmt:
@@ -1024,8 +1027,6 @@ stmt:
 			| VariableSetStmt
 			| VariableShowStmt
 			| ViewStmt
-			| /*EMPTY*/
-				{ $$ = NULL; }
 		;
 
 /*****************************************************************************
diff --git a/src/test/regress/expected/create_function_3.out b/src/test/regress/expected/create_function_3.out
index 5b6bc5eddb..41d8e49a23 100644
--- a/src/test/regress/expected/create_function_3.out
+++ b/src/test/regress/expected/create_function_3.out
@@ -290,6 +290,14 @@ CREATE FUNCTION functest_S_xx(x anyarray) RETURNS anyelement
     LANGUAGE SQL
     RETURN x[1];
 ERROR:  SQL function with unquoted function body cannot have polymorphic arguments
+-- error: empty statement
+CREATE FUNCTION functest_S_xx() RETURNS boolean
+    BEGIN ATOMIC
+        RETURN false;;
+    END;
+ERROR:  syntax error at or near ";"
+LINE 3:         RETURN false;;
+                             ^
 -- check reporting of parse-analysis errors
 CREATE FUNCTION functest_S_xx(x date) RETURNS boolean
     LANGUAGE SQL
diff --git a/src/test/regress/sql/create_function_3.sql b/src/test/regress/sql/create_function_3.sql
index 4b778999ed..5c28ddcbce 100644
--- a/src/test/regress/sql/create_function_3.sql
+++ b/src/test/regress/sql/create_function_3.sql
@@ -191,6 +191,12 @@ CREATE FUNCTION functest_S_xx(x anyarray) RETURNS anyelement
     LANGUAGE SQL
     RETURN x[1];
 
+-- error: empty statement
+CREATE FUNCTION functest_S_xx() RETURNS boolean
+    BEGIN ATOMIC
+        RETURN false;;
+    END;
+
 -- check reporting of parse-analysis errors
 CREATE FUNCTION functest_S_xx(x date) RETURNS boolean
     LANGUAGE SQL
-- 
2.31.1


--------------F62BEB39F73530B25088A8E5--