From d6fa3ce559ad49400800f1823dd12601714579f3 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Thu, 12 Jan 2023 09:30:38 -0500 Subject: [PATCH] runtime: replace panic(nil) with panic(new(runtime.PanicNilError)) Long ago we decided that panic(nil) was too unlikely to bother making a special case for purposes of recover. Unfortunately, it has turned out not to be a special case. There are many examples of code in the Go ecosystem where an author has written panic(nil) because they want to panic and don't care about the panic value. Using panic(nil) in this case has the unfortunate behavior of making recover behave as though the goroutine isn't panicking. As a result, code like: func f() { defer func() { if err := recover(); err != nil { log.Fatalf("panicked! %v", err) } }() call1() call2() } looks like it guarantees that call2 has been run any time f returns, but that turns out not to be strictly true. If call1 does panic(nil), then f returns "successfully", having recovered the panic, but without calling call2. Instead you have to write something like: func f() { done := false defer func() { if err := recover(); !done { log.Fatalf("panicked! %v", err) } }() call1() call2() done = true } which defeats nearly the whole point of recover. No one does this, with the result that almost all uses of recover are subtly broken. One specific broken use along these lines is in net/http, which recovers from panics in handlers and sends back an HTTP error. Users discovered in the early days of Go that panic(nil) was a convenient way to jump out of a handler up to the serving loop without sending back an HTTP error. This was a bug, not a feature. Go 1.8 added panic(http.ErrAbortHandler) as a better way to access the feature. Any lingering code that uses panic(nil) to abort an HTTP handler without a failure message should be changed to use http.ErrAbortHandler. Programs that need the old, unintended behavior from net/http or other packages can set GODEBUG=panicnil=1 to stop the run-time error. Uses of recover that want to detect panic(nil) in new programs can check for recover returning a value of type *runtime.PanicNilError. Because the new GODEBUG is used inside the runtime, we can't import internal/godebug, so there is some new machinery to cross-connect those in this CL, to allow a mutable GODEBUG setting. That won't be necessary if we add any other mutable GODEBUG settings in the future. The CL also corrects the handling of defaulted GODEBUG values in the runtime, for #56986. Fixes #25448. Change-Id: I2b39c7e83e4f7aa308777dabf2edae54773e03f5 Reviewed-on: https://go-review.googlesource.com/c/go/+/461956 Reviewed-by: Robert Griesemer Run-TryBot: Russ Cox TryBot-Result: Gopher Robot Auto-Submit: Russ Cox --- api/next/25448.txt | 3 + doc/go_spec.html | 20 ++-- src/builtin/builtin.go | 4 + src/net/http/clientserver_test.go | 4 +- src/runtime/panic.go | 21 ++++ src/runtime/panicnil_test.go | 36 +++++++ src/runtime/runtime.go | 22 +++-- src/runtime/runtime1.go | 159 ++++++++++++++++++++++-------- test/fixedbugs/issue19658.go | 5 +- 9 files changed, 207 insertions(+), 67 deletions(-) create mode 100644 api/next/25448.txt create mode 100644 src/runtime/panicnil_test.go diff --git a/api/next/25448.txt b/api/next/25448.txt new file mode 100644 index 00000000000000..1b8901710cd8c0 --- /dev/null +++ b/api/next/25448.txt @@ -0,0 +1,3 @@ +pkg runtime, method (*PanicNilError) Error() string #25448 +pkg runtime, method (*PanicNilError) RuntimeError() #25448 +pkg runtime, type PanicNilError struct #25448 diff --git a/doc/go_spec.html b/doc/go_spec.html index f93f2ab9f17f62..9f0cbb09dc11e9 100644 --- a/doc/go_spec.html +++ b/doc/go_spec.html @@ -1,6 +1,6 @@ @@ -7574,19 +7574,13 @@

Handling panics

-The return value of recover is nil if any of the following conditions holds: +The return value of recover is nil when the +goroutine is not panicking or recover was not called directly by a deferred function. +Conversely, if a goroutine is panicking and recover was called directly by a deferred function, +the return value of recover is guaranteed not to be nil. +To ensure this, calling panic with a nil interface value (or an untyped nil) +causes a run-time panic.

-
    -
  • -panic's argument was nil; -
    • -
  • -the goroutine is not panicking; -
    • -
  • -recover was not called directly by a deferred function. -
    • -

