From aa51c40b1cc62d53603d7b7aea3232969aa40afe 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 | 161 ++++++++++++++++++++++-------- test/fixedbugs/issue19658.go | 5 +- 9 files changed, 208 insertions(+), 68 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 0000000000..1b8901710c --- /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 f93f2ab9f1..9f0cbb09dc 100644 --- a/doc/go_spec.html +++ b/doc/go_spec.html @@ -1,6 +1,6 @@ @@ -7574,19 +7574,13 @@ execution terminates by returning to its caller.

-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 7feb209bb4..d3637584fe 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 da5671d9b9..e49bed113a 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 26618db7ce..905515f822 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 0000000000..441bef3b07 --- /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 9f68738aa7..ab2a54f00b 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 277f18a5a6..5f9555e404 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,33 +332,34 @@ var debug struct { allocfreetrace int32 inittrace int32 sbrk int32 + + panicnil atomic.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}, +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}, } -var globalGODEBUG string - func parsedebugvars() { // defaults debug.cgocheck = 1 @@ -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, "" - } else { - field, p = p[:i], p[i+1:] + 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) + } } - i = bytealg.IndexByteString(field, '=') + } + + // 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 { + // 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, '=') 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 bab409c6c0..70fa3a65c3 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"},