text/template: panics encountered during function calls are hard to treat and debug #28242

bep · 2018-10-16T21:44:52Z

 go version && go env
go version go1.11 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/bep/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/bep/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/bep/dev/go/gohugoio/hugo/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/n6/s_85mm8d31j6yctssnmn_g1r0000gn/T/go-build656061374=/tmp/go-build -gno-record-gcc-switches -fno-common"

package main

import (
	"bytes"
	"log"
	"strings"
	"text/template"
)

type F struct {
	name string
}

func (f *F) Name() string {
	return f.name
}

func (f *F) Other() *F {
	return nil
}

func main() {
	var buf bytes.Buffer
	tmpl, err := template.New("").Parse(`{{ .Other.Name }}`)
	if err != nil {
		log.Fatal(err)
	}

	data := &F{name: "foo"}

	if err := tmpl.Execute(&buf, data); err != nil {
		log.Fatal(err)
	}

	result := strings.TrimSpace(buf.String())

	if !strings.Contains(result, "foo") {
		log.Fatal(result)
	}

}

The above panics:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x10ec8a5]

goroutine 1 [running]:
text/template.errRecover(0xc0000a9ec0)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:160 +0x1d0
panic(0x110e180, 0x121a910)
	/usr/local/Cellar/go/1.11/libexec/src/runtime/panic.go:513 +0x1b9
main.(*F).Name(0x0, 0x0, 0x0)
	/Users/bep/dev/go/bep/temp/main.go:15 +0x5
reflect.Value.call(0x110e5c0, 0xc000068338, 0x293, 0x112ebd0, 0x4, 0x123da30, 0x0, 0x0, 0x112d940, 0x1, ...)
	/usr/local/Cellar/go/1.11/libexec/src/reflect/value.go:447 +0x449
reflect.Value.Call(0x110e5c0, 0xc000068338, 0x293, 0x123da30, 0x0, 0x0, 0xc000072100, 0x1, 0x1)
	/usr/local/Cellar/go/1.11/libexec/src/reflect/value.go:308 +0xa4
text/template.(*state).evalCall(0xc0000a9e40, 0x110e5c0, 0xc000068290, 0x16, 0x110e5c0, 0xc000068338, 0x293, 0x114e920, 0xc00006a360, 0xc000082027, ...)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:695 +0x6bd
text/template.(*state).evalField(0xc0000a9e40, 0x110e5c0, 0xc000068290, 0x16, 0xc000082027, 0x4, 0x114e920, 0xc00006a360, 0xc000068270, 0x1, ...)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:585 +0xd78
text/template.(*state).evalFieldChain(0xc0000a9e40, 0x110e5c0, 0xc000068290, 0x16, 0x110e5c0, 0xc000068290, 0x16, 0x114e920, 0xc00006a360, 0xc000072060, ...)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:553 +0x217
text/template.(*state).evalFieldNode(0xc0000a9e40, 0x110e5c0, 0xc000068290, 0x16, 0xc00006a360, 0xc000068270, 0x1, 0x1, 0x110e420, 0x123da30, ...)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:517 +0x113
text/template.(*state).evalCommand(0xc0000a9e40, 0x110e5c0, 0xc000068290, 0x16, 0xc00006a300, 0x110e420, 0x123da30, 0x99, 0x100b738, 0xd0, ...)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:455 +0x725
text/template.(*state).evalPipeline(0xc0000a9e40, 0x110e5c0, 0xc000068290, 0x16, 0xc000092180, 0xc0000a9c01, 0xc000074750, 0xc0000a9d40)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:429 +0x126
text/template.(*state).walk(0xc0000a9e40, 0x110e5c0, 0xc000068290, 0x16, 0x114e7e0, 0xc00006a390)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:254 +0x49b
text/template.(*state).walk(0xc0000a9e40, 0x110e5c0, 0xc000068290, 0x16, 0x114e9e0, 0xc00006a2d0)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:262 +0x142
text/template.(*Template).execute(0xc000084080, 0x114e2e0, 0xc0000a0000, 0x110e5c0, 0xc000068290, 0x0, 0x0)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:217 +0x215
text/template.(*Template).Execute(0xc000084080, 0x114e2e0, 0xc0000a0000, 0x110e5c0, 0xc000068290, 0x0, 0xc00007ef38)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:200 +0x53
main.main()

