Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 29 Feb 2024 16:20:53 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Propagate sanity checks of ProcessUtility() to
 standard_ProcessUtility()?
Message-ID: <ZeAwVfUqGtRCx8ux@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="W48wypHRhaotZjBh"
Content-Disposition: inline
Archived-At: <https://www.postgresql.org/message-id/ZeAwVfUqGtRCx8ux%40paquier.xyz>
Precedence: bulk


--W48wypHRhaotZjBh
Content-Type: multipart/mixed; boundary="XNoBAAwkRLY0LdEB"
Content-Disposition: inline


--XNoBAAwkRLY0LdEB
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi all,

It's been brought to me that an extension may finish by breaking the
assumptions ProcessUtility() relies on when calling
standard_ProcessUtility(), causing breakages when passing down data to
cascading utility hooks.

Isn't the state of the arguments given something we should check not
only in the main entry point ProcessUtility() but also in
standard_ProcessUtility(), to prevent issues if an extension
incorrectly manipulates the arguments it needs to pass down to other
modules that use the utility hook, like using a NULL query string?

See the attached for the idea.
Thanks,
--
Michael

--XNoBAAwkRLY0LdEB
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="utility-checks.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 8de821f960..ee82551dde 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -88,6 +88,20 @@ static void ProcessUtilitySlow(ParseState *pstate,
 							   QueryCompletion *qc);
 static void ExecDropStmt(DropStmt *stmt, bool isTopLevel);
=20
+
+/*
+ * Check the state of the arguments given to entry points for utility
+ * processing.
+ */
+#define UTILITY_CHECKS \
+do { \
+	Assert(IsA(pstmt, PlannedStmt)); \
+	Assert(pstmt->commandType =3D=3D CMD_UTILITY); \
+	Assert(queryString !=3D NULL);	/* required as of 8.4 */ \
+	Assert(qc =3D=3D NULL || qc->commandTag =3D=3D CMDTAG_UNKNOWN); \
+} while (0)
+
+
 /*
  * CommandIsReadOnly: is an executable query read-only?
  *
@@ -512,10 +526,7 @@ ProcessUtility(PlannedStmt *pstmt,
 			   DestReceiver *dest,
 			   QueryCompletion *qc)
 {
-	Assert(IsA(pstmt, PlannedStmt));
-	Assert(pstmt->commandType =3D=3D CMD_UTILITY);
-	Assert(queryString !=3D NULL);	/* required as of 8.4 */
-	Assert(qc =3D=3D NULL || qc->commandTag =3D=3D CMDTAG_UNKNOWN);
+	UTILITY_CHECKS;
=20
 	/*
 	 * We provide a function hook variable that lets loadable plugins get
@@ -559,6 +570,8 @@ standard_ProcessUtility(PlannedStmt *pstmt,
 	ParseState *pstate;
 	int			readonly_flags;
=20
+	UTILITY_CHECKS;
+
 	/* This can recurse, so check for excessive recursion */
 	check_stack_depth();
=20

--XNoBAAwkRLY0LdEB--

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

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

iQIyBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmXgMFUACgkQnvQgOdby
QH27+Q/4kxu+d8AZNPmnOo7FNSzB76WI7idel35ZDez7cvN0CZ80R3Y9PeYX49aO
bKucVlQyZjM2gA2+avJP+2XAbWH+xlfrNmZWij6cp0YbPB4qzdmvVZBbLbV4nOUm
DiUUPJEtdM9UlqImck/W+VHrNqQZP8gp4jTW9406w1q6OKxu7iQf1+V0ZXxfCbu9
GAD7mtdUwtkSGqjup5dJsnOTq4PYMbAU87xBU+OXzjjRm/5OdLVuVbVznTqA91Db
G4A1e1v5+pHTEafZ1p5J16MoQDjhO7vD/fpTe9MOQkVJxCjP0BMNcV7g2h/tsjdy
mjCmIJaDkjFfU7m6jAOzSWoP2Legpk51lKE8hQfwkdn9kdmwtCGnEc0lYdyBEVYX
sPINxzJgYjxXTDreqYUOqixjZ78MZZJWia+OTGcNVWOFOlxKQsg7f69tb2St8s5G
gN8z0/4qN/xGxu6BrgTD3mmq84G+/Pt4ucNEypHlE94efpy7UERN44NBtO1J6BDr
L0BKQqb/sEU6XDb3+3B5V+YDStTQ4MEXmkuYAIxYeWy2laeoqsAg2+yJ+nWDDqHg
irkaQGeKU0UYuY7d7V0NMRtnrq3sTlKKEOsV2XJf5UZg7nRyMUOGS2nFfEqZ2ypd
0RvgGhT403zFr9ASFFY3mwsbb2QD7yp1wTuvhSfYiNGpmJ+HXQ==
=LMW9
-----END PGP SIGNATURE-----

--W48wypHRhaotZjBh--