Merge pull request #133636 from cockroachdb/blathers/backport-release…

…-24.1-133620

release-24.1: colexecerror: improve the catcher due to a recent regression

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
 				}

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) {