Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 12 May 2026 12:30:53 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Chao Li <li.evan.chao@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Fix unsafe PlannedStmt access in pg_stat_statements
Message-ID: <agKe7cS_I-60GlpD@paquier.xyz>
References: <2F91906A-F2B5-4A6B-9695-D136957D4545@gmail.com>
 <CE0E4BE8-2180-46CB-9D3C-6F42D145AB3D@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="sf0x10Q1cKmw9eot"
Content-Disposition: inline
In-Reply-To: <CE0E4BE8-2180-46CB-9D3C-6F42D145AB3D@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/agKe7cS_I-60GlpD%40paquier.xyz>
Precedence: bulk


--sf0x10Q1cKmw9eot
Content-Type: multipart/mixed; boundary="tGrYNh8ptNxdmksD"
Content-Disposition: inline


--tGrYNh8ptNxdmksD
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, May 11, 2026 at 04:11:41PM +0800, Chao Li wrote:
> On May 11, 2026, at 16:07, Chao Li <li.evan.chao@gmail.com> wrote:
>> In pgss_ProcessUtility(), there is this comment:
>> ```
>> /*
>> * CAUTION: do not access the *pstmt data structure again below here.
>> * If it was a ROLLBACK or similar, that data structure may have been
>> * freed.  We must copy everything we still need into local variables,
>> * which we did above.
>> *
>> * For the same reason, we can't risk restoring pstmt->queryId to its
>> * former value, which'd otherwise be a good idea.
>> */
>> ```
>>=20
>> The attached patch fixes this by saving pstmt->planOrigin,
>> following the same pattern already used for queryId, stmt_location,
>> and stmt_len.

Yeah, you are right.  This code should save the planOrigin but it does
not do so.

This is also pointing at a second issue that is not addressed by your
patch: there is no coverage for the assumption of this comment, or we
would have known about this issue when 3357471cf9f5 was committed.
Running the regression tests of PGSS with valgrind enabled leads to no
reports.

And that's where things got hairy on my side because it took me a
few hours before finding a case where I was able to get the following
report, where a PlannedStmt can get freed while we are in the hook
path:
=3D=3D28059=3D=3D Invalid read of size 4
=3D=3D28059=3D=3D    at 0x165E42DF: pgss_ProcessUtility (pg_stat_statements=
=2Ec:1200)
=3D=3D28059=3D=3D    by 0x158AFCD: ProcessUtility (utility.c:524)
=3D=3D28059=3D=3D    by 0x1587F3B: PortalRunUtility (pquery.c:1149)
=3D=3D28059=3D=3D    by 0x15877E9: FillPortalStore (pquery.c:1022)

This has come down to the extended protocol and a ROLLBACK inside a
CALL procedure.  The internal ROLLBACK is able to free the PlannedStmt
in pgss_ProcessUtility() in this case, after two executions.

> Oops! Forgot the attachment.

No problem.

All that said, I am attaching an updated version of the patch, for
posterity.  I'll apply that in a bit with your fix and the test on
HEAD, after more checks.
--
Michael

--tGrYNh8ptNxdmksD
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=0001-pg_stat_statements-Fix-potential-use-after-free-of-P.patch
Content-Transfer-Encoding: quoted-printable

=46rom 19281d8708facd17cd65a9ab07dd7b775496a5c9 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Tue, 12 May 2026 12:24:52 +0900
Subject: [PATCH] pg_stat_statements: Fix potential use-after-free of
 PlannedStmt

Blahblahblah.

Author: Chao Li
Co-authored-by: Michael Paquier
Discussion: https://postgr.es/m/
---
 .../pg_stat_statements/expected/plancache.out | 38 +++++++++++++++++++
 .../pg_stat_statements/pg_stat_statements.c   |  3 +-
 contrib/pg_stat_statements/sql/plancache.sql  | 19 ++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/contrib/pg_stat_statements/expected/plancache.out b/contrib/pg=
_stat_statements/expected/plancache.out
index 32bf913b2861..d0796d5693c0 100644
--- a/contrib/pg_stat_statements/expected/plancache.out
+++ b/contrib/pg_stat_statements/expected/plancache.out
@@ -216,6 +216,44 @@ SELECT calls, generic_plan_calls, custom_plan_calls, t=
oplevel, query FROM pg_sta
=20
 RESET pg_stat_statements.track;
 --