The protect function in the example below invokes diff --git a/src/builtin/builtin.go b/src/builtin/builtin.go index 7feb209bb4992b..d3637584fe9907 100644 --- a/src/builtin/builtin.go +++ b/src/builtin/builtin.go @@ -249,6 +249,10 @@ func close(c chan<- Type) // that point, the program is terminated with a non-zero exit code. This // termination sequence is called panicking and can be controlled by the // built-in function recover. +// +// Starting in Go 1.21, calling panic with a nil interface value or an +// untyped nil causes a run-time error (a different panic). +// The GODEBUG setting panicnil=1 disables the run-time error. func panic(v any) // The recover built-in function allows a program to manage behavior of a diff --git a/src/net/http/clientserver_test.go b/src/net/http/clientserver_test.go index da5671d9b9f7ea..e49bed113ae551 100644 --- a/src/net/http/clientserver_test.go +++ b/src/net/http/clientserver_test.go @@ -1239,9 +1239,9 @@ func testTransportRejectsInvalidHeaders(t *testing.T, mode testMode) { func TestInterruptWithPanic(t *testing.T) { run(t, func(t *testing.T, mode testMode) { t.Run("boom", func(t *testing.T) { testInterruptWithPanic(t, mode, "boom") }) - t.Run("nil", func(t *testing.T) { testInterruptWithPanic(t, mode, nil) }) + t.Run("nil", func(t *testing.T) { t.Setenv("GODEBUG", "panicnil=1"); testInterruptWithPanic(t, mode, nil) }) t.Run("ErrAbortHandler", func(t *testing.T) { testInterruptWithPanic(t, mode, ErrAbortHandler) }) - }) + }, testNotParallel) } func testInterruptWithPanic(t *testing.T, mode testMode, panicValue any) { const msg = "hello" diff --git a/src/runtime/panic.go b/src/runtime/panic.go index 6a6437de249964..df5c20d5b7e33b 100644 --- a/src/runtime/panic.go +++ b/src/runtime/panic.go @@ -800,8 +800,29 @@ func deferCallSave(p *_panic, fn func()) { } } +// A PanicNilError happens when code calls panic(nil). +// +// Before Go 1.21, programs that called panic(nil) observed recover returning nil. +// Starting in Go 1.21, programs that call panic(nil) observe recover returning a *PanicNilError. +// Programs can change back to the old behavior by setting GODEBUG=panicnil=1. +type PanicNilError struct { + // This field makes PanicNilError structurally different from + // any other struct in this package, and the _ makes it different + // from any struct in other packages too. + // This avoids any accidental conversions being possible + // between this struct and some other struct sharing the same fields, + // like happened in go.dev/issue/56603. + _ [0]*PanicNilError +} + +func (*PanicNilError) Error() string { return "panic called with nil argument" } +func (*PanicNilError) RuntimeError() {} + // The implementation of the predeclared function panic. func gopanic(e any) { + if e == nil && debug.panicnil.Load() != 1 { + e = new(PanicNilError) + } gp := getg() if gp.m.curg != gp { print("panic: ") diff --git a/src/runtime/panicnil_test.go b/src/runtime/panicnil_test.go new file mode 100644 index 00000000000000..441bef3b071e78 --- /dev/null +++ b/src/runtime/panicnil_test.go @@ -0,0 +1,36 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package runtime_test + +import ( + "reflect" + "runtime" + "testing" +) + +func TestPanicNil(t *testing.T) { + t.Run("default", func(t *testing.T) { + checkPanicNil(t, new(runtime.PanicNilError)) + }) + t.Run("GODEBUG=panicnil=0", func(t *testing.T) { + t.Setenv("GODEBUG", "panicnil=0") + checkPanicNil(t, new(runtime.PanicNilError)) + }) + t.Run("GODEBUG=panicnil=1", func(t *testing.T) { + t.Setenv("GODEBUG", "panicnil=1") + checkPanicNil(t, nil) + }) +} + +func checkPanicNil(t *testing.T, want any) { + defer func() { + e := recover() + if reflect.TypeOf(e) != reflect.TypeOf(want) { + println(e, want) + t.Errorf("recover() = %v, want %v", e, want) + } + }() + panic(nil) +} diff --git a/src/runtime/runtime.go b/src/runtime/runtime.go index 9f68738aa7ba07..ab2a54f00b0a19 100644 --- a/src/runtime/runtime.go +++ b/src/runtime/runtime.go @@ -75,15 +75,19 @@ func godebug_setUpdate(update func(string, string)) { p := new(func(string, string)) *p = update godebugUpdate.Store(p) - godebugNotify() + godebugNotify(false) } -func godebugNotify() { - if update := godebugUpdate.Load(); update != nil { - var env string - if p := godebugEnv.Load(); p != nil { - env = *p - } +func godebugNotify(envChanged bool) { + update := godebugUpdate.Load() + var env string + if p := godebugEnv.Load(); p != nil { + env = *p + } + if envChanged { + reparsedebugvars(env) + } + if update != nil { (*update)(godebugDefault, env) } } @@ -95,7 +99,7 @@ func syscall_runtimeSetenv(key, value string) { p := new(string) *p = value godebugEnv.Store(p) - godebugNotify() + godebugNotify(true) } } @@ -104,7 +108,7 @@ func syscall_runtimeUnsetenv(key string) { unsetenv_c(key) if key == "GODEBUG" { godebugEnv.Store(nil) - godebugNotify() + godebugNotify(true) } } diff --git a/src/runtime/runtime1.go b/src/runtime/runtime1.go index 277f18a5a6733a..5f9555e40490e6 100644 --- a/src/runtime/runtime1.go +++ b/src/runtime/runtime1.go @@ -296,8 +296,10 @@ func check() { } type dbgVar struct { - name string - value *int32 + name string + value *int32 // for variables that can only be set at startup + atomic *atomic.Int32 // for variables that can be changed during execution + def int32 // default value (ideally zero) } // Holds variables parsed from GODEBUG env var, @@ -330,32 +332,33 @@ var debug struct { allocfreetrace int32 inittrace int32 sbrk int32 -} -var dbgvars = []dbgVar{ - {"allocfreetrace", &debug.allocfreetrace}, - {"clobberfree", &debug.clobberfree}, - {"cgocheck", &debug.cgocheck}, - {"efence", &debug.efence}, - {"gccheckmark", &debug.gccheckmark}, - {"gcpacertrace", &debug.gcpacertrace}, - {"gcshrinkstackoff", &debug.gcshrinkstackoff}, - {"gcstoptheworld", &debug.gcstoptheworld}, - {"gctrace", &debug.gctrace}, - {"invalidptr", &debug.invalidptr}, - {"madvdontneed", &debug.madvdontneed}, - {"sbrk", &debug.sbrk}, - {"scavtrace", &debug.scavtrace}, - {"scheddetail", &debug.scheddetail}, - {"schedtrace", &debug.schedtrace}, - {"tracebackancestors", &debug.tracebackancestors}, - {"asyncpreemptoff", &debug.asyncpreemptoff}, - {"inittrace", &debug.inittrace}, - {"harddecommit", &debug.harddecommit}, - {"adaptivestackstart", &debug.adaptivestackstart}, + panicnil atomic.Int32 } -var globalGODEBUG string +var dbgvars = []*dbgVar{ + {name: "allocfreetrace", value: &debug.allocfreetrace}, + {name: "clobberfree", value: &debug.clobberfree}, + {name: "cgocheck", value: &debug.cgocheck}, + {name: "efence", value: &debug.efence}, + {name: "gccheckmark", value: &debug.gccheckmark}, + {name: "gcpacertrace", value: &debug.gcpacertrace}, + {name: "gcshrinkstackoff", value: &debug.gcshrinkstackoff}, + {name: "gcstoptheworld", value: &debug.gcstoptheworld}, + {name: "gctrace", value: &debug.gctrace}, + {name: "invalidptr", value: &debug.invalidptr}, + {name: "madvdontneed", value: &debug.madvdontneed}, + {name: "sbrk", value: &debug.sbrk}, + {name: "scavtrace", value: &debug.scavtrace}, + {name: "scheddetail", value: &debug.scheddetail}, + {name: "schedtrace", value: &debug.schedtrace}, + {name: "tracebackancestors", value: &debug.tracebackancestors}, + {name: "asyncpreemptoff", value: &debug.asyncpreemptoff}, + {name: "inittrace", value: &debug.inittrace}, + {name: "harddecommit", value: &debug.harddecommit}, + {name: "adaptivestackstart", value: &debug.adaptivestackstart}, + {name: "panicnil", atomic: &debug.panicnil}, +} func parsedebugvars() { // defaults @@ -374,26 +377,101 @@ func parsedebugvars() { debug.madvdontneed = 1 } - globalGODEBUG = gogetenv("GODEBUG") - godebugEnv.StoreNoWB(&globalGODEBUG) - for p := globalGODEBUG; p != ""; { - field := "" - i := bytealg.IndexByteString(p, ',') - if i < 0 { - field, p = p, "" + godebug := gogetenv("GODEBUG") + + p := new(string) + *p = godebug + godebugEnv.Store(p) + + // apply runtime defaults, if any + for _, v := range dbgvars { + if v.def != 0 { + // Every var should have either v.value or v.atomic set. + if v.value != nil { + *v.value = v.def + } else if v.atomic != nil { + v.atomic.Store(v.def) + } + } + } + + // apply compile-time GODEBUG settings + parsegodebug(godebugDefault, nil) + + // apply environment settings + parsegodebug(godebug, nil) + + debug.malloc = (debug.allocfreetrace | debug.inittrace | debug.sbrk) != 0 + + setTraceback(gogetenv("GOTRACEBACK")) + traceback_env = traceback_cache +} + +// reparsedebugvars reparses the runtime's debug variables +// because the environment variable has been changed to env. +func reparsedebugvars(env string) { + seen := make(map[string]bool) + // apply environment settings + parsegodebug(env, seen) + // apply compile-time GODEBUG settings for as-yet-unseen variables + parsegodebug(godebugDefault, seen) + // apply defaults for as-yet-unseen variables + for _, v := range dbgvars { + if v.atomic != nil && !seen[v.name] { + v.atomic.Store(0) + } + } +} + +// parsegodebug parses the godebug string, updating variables listed in dbgvars. +// If seen == nil, this is startup time and we process the string left to right +// overwriting older settings with newer ones. +// If seen != nil, $GODEBUG has changed and we are doing an +// incremental update. To avoid flapping in the case where a value is +// set multiple times (perhaps in the default and the environment, +// or perhaps twice in the environment), we process the string right-to-left +// and only change values not already seen. After doing this for both +// the environment and the default settings, the caller must also call +// cleargodebug(seen) to reset any now-unset values back to their defaults. +func parsegodebug(godebug string, seen map[string]bool) { + for p := godebug; p != ""; { + var field string + if seen == nil { + // startup: process left to right, overwriting older settings with newer + i := bytealg.IndexByteString(p, ',') + if i < 0 { + field, p = p, "" + } else { + field, p = p[:i], p[i+1:] + } } else { - field, p = p[:i], p[i+1:] + // incremental update: process right to left, updating and skipping seen + i := len(p) - 1 + for i >= 0 && p[i] != ',' { + i-- + } + if i < 0 { + p, field = "", p + } else { + p, field = p[:i], p[i+1:] + } } - i = bytealg.IndexByteString(field, '=') + i := bytealg.IndexByteString(field, '=') if i < 0 { continue } key, value := field[:i], field[i+1:] + if seen[key] { + continue + } + if seen != nil { + seen[key] = true + } // Update MemProfileRate directly here since it // is int, not int32, and should only be updated // if specified in GODEBUG. - if key == "memprofilerate" { + if seen == nil && key == "memprofilerate" { if n, ok := atoi(value); ok { MemProfileRate = n } @@ -401,17 +479,16 @@ func parsedebugvars() { for _, v := range dbgvars { if v.name == key { if n, ok := atoi32(value); ok { - *v.value = n + if seen == nil && v.value != nil { + *v.value = n + } else if v.atomic != nil { + v.atomic.Store(n) + } } } } } } - - debug.malloc = (debug.allocfreetrace | debug.inittrace | debug.sbrk) != 0 - - setTraceback(gogetenv("GOTRACEBACK")) - traceback_env = traceback_cache } //go:linkname setTraceback runtime/debug.SetTraceback diff --git a/test/fixedbugs/issue19658.go b/test/fixedbugs/issue19658.go index bab409c6c03cc8..70fa3a65c39ce6 100644 --- a/test/fixedbugs/issue19658.go +++ b/test/fixedbugs/issue19658.go @@ -1,5 +1,5 @@ -// +build !nacl,!js,!gccgo // run +//go:build !nacl && !js && !gccgo // Copyright 2017 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style @@ -46,7 +46,8 @@ func main() { Type string Input string Expect string - }{{"", "nil", "panic: nil"}, + }{ + {"", "nil", "panic: panic called with nil argument"}, {"errors.New", `"test"`, "panic: test"}, {"S", "S{}", "panic: s-stringer"}, {"byte", "8", "panic: 8"},