I understand that the user should wrap the above in a with or something, but having maintained Hugo for some years I can say this:

This is a very common problem
And the error you get from this is extremely hard to debug

That, and if you follow the code in the Go source, there are nil checks for the other cases, but not for the "method case":

https://github.com/golang/go/blob/master/src/text/template/exec.go#L594

The text was updated successfully, but these errors were encountered:

odeke-em · 2018-10-16T22:27:55Z

Thank you @bep for filing this issue. For posterity the link you've provided would have this permalink

go/src/text/template/exec.go

Line 594 in fa913a3

s.errorf("nil pointer evaluating %s.%s", typ, fieldName)

I'll tweak the title a little bit and also kindly page @mvdan @robpike for text/template

odeke-em · 2018-10-16T23:13:15Z

Also updated since we get the same result for html/template too, please feel free to update accordingly.

mvdan · 2018-10-17T07:45:14Z

I agree that this shouldn't panic, just like in the other cases. Otherwise, some valid uses of text/template would need to be used along with a recover. Will take a look.

mvdan · 2018-10-17T08:27:54Z

Here is a shorter repro - the first method call isn't necessary:

package main

import (
        "os"
        "text/template"
)

type T struct {
        name string
}

func (t *T) Name() string {
        return t.name
}

func main() {
        tmpl := template.Must(template.New("").Parse(`{{ .Name }}`))
        var t *T
        tmpl.Execute(os.Stdout, t)
}

Upon closer inspection, I'm no longer sure that this is a simple fix. Calling methods on nil receivers is a valid thing to do in Go, and some named types with methods have nil as a valid value that can be used like any other.

I do understand that the reported error is very confusing though, as the panic looks almost exactly like what a nil dereference bug in text/template itself would look like.

@bep do you specifically want an error in this case, or would a better panic message with more context be enough? I'm thinking that we have a few options:

Run the method, and have the above case panic normally (what happens now)
Run the method, recover any panic during it, add context to the message, and panic again
Run the method, recover any panic during it, and return the panic message with an error
Always error when running methods on nil receivers (I presume this breaks valid programs)

The least invasive solution is adding context to the panic. For example, imagine:

panic: text/template: encountered a panic while evaluating call .Name at <template source position>:
 	panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x10ec8a5]

goroutine 1 [running]:
text/template.errRecover(0xc0000a9ec0)
	/usr/local/Cellar/go/1.11/libexec/src/text/template/exec.go:160 +0x1d0
...

A somewhat more friendly approach would be to return an ExecError, which can already contain context about where an error was encountered:

fmt.Sprintf("template: %s: executing %q at <%s>: %s", location, name, doublePercent(context), format)

bep · 2018-10-17T08:44:56Z

@bep do you specifically want an error in this case, or would a better panic message with more context be enough?

A panic is fine as long as I can get some contextual information from it (template name, line number, field/method name). The below is an example of what I get if I try to access a non-existing field:

template: example.html:2:15: executing "example.html" at <.NotFound>: can't evaluate field ...`

If I could get something similar as a panic in my "nil method" example, I would be a happy camper.

mvdan · 2018-10-17T09:30:03Z

There are other instances of standard library packages recovering panics and not panicking again, such as fmt with Format method calls. So there is some precedent to not panicking here, and I'd prefer if we returned an error instead.

Then there is the question of whether swapping this panic with an error would be considered a breaking change. If so, we could then add a setting via Template.Option, such as safecalls, which would replace panics during calls with errors containing all the context and information.

/cc @robpike @rogpeppe for a decision. I'd prefer returning an error in this case, even if it means an extra template option, over returning panics with context.

mvdan · 2018-10-17T10:04:33Z

Just realised that, even if we just replace a panic with another one that contains context, we could still break existing programs. They could depend on the type of panic's argument, which would be lost if we switch the panic or replace it with an error.

