]> Cypherpunks.ru repositories - gostls13.git/commitdiff
runtime: replace panic(nil) with panic(new(runtime.PanicNilError))
authorRuss Cox <rsc@golang.org>
Thu, 12 Jan 2023 14:30:38 +0000 (09:30 -0500)
committerGopher Robot <gobot@golang.org>
Thu, 19 Jan 2023 22:21:50 +0000 (22:21 +0000)
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 <gri@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>

api/next/25448.txt [new file with mode: 0644]
doc/go_spec.html
src/builtin/builtin.go
src/net/http/clientserver_test.go
src/runtime/panic.go
src/runtime/panicnil_test.go [new file with mode: 0644]
src/runtime/runtime.go
src/runtime/runtime1.go
test/fixedbugs/issue19658.go

diff --git a/api/next/25448.txt b/api/next/25448.txt
new file mode 100644 (file)
index 0000000..1b89017
--- /dev/null
@@ -0,0 +1,3 @@
+pkg runtime, method (*PanicNilError) Error() string #25448
+pkg runtime, method (*PanicNilError) RuntimeError() #25448
+pkg runtime, type PanicNilError struct #25448
index f93f2ab9f17f629e0245e0b38157a954f7d19eb3..9f0cbb09dc11e927bceaaefa63fa787bfd51fb35 100644 (file)
@@ -1,6 +1,6 @@
 <!--{
        "Title": "The Go Programming Language Specification",
-       "Subtitle": "Version of December 15, 2022",
+       "Subtitle": "Version of January 19, 2022",
        "Path": "/ref/spec"
 }-->
 
@@ -7574,19 +7574,13 @@ execution terminates by returning to its caller.
 </p>
 
 <p>
-The return value of <code>recover</code> is <code>nil</code> if any of the following conditions holds:
+The return value of <code>recover</code> is <code>nil</code> when the
+goroutine is not panicking or <code>recover</code> was not called directly by a deferred function.
+Conversely, if a goroutine is panicking and <code>recover</code> was called directly by a deferred function,
+the return value of <code>recover</code> is guaranteed not to be <code>nil</code>.
+To ensure this, calling <code>panic</code> with a <code>nil</code> interface value (or an untyped <code>nil</code>)
+causes a <a href="#Run_time_panics">run-time panic</a>.
 </p>
-<ul>
-<li>
-<code>panic</code>'s argument was <code>nil</code>;
-</li>
-<li>
-the goroutine is not panicking;
-</li>
-<li>
-<code>recover</code> was not called directly by a deferred function.
-</li>
-</ul>
 
 <p>
 The <code>protect</code> function in the example below invokes
index 7feb209bb4992b20178d946d85a6a13c8a7c582c..d3637584fe9907a067b7b5ab8359d4f4ba0aab74 100644 (file)
@@ -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
index da5671d9b9f7ea55c4dd892a25ab0ea3f5e489b8..e49bed113ae55171ef517dec79ffcca65cddc521 100644 (file)
@@ -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"
index 26618db7ceb470473bacdfa2d8969f63b258779f..905515f8224dd91b7b92374f7470d72f83d1ef90 100644 (file)
@@ -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 (file)
index 0000000..441bef3
--- /dev/null
@@ -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)
+}
index 9f68738aa7ba07c5992c4e6124ee0777a9f876ad..ab2a54f00b0a19854d0a7142c80e0517d9317f79 100644 (file)
@@ -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)
        }
 }
 
index 277f18a5a6733a0dfe1fbd6c5be0626c976e1d27..5f9555e40490e60fb46512c28e468d8ce846975a 100644 (file)
@@ -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
index bab409c6c03cc86d64ae51ef3bb51a09d77f2d6a..70fa3a65c39ce6a79a8cc88cde293e5cc73222d4 100644 (file)
@@ -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"},