Propagate sanity checks of ProcessUtility() to standard_ProcessUtility()?
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
Attachments:
utility-checks.patchtext/x-diff; charset=us-asciiDownload
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);
+
+/*
+ * Check the state of the arguments given to entry points for utility
+ * processing.
+ */
+#define UTILITY_CHECKS \
+do { \
+ Assert(IsA(pstmt, PlannedStmt)); \
+ Assert(pstmt->commandType == CMD_UTILITY); \
+ Assert(queryString != NULL); /* required as of 8.4 */ \
+ Assert(qc == NULL || qc->commandTag == 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 == CMD_UTILITY);
- Assert(queryString != NULL); /* required as of 8.4 */
- Assert(qc == NULL || qc->commandTag == CMDTAG_UNKNOWN);
+ UTILITY_CHECKS;
/*
* We provide a function hook variable that lets loadable plugins get
@@ -559,6 +570,8 @@ standard_ProcessUtility(PlannedStmt *pstmt,
ParseState *pstate;
int readonly_flags;
+ UTILITY_CHECKS;
+
/* This can recurse, so check for excessive recursion */
check_stack_depth();
On Thu, Feb 29, 2024 at 3:21 PM Michael Paquier <michael@paquier.xyz> wrote:
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.
why not just shovel these to standard_ProcessUtility.
so ProcessUtility will looking consistent with (in format)
* ExecutorStart()
* ExecutorRun()
* ExecutorFinish()
* ExecutorEnd()
On Thu, Feb 29, 2024 at 04:10:26PM +0800, jian he wrote:
why not just shovel these to standard_ProcessUtility.
so ProcessUtility will looking consistent with (in format)
* ExecutorStart()
* ExecutorRun()
* ExecutorFinish()
* ExecutorEnd()
That's one of the points of the change: checking that only in
standard_ProcessUtility() may not be sufficient for utility hooks that
don't call standard_ProcessUtility(), so you'd stil want one in
ProcessUtility().
--
Michael
On Thu, Feb 29, 2024 at 2:21 AM Michael Paquier <michael@paquier.xyz> wrote:
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?
I can't imagine a scenario where this change saves more than 5 minutes
of debugging, so I'd rather leave things the way they are. If you do
this, then people will see the macro and have to go look at what it
does, whereas right now, they can see the assertions themselves, which
is better.
The usual pattern for using hooks like this is to call the next
implementation, or the standard implementation, and pass down the
arguments that you got. If you try to do that and mess it up, you're
going to get a crash really, really quickly and it shouldn't be very
difficult at all to figure out what you did wrong. Honestly, that
doesn't seem like it would be hard even without the assertions: for
the most part, a quick glance at the stack backtrace ought to be
enough. If you're trying to do something more sophisticated, like
mutating the node tree before passing it down, the chances of your
mistakes getting caught by these assertions are pretty darn low, since
they're very superficial checks.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Fri, May 17, 2024 at 03:53:58PM -0400, Robert Haas wrote:
The usual pattern for using hooks like this is to call the next
implementation, or the standard implementation, and pass down the
arguments that you got. If you try to do that and mess it up, you're
going to get a crash really, really quickly and it shouldn't be very
difficult at all to figure out what you did wrong. Honestly, that
doesn't seem like it would be hard even without the assertions: for
the most part, a quick glance at the stack backtrace ought to be
enough. If you're trying to do something more sophisticated, like
mutating the node tree before passing it down, the chances of your
mistakes getting caught by these assertions are pretty darn low, since
they're very superficial checks.
Perhaps, still that would be something.
Hmm. We've had in the past bugs where DDL paths were playing with the
manipulation of Querys in core, corrupting their contents. It seems
like what you would mean is to have a check at the *end* of
ProcessUtility() that does an equalFuncs() on the Query, comparing it
with a copy of it taken at its beginning, all that hidden within two
USE_ASSERT_CHECKING blocks, when we'd expect the tree to not change.
With readOnlyTree, that would be just changing from one policy to
another, which is not really interesting at this stage based on how
ProcessUtility() is shaped.
--
Michael
On Fri, May 17, 2024 at 10:11 PM Michael Paquier <michael@paquier.xyz> wrote:
On Fri, May 17, 2024 at 03:53:58PM -0400, Robert Haas wrote:
The usual pattern for using hooks like this is to call the next
implementation, or the standard implementation, and pass down the
arguments that you got. If you try to do that and mess it up, you're
going to get a crash really, really quickly and it shouldn't be very
difficult at all to figure out what you did wrong. Honestly, that
doesn't seem like it would be hard even without the assertions: for
the most part, a quick glance at the stack backtrace ought to be
enough. If you're trying to do something more sophisticated, like
mutating the node tree before passing it down, the chances of your
mistakes getting caught by these assertions are pretty darn low, since
they're very superficial checks.Perhaps, still that would be something.
Hmm. We've had in the past bugs where DDL paths were playing with the
manipulation of Querys in core, corrupting their contents. It seems
like what you would mean is to have a check at the *end* of
ProcessUtility() that does an equalFuncs() on the Query, comparing it
with a copy of it taken at its beginning, all that hidden within two
USE_ASSERT_CHECKING blocks, when we'd expect the tree to not change.
With readOnlyTree, that would be just changing from one policy to
another, which is not really interesting at this stage based on how
ProcessUtility() is shaped.
I don't think I meant to imply that. I think what I feel is that
there's no real problem here, and that the patch sort of superficially
looks useful, but isn't really. I'd suggest just dropping it.
--
Robert Haas
EDB: http://www.enterprisedb.com