So I think my final proposal is to add Template.Option("safecalls"), which will recover any panic during template function calls, and return an error.

rsc · 2018-10-17T18:12:33Z

This does seem very similar to fmt. It seems reasonable to catch a panic in a method and turn it into a more gracefully-returned error.

mvdan · 2018-10-17T19:34:20Z

@rsc to clarify - are you fine with changing this behavior always, or only behind a Template.Option like I described above?

gopherbot · 2018-10-18T10:44:58Z

Change https://golang.org/cl/143097 mentions this issue: text/template: recover panics during function calls

gopherbot · 2019-01-29T02:43:30Z

Change https://golang.org/cl/159998 mentions this issue: doc: go1.12: mention change in text/template user function panic

Updates #28242 Change-Id: Ib717b64f1f368cc889895a2437ff2943ed4eab0d Reviewed-on: https://go-review.googlesource.com/c/159998 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Daniel Martí <mvdan@mvdan.cc>

Updates golang#28242 Change-Id: Ib717b64f1f368cc889895a2437ff2943ed4eab0d Reviewed-on: https://go-review.googlesource.com/c/159998 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Daniel Martí <mvdan@mvdan.cc>

bep mentioned this issue Oct 16, 2018

Method invoke in templates panics if the receiver is nil gohugoio/hugo#5327

Closed

odeke-em changed the title ~~text/template: Method invoke panics if the receiver is nil~~ text/template: method invocations on nil receivers should return an error and not panic Oct 16, 2018

odeke-em changed the title ~~text/template: method invocations on nil receivers should return an error and not panic~~ text/template, html/template: method invocations on nil receivers should return an error and not panic Oct 16, 2018

FiloSottile added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 16, 2018

FiloSottile added this to the Unplanned milestone Oct 16, 2018

mvdan added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 17, 2018

mvdan changed the title ~~text/template, html/template: method invocations on nil receivers should return an error and not panic~~ text/template: panics encountered during function calls are hard to treat and debug Oct 17, 2018

gopherbot closed this as completed in 202e903 Oct 24, 2018

bcmills mentioned this issue Oct 25, 2018

proposal: fmt: don't recover panics except for dereferencing nil fmt.Stringer receivers #28150

Closed

bep mentioned this issue Feb 9, 2019

text/template: panics on method on nil interface value #30143

Closed

golang locked and limited conversation to collaborators Jan 29, 2020

gopherbot added the FrozenDueToAge label Jan 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

text/template: panics encountered during function calls are hard to treat and debug #28242

text/template: panics encountered during function calls are hard to treat and debug #28242

bep commented Oct 16, 2018 •

edited

odeke-em commented Oct 16, 2018

odeke-em commented Oct 16, 2018

mvdan commented Oct 17, 2018

mvdan commented Oct 17, 2018 •

edited

bep commented Oct 17, 2018 •

edited

mvdan commented Oct 17, 2018 •

edited

mvdan commented Oct 17, 2018

rsc commented Oct 17, 2018

mvdan commented Oct 17, 2018

gopherbot commented Oct 18, 2018

gopherbot commented Jan 29, 2019

text/template: panics encountered during function calls are hard to treat and debug #28242

text/template: panics encountered during function calls are hard to treat and debug #28242

Comments

bep commented Oct 16, 2018 • edited

odeke-em commented Oct 16, 2018

odeke-em commented Oct 16, 2018

mvdan commented Oct 17, 2018

mvdan commented Oct 17, 2018 • edited

bep commented Oct 17, 2018 • edited

mvdan commented Oct 17, 2018 • edited

mvdan commented Oct 17, 2018

rsc commented Oct 17, 2018

mvdan commented Oct 17, 2018

gopherbot commented Oct 18, 2018

gopherbot commented Jan 29, 2019

bep commented Oct 16, 2018 •

edited

mvdan commented Oct 17, 2018 •

edited

bep commented Oct 17, 2018 •

edited

mvdan commented Oct 17, 2018 •

edited