Merge #133620

133620: colexecerror: improve the catcher due to a recent regression r=yuzefovich a=yuzefovich

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.

Fixes: #133617.

Release note: None

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

pkg/sql/colexecerror/error.go

@@ -17,7 +17,11 @@ import (
 	"github.com/gogo/protobuf/proto"
 )
 
-const panicLineSubstring = "runtime/panic.go"
+const (
+	panicLineSubstring            = "runtime/panic.go"
+	runtimePanicFileSubstring     = "runtime"
+	runtimePanicFunctionSubstring = "runtime."
+)
 
 var testingKnobShouldCatchPanic bool
 
@@ -95,9 +99,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])
@@ -107,6 +115,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

@@ -87,6 +87,40 @@ 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)
+
+	// Use the release-build panic-catching behavior instead of the
+	// crdb_test-build behavior.
+	defer colexecerror.ProductionBehaviorForTests()()
+
+	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) {