+-- Procedure with internal ROLLBACK and the extended query protocol.
+-- The PlannedStmt used in pgss_ProcessUtility() is freed by the internal
+-- ROLLBACK.
+--
+CREATE OR REPLACE PROCEDURE rollback_proc(a INOUT int) AS $$
+BEGIN
+  ROLLBACK;
+END;
+$$ LANGUAGE plpgsql;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t=20
+---
+ t
+(1 row)
+
+CALL rollback_proc($1) \parse stmt_rollback
+\bind_named stmt_rollback 1 \g
+ a=20
+---
+ 1
+(1 row)
+
+\bind_named stmt_rollback 2 \g
+ a=20
+---
+ 2
+(1 row)
+
+SELECT calls, query FROM pg_stat_statements
+  WHERE query LIKE '%rollback_proc%'
+  ORDER BY query COLLATE "C";
+ calls |         query         =20
+-------+------------------------
+     2 | CALL rollback_proc($1)
+(1 row)
+
+DROP PROCEDURE rollback_proc;
+--
 -- Cleanup
 --
 DROP FUNCTION select_one_func(int);
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_s=
tat_statements/pg_stat_statements.c
index 95a5411a39d9..a2d3ab770cc6 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1099,6 +1099,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *q=
ueryString,
 	int64		saved_queryId =3D pstmt->queryId;
 	int			saved_stmt_location =3D pstmt->stmt_location;
 	int			saved_stmt_len =3D pstmt->stmt_len;
+	PlannedStmtOrigin saved_planOrigin =3D pstmt->planOrigin;
 	bool		enabled =3D pgss_track_utility && pgss_enabled(nesting_level);
=20
 	/*
@@ -1210,7 +1211,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *q=
ueryString,
 				   NULL,
 				   0,
 				   0,
-				   pstmt->planOrigin);
+				   saved_planOrigin);
 	}
 	else
 	{
diff --git a/contrib/pg_stat_statements/sql/plancache.sql b/contrib/pg_stat=
_statements/sql/plancache.sql
index 160ced7add36..948d3e985180 100644
--- a/contrib/pg_stat_statements/sql/plancache.sql
+++ b/contrib/pg_stat_statements/sql/plancache.sql
@@ -87,6 +87,25 @@ SELECT calls, generic_plan_calls, custom_plan_calls, top=
level, query FROM pg_sta
=20
 RESET pg_stat_statements.track;
=20
+--
+-- Procedure with internal ROLLBACK and the extended query protocol.
+-- The PlannedStmt used in pgss_ProcessUtility() is freed by the internal
+-- ROLLBACK.
+--
+CREATE OR REPLACE PROCEDURE rollback_proc(a INOUT int) AS $$
+BEGIN
+  ROLLBACK;
+END;
+$$ LANGUAGE plpgsql;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+CALL rollback_proc($1) \parse stmt_rollback
+\bind_named stmt_rollback 1 \g
+\bind_named stmt_rollback 2 \g
+SELECT calls, query FROM pg_stat_statements
+  WHERE query LIKE '%rollback_proc%'
+  ORDER BY query COLLATE "C";
+DROP PROCEDURE rollback_proc;
+
 --
 -- Cleanup
 --
--=20
2.54.0


--tGrYNh8ptNxdmksD--

--sf0x10Q1cKmw9eot
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmoCnu0ACgkQnvQgOdby
QH17LxAAllX5tzkEeT61VjKP7netqRx7eKE2W99/VVhLKjRIn6tr/ImFLb9Qn0i1
V9/fECddMiLQouAEMqv6YhKSFI/x2FMcoG+rH5eiHOeRAm78OAQpgAl6AR3074Uz
YP8ED1O67SLbBCxbzPUBqrgYd0+z6RH/8yqdUn0YUayn9inppfPzRrezAnbsFto6
/Ce3vjAdRPuA1926gxy4JK5LCZY7vEf5SBQclOB42T3R1EWRr2XOQaLcS1NnQ+kL
9TUXyvmkb66JGb/ZtN1rjAkljU0NO8Uz8JAjZ88R1HevVRubfmXBalQmNK3hoHnI
c5ZGtAtLFu6XMA3FgUzKviyewdbBxp2jLXQa0dvooPG0DZRxKAclANF7gBEQic41
Rtn/GMR12+SGCF+6SiYgwpKZwMBZNf4V4QMOCUqMnGYxiI+5OrntRObPWckrQnDH
S4wiFUufCEff0iIu2yFIMevnrQDXlDnrQxNIU8+nokfoVcnOGOQaRjO+AcL0VsU7
XxKyQ1yqxwfVECNGCvbIXrAkGSLCdJptzhBBE8OfeIgd2Gz6IjX0ZXxBN5jHzP3V
KllzvE9BQqje6IzTksrg0NXWaxpFs4d3MY07c7EOHCAGs+UJxAh5p4FsUQc7bWRp
4VUIWQ7ZQoTB//WXsRig84vXhNhqXgGz6cxt88iMVtK6CWL3cxc=
=lgwj
-----END PGP SIGNATURE-----

--sf0x10Q1cKmw9eot--