Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
bep opened this issue Oct 16, 2018 · 11 comments
Closed

Comments

@bep
Copy link
Contributor

@bep bep commented Oct 16, 2018

 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

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Oct 16, 2018

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

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 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 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
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Oct 16, 2018

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

@FiloSottile FiloSottile added this to the Unplanned milestone Oct 16, 2018
@mvdan
Copy link
Member

@mvdan mvdan commented Oct 17, 2018

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
Copy link
Member

@mvdan mvdan commented Oct 17, 2018

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
Copy link
Contributor Author

@bep bep commented Oct 17, 2018

@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
Copy link
Member

@mvdan mvdan commented Oct 17, 2018

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 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
@mvdan
Copy link
Member

@mvdan mvdan commented Oct 17, 2018

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
Copy link
Contributor

@rsc rsc commented Oct 17, 2018

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
Copy link
Member

@mvdan mvdan commented Oct 17, 2018

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

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 18, 2018

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 29, 2019

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

gopherbot pushed a commit that referenced this issue Jan 29, 2019
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>
nebulabox added a commit to nebulabox/go that referenced this issue Feb 18, 2019
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>
nebulabox added a commit to nebulabox/go that referenced this issue Feb 20, 2019
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>
@golang golang locked and limited conversation to collaborators Jan 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.