Merge #154541 #154740

154541: unsafesql: avoid panicking during query formatting r=angles-n-daemons a=angles-n-daemons

Part of the effort to guard access to the crdb_internal and system namespaces includes auditing override access (and denied access) to these unsafe internals. Included in this audit is the offending query which attempted to pry into these namespaces.

In multiple locations however, this auditing caused the system to panic, for different reasons. In one case, an incorrect number of annotations on the query caused a panic. Another included a plan builder which had no associated statement.

We see the process of going from plan -> query as a difficult one, and thus guard this attempt to audit these accesses in a blanket panic catcher, as it's not common that this will happen, and when it does we don't want the system to wholesale fail the query.

Fixes: #153590 Epic: CRDB-24527

Release note: none

154740: go.mod: bump Pebble to 0ac45a74e10a r=RaduBerinde a=RaduBerinde

Changes:

 * [`0ac45a74`](cockroachdb/pebble@0ac45a74) metrics: fix TestMetrics flake
 * [`98c989b1`](cockroachdb/pebble@98c989b1) metamorphic: fix bug in suffix generation when prefix == startPrefix
 * [`d37d2f4b`](cockroachdb/pebble@d37d2f4b) pebble: materialize virtual tables only if backing contains >= 30% garbage

Release note: none.
Epic: none.

Co-authored-by: Brian Dillmann <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>

Author: 3 people

DEPS.bzl

@@ -1834,10 +1834,10 @@ def go_deps():
         patches = [
             "@com_github_cockroachdb_cockroach//build/patches:com_github_cockroachdb_pebble.patch",
         ],
