From 5e05c2d968b70513842ad1e04223ba523564096c Mon Sep 17 00:00:00 2001
From: Yahor Yuzefovich <yahor@cockroachlabs.com>
Date: Mon, 28 Oct 2024 22:44:25 +0000
Subject: [PATCH] colexecerror: improve the catcher due to a recent regression

Earlier this year we made the vectorized panic-catcher much more
efficient (in #123277) by switching from using `debug.Stack()` to
`runtime.CallersFrames`. It appears that there is slight difference in
the behavior between the two: the former omits frames from within the
runtime (only a single frame for the panic itself is included) whereas
the latter keeps the additional runtime frames. As a result, if a panic
occurs due to a Go runtime internal violation (e.g. invalid interface
assertion) it is no longer caught to be converted into an internal CRDB
error and now crashes the server. This commit fixes this regression by
skipping over the frames that belong to the Go runtime. Note that we
will do so only for up to 5 frames within the runtime, so if there
happens to be more deeply-nested panic there, we'll still crash the CRDB
server.

Release note: None
---
 pkg/sql/colexecerror/error.go      | 23 +++++++++++++++++++----
 pkg/sql/colexecerror/error_test.go | 30 ++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/pkg/sql/colexecerror/error.go b/pkg/sql/colexecerror/error.go
index 9d137ea0e929..4edc08b6c0ff 100644
--- a/pkg/sql/colexecerror/error.go
+++ b/pkg/sql/colexecerror/error.go
@@ -16,7 +16,11 @@ import (
 	"github.com/gogo/protobuf/proto"
 )
 
-const panicLineSubstring = "runtime/panic.go"
+const (
+	panicLineSubstring            = "runtime/panic.go"
+	runtimePanicFileSubstring     = "runtime"
+	runtimePanicFunctionSubstring = "runtime."
+)
 
 // CatchVectorizedRuntimeError executes operation, catches a runtime error if
 // it is coming from the vectorized engine, and returns it. If an error not
@@ -78,9 +82,13 @@ func CatchVectorizedRuntimeError(operation func()) (retErr error) {
 		// panic frame.
 		var panicLineFound bool
 		var panicEmittedFrom string
-		// We should be able to find it within 3 program counters, starting with the
-		// caller of this deferred function (2 above the runtime.Callers frame).
-		pc := make([]uintptr, 3)
+		// Usually, we'll find the offending frame within 3 program counters,
+		// starting with the caller of this deferred function (2 above the
+		// runtime.Callers frame). However, we also want to catch panics
+		// originating in the Go runtime with the runtime frames being returned
+		// by CallersFrames, so we allow for 5 more program counters for that
+		// case (e.g. invalid interface conversions use 2 counters).
+		pc := make([]uintptr, 8)
 		n := runtime.Callers(2, pc)
 		if n >= 1 {
 			frames := runtime.CallersFrames(pc[:n])
@@ -90,6 +98,13 @@ func CatchVectorizedRuntimeError(operation func()) (retErr error) {
 				if strings.Contains(frame.File, panicLineSubstring) {
 					panicLineFound = true
 				} else if panicLineFound {
+					if strings.HasPrefix(frame.Function, runtimePanicFunctionSubstring) &&
+						strings.Contains(frame.File, runtimePanicFileSubstring) {
+						// This frame is from the Go runtime, so we simply
+						// ignore it to get to the offending one within the
+						// CRDB.
+						continue
+					}
 					panicEmittedFrom = frame.Function
 					break
 				}
diff --git a/pkg/sql/colexecerror/error_test.go b/pkg/sql/colexecerror/error_test.go
index 6654f794c9d4..6e0112ca5f96 100644
--- a/pkg/sql/colexecerror/error_test.go
+++ b/pkg/sql/colexecerror/error_test.go
@@ -79,6 +79,36 @@ func TestNonCatchablePanicIsNotCaught(t *testing.T) {
 	})
 }
 
+type testInterface interface {
+	foo()
+}
+
+type testImpl1 struct{}
+
+var _ testInterface = &testImpl1{}
+
+func (t testImpl1) foo() {}
+
+type testImpl2 struct{}
+
+var _ testInterface = &testImpl2{}
+
+func (t testImpl2) foo() {}
+
+// TestRuntimePanicIsCaught verifies that if a runtime panic occurs in the
+// safe-to-catch package (which this test package is), then it is converted into
+// an internal error (#133167).
+func TestRuntimePanicIsCaught(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	require.Error(t, colexecerror.CatchVectorizedRuntimeError(func() {
+		// Attempt an invalid interface conversion.
+		var o testInterface = &testImpl1{}
+		_ = o.(*testImpl2)
+	}))
+}
+
 // BenchmarkCatchVectorizedRuntimeError measures the time for
 // CatchVectorizedRuntimeError to catch and process an error.
 func BenchmarkCatchVectorizedRuntimeError(b *testing.B) {