text/template: recover panics during function calls

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í <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>

diff --git a/src/text/template/exec.go b/src/text/template/exec.go
index 36cea3d24db9f2136a3731771c2e8faeb76fca8b..c6ce657cf64effcb27766c1afc1eea2646ccb63f 100644 (file)
--- 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 648ad8ff03d269f7145e419c92266c8e26a40729..bfd6d38bf42cfee7c91a2637994fc586dba787f8 100644 (file)
--- 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 <doPanic>: error calling doPanic: custom panic string`,
+               },
+               {
+                       "indirect func call panics",
+                       "{{call doPanic}}", (*T)(nil),
+                       `template: t:1:7: executing "t" at <doPanic>: 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 <call .PanicFunc>: 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 31fe77a327dafad0036047c2970253ba462c6616..72d3f666918d6569aa8b3cb9d207d1d42e22169d 100644 (file)
--- 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.