Merge #151159

151159: sql: fix a crash with `crdb_internal.execute_internally` in an edge case r=yuzefovich a=yuzefovich

A recent thread reminded me of an issue we discovered a year ago in a support ticket https://github.com/cockroachlabs/support/issues/2954 where we could crash a node with SHOW COMMIT TIMESTAMP when run via `crdb_internal.execute_internally` builtin. It was a mistake that when I added this builtin, I prohibited stmts that have `Ack` return type: the goal was to prohibit stmts that modify txn state, which are marked with `TCL` stmt type. We already included this properly in the SQL shell API, but it was missing for the builtin, and this is now fixed.

I considered only using TCL as the prohibition marker, but after reviewing stmts that have Ack return type and are not TCL, I found only one (TRUNCATE) that seems useful to allow whereas others (like AlterTenant..., Cursor-related) could be problematic, so I kept the Ack as prohibition marker and added an exception for TRUNCATE.

Two spots where we check this are now unified.

Epic: None
Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>

Author: craig[bot]

pkg/server/api_v2_sql.go

@@ -472,12 +472,7 @@ func (a *apiV2Server) execSQL(w http.ResponseWriter, r *http.Request) {
 						}
 					}()
 
-					if returnType == tree.Ack || stmt.stmt.AST.StatementType() == tree.TypeTCL {
-						// We want to disallow statements that modify txn state (like
-						// BEGIN and COMMIT) because the internal executor does not
-						// expect such statements. We'll lean on the safe side and
-						// prohibit all statements with an ACK return type, similar
-						// to the builtin `crdb_internal.execute_internally(...)`.
+					if !tree.UserStmtAllowedForInternalExecutor(stmt.stmt.AST) {
 						return errors.New("disallowed statement type")
 					}
 

pkg/sql/opt/exec/execbuilder/testdata/execute_internally_builtin

@@ -24,6 +24,12 @@ SELECT crdb_internal.execute_internally('SELECT k FROM t;', true);
 3
 5
 
+statement ok
+SELECT crdb_internal.execute_internally('TRUNCATE t;', true);
+
+query empty
+SELECT crdb_internal.execute_internally('SELECT k FROM t;', true);
+
 # Set the database via the override.
 query T rowsort
 SELECT crdb_internal.execute_internally('EXPLAIN SELECT k FROM t;', 'Database=test');
@@ -281,3 +287,7 @@ SELECT crdb_internal.execute_internally('SHOW disallow_full_table_scans;', true)
 off
 
 subtest end
+
+# Regression test for crashing with SHOW COMMIT TIMESTAMP.
+statement error this statement is disallowed
+SELECT crdb_internal.execute_internally('SHOW COMMIT TIMESTAMP;', true);

pkg/sql/sem/builtins/generator_builtins.go

@@ -4068,12 +4068,7 @@ func makeInternallyExecutedQueryGeneratorOverload(
 			if len(stmts) != 1 {
 				return nil, errors.Newf("only one statement is supported, %d were given", len(stmts))
 			}
-			if stmts[0].AST.StatementReturnType() == tree.Ack {
-				// We want to disallow statements that modify txn state (like
-				// BEGIN and COMMIT). Such statements (as well as some others
-				// like changing cluster settings and dealing with prepared
-				// statements) have the Ack return type, so we'll lean on the
-				// safe side and prohibit them all.
+			if !tree.UserStmtAllowedForInternalExecutor(stmts[0].AST) {
 				return nil, errors.New("this statement is disallowed")
 			}
 			var sessionBound bool

pkg/sql/sem/tree/stmt.go

@@ -206,7 +206,22 @@ func ReturnsAtMostOneRow(stmt Statement) bool {
 		return true
 	}
 	return false
+}
 
+// UserStmtAllowedForInternalExecutor returns whether the user-provided stmt is
+// allowed to be executed via the internal executor.
+func UserStmtAllowedForInternalExecutor(stmt Statement) bool {
+	if stmt.StatementType() == TypeTCL || stmt.StatementReturnType() == Ack {
+		// We need to disallow stmts that modify txn state (i.e. TCL) since the
+		// internal executor doesn't support them.
+		//
+		// Additionally, out of caution, we disallow stmts that have the Ack
+		// return type (which include some AlterTenant*, Cursor-related, and a
+		// few others). The only exception that seems nice to allow is TRUNCATE.
+		_, isTruncate := stmt.(*Truncate)
+		return isTruncate
+	}
+	return true
 }
 
 // HiddenFromShowQueries is a pseudo-interface to be implemented