From 202e9031444cef93c16a97cf076e5f8a9d9c3a75 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Daniel=20Mart=C3=AD?= Date: Thu, 18 Oct 2018 10:53:44 +0100 Subject: [PATCH] text/template: recover panics during function calls MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit There's precedent in handling panics that happen in functions called from the standard library. For example, if a fmt.Formatter implementation fails, fmt will absorb the panic into the output text. Recovering panics is useful, because otherwise one would have to wrap some Template.Execute calls with a recover. For example, if there's a chance that the callbacks may panic, or if part of the input data is nil when it shouldn't be. In particular, it's a common confusion amongst new Go developers that one can call a method on a nil receiver. Expecting text/template to error on such a call, they encounter a long and confusing panic if the method expects the receiver to be non-nil. To achieve this, introduce safeCall, which takes care of handling error returns as well as recovering panics. Handling panics in the "call" function isn't strictly necessary, as that func itself is run via evalCall. However, this makes the code more consistent, and can allow for better context in panics via the "call" function. Finally, add some test cases with a mix of funcs, methods, and func fields that panic. Fixes #28242. Change-Id: Id67be22cc9ebaedeb4b17fa84e677b4b6e09ec67 Reviewed-on: https://go-review.googlesource.com/c/143097 Run-TryBot: Daniel Martí TryBot-Result: Gobot Gobot Reviewed-by: Rob Pike --- src/text/template/exec.go | 10 +++--- src/text/template/exec_test.go | 59 ++++++++++++++++++++++++++++++++++ src/text/template/funcs.go | 23 ++++++++++--- 3 files changed, 83 insertions(+), 9 deletions(-) diff --git a/src/text/template/exec.go b/src/text/template/exec.go index 36cea3d24d..c6ce657cf6 100644 --- a/src/text/template/exec.go +++ b/src/text/template/exec.go @@ -693,13 +693,13 @@ func (s *state) evalCall(dot, fun reflect.Value, node parse.Node, name string, a } argv[i] = s.validateType(final, t) } - result := fun.Call(argv) - // If we have an error that is not nil, stop execution and return that error to the caller. - if len(result) == 2 && !result[1].IsNil() { + v, err := safeCall(fun, argv) + // If we have an error that is not nil, stop execution and return that + // error to the caller. + if err != nil { s.at(node) - s.errorf("error calling %s: %s", name, result[1].Interface().(error)) + s.errorf("error calling %s: %v", name, err) } - v := result[0] if v.Type() == reflectValueType { v = v.Interface().(reflect.Value) } diff --git a/src/text/template/exec_test.go b/src/text/template/exec_test.go index 648ad8ff03..bfd6d38bf4 100644 --- a/src/text/template/exec_test.go +++ b/src/text/template/exec_test.go @@ -74,6 +74,7 @@ type T struct { VariadicFuncInt func(int, ...string) string NilOKFunc func(*int) bool ErrFunc func() (string, error) + PanicFunc func() string // Template to test evaluation of templates. Tmpl *Template // Unexported field; cannot be accessed by template. @@ -156,6 +157,7 @@ var tVal = &T{ VariadicFuncInt: func(a int, s ...string) string { return fmt.Sprint(a, "=<", strings.Join(s, "+"), ">") }, NilOKFunc: func(s *int) bool { return s == nil }, ErrFunc: func() (string, error) { return "bla", nil }, + PanicFunc: func() string { panic("test panic") }, Tmpl: Must(New("x").Parse("test template")), // "x" is the value of .X } @@ -1451,3 +1453,60 @@ func TestInterfaceValues(t *testing.T) { } } } + +// Check that panics during calls are recovered and returned as errors. +func TestExecutePanicDuringCall(t *testing.T) { + funcs := map[string]interface{}{ + "doPanic": func() string { + panic("custom panic string") + }, + } + tests := []struct { + name string + input string + data interface{} + wantErr string + }{ + { + "direct func call panics", + "{{doPanic}}", (*T)(nil), + `template: t:1:2: executing "t" at : error calling doPanic: custom panic string`, + }, + { + "indirect func call panics", + "{{call doPanic}}", (*T)(nil), + `template: t:1:7: executing "t" at : error calling doPanic: custom panic string`, + }, + { + "direct method call panics", + "{{.GetU}}", (*T)(nil), + `template: t:1:2: executing "t" at <.GetU>: error calling GetU: runtime error: invalid memory address or nil pointer dereference`, + }, + { + "indirect method call panics", + "{{call .GetU}}", (*T)(nil), + `template: t:1:7: executing "t" at <.GetU>: error calling GetU: runtime error: invalid memory address or nil pointer dereference`, + }, + { + "func field call panics", + "{{call .PanicFunc}}", tVal, + `template: t:1:2: executing "t" at : error calling call: test panic`, + }, + } + for _, tc := range tests { + b := new(bytes.Buffer) + tmpl, err := New("t").Funcs(funcs).Parse(tc.input) + if err != nil { + t.Fatalf("parse error: %s", err) + } + err = tmpl.Execute(b, tc.data) + if err == nil { + t.Errorf("%s: expected error; got none", tc.name) + } else if !strings.Contains(err.Error(), tc.wantErr) { + if *debug { + fmt.Printf("%s: test execute error: %s\n", tc.name, err) + } + t.Errorf("%s: expected error:\n%s\ngot:\n%s", tc.name, tc.wantErr, err) + } + } +} diff --git a/src/text/template/funcs.go b/src/text/template/funcs.go index 31fe77a327..72d3f66691 100644 --- a/src/text/template/funcs.go +++ b/src/text/template/funcs.go @@ -275,11 +275,26 @@ func call(fn reflect.Value, args ...reflect.Value) (reflect.Value, error) { return reflect.Value{}, fmt.Errorf("arg %d: %s", i, err) } } - result := v.Call(argv) - if len(result) == 2 && !result[1].IsNil() { - return result[0], result[1].Interface().(error) + return safeCall(v, argv) +} + +// safeCall runs fun.Call(args), and returns the resulting value and error, if +// any. If the call panics, the panic value is returned as an error. +func safeCall(fun reflect.Value, args []reflect.Value) (val reflect.Value, err error) { + defer func() { + if r := recover(); r != nil { + if e, ok := r.(error); ok { + err = e + } else { + err = fmt.Errorf("%v", r) + } + } + }() + ret := fun.Call(args) + if len(ret) == 2 && !ret[1].IsNil() { + return ret[0], ret[1].Interface().(error) } - return result[0], nil + return ret[0], nil } // Boolean logic. -- 2.48.1