Skip to content

Commit 87ceb0b

Browse files
craig[bot]angles-n-daemonsRaduBerinde
committed
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. 154867: go.mod: bump datadriven r=RaduBerinde a=RaduBerinde Bump datadriven to incorporate a fix (cockroachdb/datadriven#60). Epic: none Release note: None Co-authored-by: Brian Dillmann <[email protected]> Co-authored-by: Radu Berinde <[email protected]>
4 parents 52833a4 + 96ffcb5 + fdd44c4 + 10c45ca commit 87ceb0b

File tree

8 files changed

+72
-17
lines changed

8 files changed

+72
-17
lines changed

‎DEPS.bzl‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1764,10 +1764,10 @@ def go_deps():
17641764
name = "com_github_cockroachdb_datadriven",
17651765
build_file_proto_mode = "disable_global",
17661766
importpath = "github.com/cockroachdb/datadriven",
1767-
sha256 = "417027b5ff27774000129768f79e9ae62021b95d3ac9d3181e132dbe46b44da3",
1768-
strip_prefix = "github.com/cockroachdb/[email protected].20250911232732-d959cf14706c",
1767+
sha256 = "a7ffcef0b264d9c28c36b2f9b737ff739542f472d7614938ae507e2da269f6c2",
1768+
strip_prefix = "github.com/cockroachdb/[email protected].20251006155849-f84f9e519edd",
17691769
urls = [
1770-
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/datadriven/com_github_cockroachdb_datadriven-v1.0.3-0.20250911232732-d959cf14706c.zip",
1770+
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/datadriven/com_github_cockroachdb_datadriven-v1.0.3-0.20251006155849-f84f9e519edd.zip",
17711771
],
17721772
)
17731773
go_repository(
@@ -1834,10 +1834,10 @@ def go_deps():
18341834
patches = [
18351835
"@com_github_cockroachdb_cockroach//build/patches:com_github_cockroachdb_pebble.patch",
18361836
],
1837-
sha256 = "1fcaa5c3baecb9719f8aa46b75020d0a87927e5e2496bcfa82e689d2ffea9c6b",
1838-
strip_prefix = "github.com/cockroachdb/[email protected]20251002180823-347d5dc77850",
1837+
sha256 = "ca8b29434d9447e2e943160fa26b234cb9296d7e919e58ca2dda023a95776920",
1838+
strip_prefix = "github.com/cockroachdb/[email protected]20251002220049-0ac45a74e10a",
18391839
urls = [
1840-
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251002180823-347d5dc77850.zip",
1840+
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251002220049-0ac45a74e10a.zip",
18411841
],
18421842
)
18431843
go_repository(

‎build/bazelutil/distdir_files.bzl‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,7 @@ DISTDIR_FILES = {
347347
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/cockroach-go/v2/com_github_cockroachdb_cockroach_go_v2-v2.4.1.zip": "ba646db91152f3121a6812c7b74d12d8c0e126f7b4d3b927618b159692ceb424",
348348
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/crlfmt/com_github_cockroachdb_crlfmt-v0.0.0-20221214225007-b2fc5c302548.zip": "fedc01bdd6d964da0425d5eaac8efadc951e78e13f102292cc0774197f09ab63",
349349
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/crlib/com_github_cockroachdb_crlib-v0.0.0-20251001180057-2a49e1873587.zip": "539dd737ca1da53ee5c296ea0f3aad92f6d59f577e0d169dffb5a4ad706e1728",
350-
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/datadriven/com_github_cockroachdb_datadriven-v1.0.3-0.20250911232732-d959cf14706c.zip": "417027b5ff27774000129768f79e9ae62021b95d3ac9d3181e132dbe46b44da3",
350+
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/datadriven/com_github_cockroachdb_datadriven-v1.0.3-0.20251006155849-f84f9e519edd.zip": "a7ffcef0b264d9c28c36b2f9b737ff739542f472d7614938ae507e2da269f6c2",
351351
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/drpc/com_github_cockroachdb_drpc-v0.0.0-20250924114114-78d4e121902a.zip": "98b44a51f82873f93f77da80230212ab40f35044e8d38645cb1392ae03462f0b",
352352
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/errors/com_github_cockroachdb_errors-v1.12.0.zip": "f73d8a5f4d8fcbc4ed61db2b47f17e2601d8b32e9a49c0665667489d9d9d6e7c",
353353
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/go-test-teamcity/com_github_cockroachdb_go_test_teamcity-v0.0.0-20191211140407-cff980ad0a55.zip": "bac30148e525b79d004da84d16453ddd2d5cd20528e9187f1d7dac708335674b",
@@ -356,7 +356,7 @@ DISTDIR_FILES = {
356356
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/gostdlib/com_github_cockroachdb_gostdlib-v1.19.0.zip": "c4d516bcfe8c07b6fc09b8a9a07a95065b36c2855627cb3514e40c98f872b69e",
357357
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/logtags/com_github_cockroachdb_logtags-v0.0.0-20241215232642-bb51bb14a506.zip": "920068af09e3846d9ebb4e4a7787ff1dd10f3989c5f940ad861b0f6a9f824f6e",
358358
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/metamorphic/com_github_cockroachdb_metamorphic-v0.0.0-20231108215700-4ba948b56895.zip": "28c8cf42192951b69378cf537be5a9a43f2aeb35542908cc4fe5f689505853ea",
359-
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251002180823-347d5dc77850.zip": "1fcaa5c3baecb9719f8aa46b75020d0a87927e5e2496bcfa82e689d2ffea9c6b",
359+
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251002220049-0ac45a74e10a.zip": "ca8b29434d9447e2e943160fa26b234cb9296d7e919e58ca2dda023a95776920",
360360
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/redact/com_github_cockroachdb_redact-v1.1.6.zip": "018eccb5fb9ca52d43ec9eaf213539d01c1f2b94e0e822406ebfb2e9321ef6cf",
361361
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/returncheck/com_github_cockroachdb_returncheck-v0.0.0-20200612231554-92cdbca611dd.zip": "ce92ba4352deec995b1f2eecf16eba7f5d51f5aa245a1c362dfe24c83d31f82b",
362362
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/stress/com_github_cockroachdb_stress-v0.0.0-20220803192808-1806698b1b7b.zip": "3fda531795c600daf25532a4f98be2a1335cd1e5e182c72789bca79f5f69fcc1",

‎go.mod‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,12 +137,12 @@ require (
137137
github.com/cockroachdb/cockroach-go/v2 v2.4.1
138138
github.com/cockroachdb/crlfmt v0.0.0-20221214225007-b2fc5c302548
139139
github.com/cockroachdb/crlib v0.0.0-20251001180057-2a49e1873587
140-
github.com/cockroachdb/datadriven v1.0.3-0.20250911232732-d959cf14706c
140+
github.com/cockroachdb/datadriven v1.0.3-0.20251006155849-f84f9e519edd
141141
github.com/cockroachdb/errors v1.12.0
142142
github.com/cockroachdb/go-test-teamcity v0.0.0-20191211140407-cff980ad0a55
143143
github.com/cockroachdb/gostdlib v1.19.0
144144
github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506
145-
github.com/cockroachdb/pebble v0.0.0-20251002180823-347d5dc77850
145+
github.com/cockroachdb/pebble v0.0.0-20251002220049-0ac45a74e10a
146146
github.com/cockroachdb/redact v1.1.6
147147
github.com/cockroachdb/returncheck v0.0.0-20200612231554-92cdbca611dd
148148
github.com/cockroachdb/stress v0.0.0-20220803192808-1806698b1b7b

‎go.sum‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -559,8 +559,8 @@ github.com/cockroachdb/crlib v0.0.0-20251001180057-2a49e1873587 h1:qjG2TrBrPbGRV
559559
github.com/cockroachdb/crlib v0.0.0-20251001180057-2a49e1873587/go.mod h1:ae57yNis2F1FThSNdPdoXfiPOVi8G1TLreCBQYPOdqo=
560560
github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa/go.mod h1:zn76sxSg3SzpJ0PPJaLDCu+Bu0Lg3sKTORVIj19EIF8=
561561
github.com/cockroachdb/datadriven v1.0.2/go.mod h1:a9RdTaap04u637JoCzcUoIcDmvwSUtcUFtT/C3kJlTU=
562-
github.com/cockroachdb/datadriven v1.0.3-0.20250911232732-d959cf14706c h1:a0m7gmtv2mzJQ4wP9BkxCmJAnjZ7fsvCi2IORGD1als=
563-
github.com/cockroachdb/datadriven v1.0.3-0.20250911232732-d959cf14706c/go.mod h1:jsaKMvD3RBCATk1/jbUZM8C9idWBJME9+VRZ5+Liq1g=
562+
github.com/cockroachdb/datadriven v1.0.3-0.20251006155849-f84f9e519edd h1:vpWCe7VvdQbQ/9wGtlH3i+Oj+9OggKci3lsASL1ydvg=
563+
github.com/cockroachdb/datadriven v1.0.3-0.20251006155849-f84f9e519edd/go.mod h1:jsaKMvD3RBCATk1/jbUZM8C9idWBJME9+VRZ5+Liq1g=
564564
github.com/cockroachdb/drpc v0.0.0-20250924114114-78d4e121902a h1:zXCfk52Hpu2IoejmDm4Bkxmb5Nh9vxwaYOCiqA6f3YA=
565565
github.com/cockroachdb/drpc v0.0.0-20250924114114-78d4e121902a/go.mod h1:Ag2/Yfl22WZ8ywFUasRQ2brdltpX5QvY63jnYTZ3N5U=
566566
github.com/cockroachdb/errors v1.12.0 h1:d7oCs6vuIMUQRVbi6jWWWEJZahLCfJpnJSVobd1/sUo=
@@ -577,8 +577,8 @@ github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506 h1:ASDL+UJcILM
577577
github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506/go.mod h1:Mw7HqKr2kdtu6aYGn3tPmAftiP3QPX63LdK/zcariIo=
578578
github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895 h1:XANOgPYtvELQ/h4IrmPAohXqe2pWA8Bwhejr3VQoZsA=
579579
github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895/go.mod h1:aPd7gM9ov9M8v32Yy5NJrDyOcD8z642dqs+F0CeNXfA=
580-
github.com/cockroachdb/pebble v0.0.0-20251002180823-347d5dc77850 h1:pT7rhCsjLQJrmTxV79g6d0oSBSgJ4Mqqs448ADowab0=
581-
github.com/cockroachdb/pebble v0.0.0-20251002180823-347d5dc77850/go.mod h1:H/DxkYtsYVJwPFLikOL9yzb/PV7oIkz44CUmn4KecKg=
580+
github.com/cockroachdb/pebble v0.0.0-20251002220049-0ac45a74e10a h1:K31Go2TsBQEhEUNRSRbv63Qiu6ImEawHedYASgIyZOU=
581+
github.com/cockroachdb/pebble v0.0.0-20251002220049-0ac45a74e10a/go.mod h1:H/DxkYtsYVJwPFLikOL9yzb/PV7oIkz44CUmn4KecKg=
582582
github.com/cockroachdb/redact v1.1.6 h1:zXJBwDZ84xJNlHl1rMyCojqyIxv+7YUpQiJLQ7n4314=
583583
github.com/cockroachdb/redact v1.1.6/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg=
584584
github.com/cockroachdb/returncheck v0.0.0-20200612231554-92cdbca611dd h1:KFOt5I9nEKZgCnOSmy8r4Oykh8BYQO8bFOTgHDS8YZA=

‎pkg/sql/unsafesql/BUILD.bazel‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ go_library(
1414
"//pkg/util/log/eventpb",
1515
"//pkg/util/log/severity",
1616
"//pkg/util/timeutil",
17+
"@com_github_cockroachdb_redact//:redact",
1718
],
1819
)
1920

@@ -26,7 +27,9 @@ go_test(
2627
"//pkg/security/securityassets",
2728
"//pkg/security/securitytest",
2829
"//pkg/server",
30+
"//pkg/settings",
2931
"//pkg/sql/isql",
32+
"//pkg/sql/sem/tree",
3033
"//pkg/sql/sqlerrors",
3134
"//pkg/testutils/serverutils",
3235
"//pkg/testutils/sqlutils",

‎pkg/sql/unsafesql/unsafesql.go‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
1818
"github.com/cockroachdb/cockroach/pkg/util/log/severity"
1919
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
20+
"github.com/cockroachdb/redact"
2021
)
2122

2223
// The accessedLogLimiter is used to limit the rate of logging unsafe internal access
@@ -53,7 +54,7 @@ func CheckInternalsAccess(
5354
return nil
5455
}
5556

56-
q := tree.FormatAstAsRedactableString(stmt, ann, sv)
57+
q := SafeFormatQuery(stmt, ann, sv)
5758
// If an override is set, allow access to this virtual table.
5859
if sd.AllowUnsafeInternals {
5960
// Log this access to the SENSITIVE_ACCESS channel since the override condition bypassed normal access controls.
@@ -69,3 +70,20 @@ func CheckInternalsAccess(
6970
}
7071
return sqlerrors.ErrUnsafeTableAccess
7172
}
73+
74+
// SafeFormatQuery attempts to format the query for logging, but recovers from
75+
// any panics that may occur during formatting.
76+
func SafeFormatQuery(
77+
stmt tree.Statement, ann *tree.Annotations, sv *settings.Values,
78+
) (s redact.RedactableString) {
79+
if stmt == nil {
80+
return "<nil statement>"
81+
}
82+
defer func() {
83+
if r := recover(); r != nil {
84+
log.Dev.Errorf(context.TODO(), "panic in SafeFormatQuery: %v", r)
85+
s = "<panicked query format>"
86+
}
87+
}()
88+
return tree.FormatAstAsRedactableString(stmt, ann, sv)
89+
}

‎pkg/sql/unsafesql/unsafesql_test.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ import (
1616
"github.com/cockroachdb/cockroach/pkg/security/securityassets"
1717
"github.com/cockroachdb/cockroach/pkg/security/securitytest"
1818
"github.com/cockroachdb/cockroach/pkg/server"
19+
"github.com/cockroachdb/cockroach/pkg/settings"
1920
"github.com/cockroachdb/cockroach/pkg/sql/isql"
21+
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
2022
"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
2123
"github.com/cockroachdb/cockroach/pkg/sql/unsafesql"
2224
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -255,3 +257,35 @@ func TestAccessCheckServer(t *testing.T) {
255257
})
256258
}
257259
}
260+
261+
// panickingStatement is a mock Statement that panics during formatting
262+
type panickingStatement struct{}
263+
264+
func (ps panickingStatement) String() string {
265+
return "panicking statement"
266+
}
267+
268+
func (ps panickingStatement) Format(ctx *tree.FmtCtx) {
269+
panic("deliberate panic for testing")
270+
}
271+
272+
func (ps panickingStatement) StatementReturnType() tree.StatementReturnType {
273+
return tree.Unknown
274+
}
275+
276+
func (ps panickingStatement) StatementType() tree.StatementType {
277+
return tree.TypeDML
278+
}
279+
280+
func (ps panickingStatement) StatementTag() string {
281+
return "TEST"
282+
}
283+
284+
func TestPanickingSQLFormat(t *testing.T) {
285+
result := unsafesql.SafeFormatQuery(panickingStatement{}, nil, &settings.Values{})
286+
require.Equal(
287+
t,
288+
"<panicked query format>",
289+
string(result),
290+
)
291+
}

‎pkg/util/log/testdata/log_migration‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,14 @@ structured-event should_migrate=true new_channel=OPS depth=1
3131
DEV Channel:
3232
[]
3333
OPS Channel:
34-
[{Severity:INFO Time:0 Goroutine:0 File:datadriven/datadriven.go Line:119 Message:Structured entry: {"Timestamp":1735689600000000000,"EventType":"test_event"} Tags: Counter:14 Redactable:true Channel:OPS StructuredEnd:76 StructuredStart:18 StackTraceStart:0 TenantID: TenantName:}]
34+
[{Severity:INFO Time:0 Goroutine:0 File:datadriven/datadriven.go Line:120 Message:Structured entry: {"Timestamp":1735689600000000000,"EventType":"test_event"} Tags: Counter:14 Redactable:true Channel:OPS StructuredEnd:76 StructuredStart:18 StackTraceStart:0 TenantID: TenantName:}]
3535

3636
structured-event should_migrate=false new_channel=OPS depth=1
3737

3838
log should_migrate=true old_channel=TELEMETRY new_channel=SQL_EXEC
3939
----
4040
DEV Channel:
41-
[{Severity:INFO Time:0 Goroutine:0 File:datadriven/datadriven.go Line:119 Message:Structured entry: {"Timestamp":1735689600000000000,"EventType":"test_event"} Tags: Counter:17 Redactable:true Channel:DEV StructuredEnd:76 StructuredStart:18 StackTraceStart:0 TenantID: TenantName:}]
41+
[{Severity:INFO Time:0 Goroutine:0 File:datadriven/datadriven.go Line:120 Message:Structured entry: {"Timestamp":1735689600000000000,"EventType":"test_event"} Tags: Counter:17 Redactable:true Channel:DEV StructuredEnd:76 StructuredStart:18 StackTraceStart:0 TenantID: TenantName:}]
4242
OPS Channel:
4343
[]
4444

0 commit comments

Comments
 (0)