-        sha256 = "1fcaa5c3baecb9719f8aa46b75020d0a87927e5e2496bcfa82e689d2ffea9c6b",
-        strip_prefix = "github.com/cockroachdb/[email protected]20251002180823-347d5dc77850",
+        sha256 = "ca8b29434d9447e2e943160fa26b234cb9296d7e919e58ca2dda023a95776920",
+        strip_prefix = "github.com/cockroachdb/[email protected]20251002220049-0ac45a74e10a",
         urls = [
-            "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251002180823-347d5dc77850.zip",
+            "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251002220049-0ac45a74e10a.zip",
         ],
     )
     go_repository(

build/bazelutil/distdir_files.bzl

@@ -356,7 +356,7 @@ DISTDIR_FILES = {
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/gostdlib/com_github_cockroachdb_gostdlib-v1.19.0.zip": "c4d516bcfe8c07b6fc09b8a9a07a95065b36c2855627cb3514e40c98f872b69e",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/logtags/com_github_cockroachdb_logtags-v0.0.0-20241215232642-bb51bb14a506.zip": "920068af09e3846d9ebb4e4a7787ff1dd10f3989c5f940ad861b0f6a9f824f6e",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/metamorphic/com_github_cockroachdb_metamorphic-v0.0.0-20231108215700-4ba948b56895.zip": "28c8cf42192951b69378cf537be5a9a43f2aeb35542908cc4fe5f689505853ea",
-    "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251002180823-347d5dc77850.zip": "1fcaa5c3baecb9719f8aa46b75020d0a87927e5e2496bcfa82e689d2ffea9c6b",
+    "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251002220049-0ac45a74e10a.zip": "ca8b29434d9447e2e943160fa26b234cb9296d7e919e58ca2dda023a95776920",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/redact/com_github_cockroachdb_redact-v1.1.6.zip": "018eccb5fb9ca52d43ec9eaf213539d01c1f2b94e0e822406ebfb2e9321ef6cf",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/returncheck/com_github_cockroachdb_returncheck-v0.0.0-20200612231554-92cdbca611dd.zip": "ce92ba4352deec995b1f2eecf16eba7f5d51f5aa245a1c362dfe24c83d31f82b",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/stress/com_github_cockroachdb_stress-v0.0.0-20220803192808-1806698b1b7b.zip": "3fda531795c600daf25532a4f98be2a1335cd1e5e182c72789bca79f5f69fcc1",

go.mod

@@ -142,7 +142,7 @@ require (
 	github.com/cockroachdb/go-test-teamcity v0.0.0-20191211140407-cff980ad0a55
 	github.com/cockroachdb/gostdlib v1.19.0
 	github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506
-	github.com/cockroachdb/pebble v0.0.0-20251002180823-347d5dc77850
+	github.com/cockroachdb/pebble v0.0.0-20251002220049-0ac45a74e10a
 	github.com/cockroachdb/redact v1.1.6
 	github.com/cockroachdb/returncheck v0.0.0-20200612231554-92cdbca611dd
 	github.com/cockroachdb/stress v0.0.0-20220803192808-1806698b1b7b

go.sum

@@ -577,8 +577,8 @@ github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506 h1:ASDL+UJcILM
 github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506/go.mod h1:Mw7HqKr2kdtu6aYGn3tPmAftiP3QPX63LdK/zcariIo=
 github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895 h1:XANOgPYtvELQ/h4IrmPAohXqe2pWA8Bwhejr3VQoZsA=
 github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895/go.mod h1:aPd7gM9ov9M8v32Yy5NJrDyOcD8z642dqs+F0CeNXfA=
-github.com/cockroachdb/pebble v0.0.0-20251002180823-347d5dc77850 h1:pT7rhCsjLQJrmTxV79g6d0oSBSgJ4Mqqs448ADowab0=
-github.com/cockroachdb/pebble v0.0.0-20251002180823-347d5dc77850/go.mod h1:H/DxkYtsYVJwPFLikOL9yzb/PV7oIkz44CUmn4KecKg=
+github.com/cockroachdb/pebble v0.0.0-20251002220049-0ac45a74e10a h1:K31Go2TsBQEhEUNRSRbv63Qiu6ImEawHedYASgIyZOU=
+github.com/cockroachdb/pebble v0.0.0-20251002220049-0ac45a74e10a/go.mod h1:H/DxkYtsYVJwPFLikOL9yzb/PV7oIkz44CUmn4KecKg=
 github.com/cockroachdb/redact v1.1.6 h1:zXJBwDZ84xJNlHl1rMyCojqyIxv+7YUpQiJLQ7n4314=
 github.com/cockroachdb/redact v1.1.6/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg=
 github.com/cockroachdb/returncheck v0.0.0-20200612231554-92cdbca611dd h1:KFOt5I9nEKZgCnOSmy8r4Oykh8BYQO8bFOTgHDS8YZA=

pkg/sql/unsafesql/BUILD.bazel

@@ -14,6 +14,7 @@ go_library(
         "//pkg/util/log/eventpb",
         "//pkg/util/log/severity",
         "//pkg/util/timeutil",
+        "@com_github_cockroachdb_redact//:redact",
     ],
 )
 
@@ -26,7 +27,9 @@ go_test(
         "//pkg/security/securityassets",
         "//pkg/security/securitytest",
         "//pkg/server",
+        "//pkg/settings",
         "//pkg/sql/isql",
+        "//pkg/sql/sem/tree",
         "//pkg/sql/sqlerrors",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/sqlutils",

pkg/sql/unsafesql/unsafesql.go

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
 	"github.com/cockroachdb/cockroach/pkg/util/log/severity"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
+	"github.com/cockroachdb/redact"
 )
 
 // The accessedLogLimiter is used to limit the rate of logging unsafe internal access
@@ -53,7 +54,7 @@ func CheckInternalsAccess(
 		return nil
 	}
 
-	q := tree.FormatAstAsRedactableString(stmt, ann, sv)
+	q := SafeFormatQuery(stmt, ann, sv)
 	// If an override is set, allow access to this virtual table.
 	if sd.AllowUnsafeInternals {
 		// Log this access to the SENSITIVE_ACCESS channel since the override condition bypassed normal access controls.
@@ -69,3 +70,20 @@ func CheckInternalsAccess(
 	}
 	return sqlerrors.ErrUnsafeTableAccess
 }
+
+// SafeFormatQuery attempts to format the query for logging, but recovers from
+// any panics that may occur during formatting.
+func SafeFormatQuery(
+	stmt tree.Statement, ann *tree.Annotations, sv *settings.Values,
+) (s redact.RedactableString) {
+	if stmt == nil {
+		return "<nil statement>"
+	}
+	defer func() {
+		if r := recover(); r != nil {
+			log.Dev.Errorf(context.TODO(), "panic in SafeFormatQuery: %v", r)
+			s = "<panicked query format>"
+		}
+	}()
+	return tree.FormatAstAsRedactableString(stmt, ann, sv)
+}

pkg/sql/unsafesql/unsafesql_test.go

@@ -16,7 +16,9 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security/securityassets"
 	"github.com/cockroachdb/cockroach/pkg/security/securitytest"
 	"github.com/cockroachdb/cockroach/pkg/server"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/sql/isql"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
 	"github.com/cockroachdb/cockroach/pkg/sql/unsafesql"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -255,3 +257,35 @@ func TestAccessCheckServer(t *testing.T) {
 		})
 	}
 }
+
+// panickingStatement is a mock Statement that panics during formatting
+type panickingStatement struct{}
+
+func (ps panickingStatement) String() string {
+	return "panicking statement"
+}
+
+func (ps panickingStatement) Format(ctx *tree.FmtCtx) {
+	panic("deliberate panic for testing")
+}
+
+func (ps panickingStatement) StatementReturnType() tree.StatementReturnType {
+	return tree.Unknown
+}
+
+func (ps panickingStatement) StatementType() tree.StatementType {
+	return tree.TypeDML
+}
+
+func (ps panickingStatement) StatementTag() string {
+	return "TEST"
+}
+
+func TestPanickingSQLFormat(t *testing.T) {
+	result := unsafesql.SafeFormatQuery(panickingStatement{}, nil, &settings.Values{})
+	require.Equal(
+		t,
+		"<panicked query format>",
+		string(result),
+	)
+}