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 on method on nil interface value #30143

Closed
bep opened this issue Feb 9, 2019 · 9 comments

Comments

@bep
Copy link
Contributor

commented Feb 9, 2019

This is a follow-up to #28242 and discovered while testing go1.12beta2 with Hugo.

The program below is a common variant of the crasher in #28242 -- that still panics with go1.12beta2

package main

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

type Namer interface {
	Name() string
}

type F struct {
	name string
}

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

func (f *F) Other() Namer {
	var n Namer
	return n
}

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)
	}

}
▶ go1.12beta2 run main.go
panic: reflect: Method on nil interface value [recovered]
	panic: reflect: Method on nil interface value

goroutine 1 [running]:
text/template.errRecover(0xc0000b5f48)
	/Users/bep/sdk/go1.12beta2/src/text/template/exec.go:166 +0x1b3
panic(0x1105be0, 0x1158df0)
	/Users/bep/sdk/go1.12beta2/src/runtime/panic.go:522 +0x1b5
reflect.Value.Method(0x1113680, 0xc00000c108, 0x94, 0x0, 0x4, 0x0, 0x0)
	/Users/bep/sdk/go1.12beta2/src/reflect/value.go:1250 +0x20a
reflect.Value.MethodByName(0x1113680, 0xc00000c108, 0x94, 0xc0000180d7, 0x4, 0x94, 0x601, 0x115d080)
	/Users/bep/sdk/go1.12beta2/src/reflect/value.go:1285 +0xd5
text/template.(*state).evalField(0xc0000b5ec8, 0x1111f20, 0xc000010100, 0x16, 0xc0000180d7, 0x4, 0x115d080, 0xc00009e210, 0xc0000100e0, 0x1, ...)
	/Users/bep/sdk/go1.12beta2/src/text/template/exec.go:585 +0x218
text/template.(*state).evalFieldChain(0xc0000b5ec8, 0x1111f20, 0xc000010100, 0x16, 0x1111f20, 0xc000010100, 0x16, 0x115d080, 0xc00009e210, 0xc00000c060, ...)
	/Users/bep/sdk/go1.12beta2/src/text/template/exec.go:554 +0x220
text/template.(*state).evalFieldNode(0xc0000b5ec8, 0x1111f20, 0xc000010100, 0x16, 0xc00009e210, 0xc0000100e0, 0x1, 0x1, 0x1111de0, 0x1250ae0, ...)
	/Users/bep/sdk/go1.12beta2/src/text/template/exec.go:518 +0x114
text/template.(*state).evalCommand(0xc0000b5ec8, 0x1111f20, 0xc000010100, 0x16, 0xc00009e1b0, 0x1111de0, 0x1250ae0, 0x99, 0xd0, 0xd0, ...)
	/Users/bep/sdk/go1.12beta2/src/text/template/exec.go:456 +0x79b
text/template.(*state).evalPipeline(0xc0000b5ec8, 0x1111f20, 0xc000010100, 0x16, 0xc000054240, 0xc0000aa0f0, 0xc0000aa038, 0xc0000b80d0)
	/Users/bep/sdk/go1.12beta2/src/text/template/exec.go:430 +0x11c
text/template.(*state).walk(0xc0000b5ec8, 0x1111f20, 0xc000010100, 0x16, 0x115cf40, 0xc00009e240)
	/Users/bep/sdk/go1.12beta2/src/text/template/exec.go:254 +0x49c
text/template.(*state).walk(0xc0000b5ec8, 0x1111f20, 0xc000010100, 0x16, 0x115d140, 0xc00009e180)
	/Users/bep/sdk/go1.12beta2/src/text/template/exec.go:262 +0x143
text/template.(*Template).execute(0xc000050040, 0x115b6c0, 0xc00009e090, 0x1111f20, 0xc000010100, 0x0, 0x0)
	/Users/bep/sdk/go1.12beta2/src/text/template/exec.go:217 +0x1e8
text/template.(*Template).Execute(...)
	/Users/bep/sdk/go1.12beta2/src/text/template/exec.go:200
main.main()
	/Users/bep/dev/go/bep/temp/main.go:35 +0x136
exit status 2

/cc @mvdan

@mvdan

This comment has been minimized.

Copy link
Member

commented Feb 9, 2019

Thanks for the report. It seems like the issue here is with field evaluation, not with the actual call; we panic on ptr.MethodByName(fieldName), which is when we're finding the method to possibly call right after.

Of course, the problem is that the receiver is a nil interface, so there's no method function we can get to. That small piece of code should add an extra check at best, or a recover at worst.

Not a regression in 1.12, and the release is overdue, so I'm going to milestone for 1.13 for now. If we decide this is important enough for a backport, we can always reconsider for a bugfix release like 1.12.1.

@mvdan mvdan self-assigned this Feb 9, 2019

@mvdan mvdan added this to the Go1.13 milestone Feb 9, 2019

@mvdan mvdan added the NeedsFix label Feb 9, 2019

@bep

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2019

Not a regression in 1.12

So, this may be a new test case and a different code path, but for the end user, this is the same issue. And in that sense, it is a regression since the error behaviour now is inconsistent (two panics is more consistent than 1 error and 1 panic for the "same thing"). I know that argument isn't the strongest argument in The Book of Arguments, but I had to try :-) ...

The workaround for the above would be to always return typed nils:

func (f *F) Other() Namer {
	var ff *F
	return ff
}

Which I guess is doable in most situations. But it would, of course, be simpler if a patch to fix this in Go was accepted for Go 1.12 beta3/rc1 ...

/cc @spf13

I will try to get in earlier to test the Go beta releases in the future, but the merge of the #28242 fix went unnoticed for little too long. But as always, I really appreciated the hard and solid work you, @mvdan, and the other Go contributors do.

@mvdan

This comment has been minimized.

Copy link
Member

commented Feb 9, 2019

And in that sense, it is a regression since the error behaviour now is inconsistent (two panics is more consistent than 1 error and 1 panic for the "same thing").

Perhaps so. And I would have definitely said we should include it in 1.12, had the bug report been opened a couple of weeks ago :) At this point, I don't want to assume that it is fine to add more blocking issues for the 1.12 release.

For now, I'll work on a CL, and we can try to get it cherry-picked for the release early next week. I'm sure others will have opinions on the matter; CC @ianlancetaylor @andybons

@mvdan

This comment has been minimized.

Copy link
Member

commented Feb 9, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Feb 9, 2019

Change https://golang.org/cl/161761 mentions this issue: text/template: error on method calls on nil interfaces

@bep

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2019

@ianlancetaylor asked for code to show that this is a regression in Go 1.12.

package main

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

type Namer interface {
	Name() string
}

type F struct {
	name string
}

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

var nilF *F

func (f *F) A1() Namer {
	var ff *F
	return ff
}

func (f *F) A2() Namer {
	return nil
}

func main() {

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

	do(data, "A1")
	do(data, "A2")

}

func do(data interface{}, key string) {
	defer func() {
		if r := recover(); r != nil {
			fmt.Println("Recovered in f", r)
		}
	}()

	var buf bytes.Buffer
	tmpl, err := template.New("").Parse(fmt.Sprintf(`{{ .%s.Name }}`, key))
	if err != nil {
		log.Fatal(err)
	}

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

}

For Go 1.11 the above runs as:

▶ go run main.go
Recovered in f runtime error: invalid memory address or nil pointer dereference
Recovered in f reflect: Method on nil interface value

FOr Go 1.12 the above runs as:

019/02/10 00:12:07 template: :1:6: executing "" at <.A1.Name>: error calling Name: runtime error: invalid memory address or nil pointer dereference
exit status 1

So, the second panic is fixed, leaving us with an inconsistent error behavior; If one of them panics, is the other now considered less of an error? How do I handle that?

Note that I'm not going further into the "is this a regression" arguments. This small patch would save thousands of work hours.

@mvdan

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

Continuing the cherry-pick discussion here instead of the CL.

I agree with @ianlancetaylor that it's unnecessary risk to merge this now, given that the 1.12 release is already very late. I see @bep's point about the inconsistency, but that's a double edged sword; at this point in the freeze, if we were to take a decision in favor of consistency, it would probably be to revert my older CL.

As I said before, I'd be happy to vouch for its inclusion in 1.12, had the issue been opened last month. For now, the earliest we can aim for is 1.12.1, arguing that the original 1.12 CL did not cover all edge cases.

Also, see my arguments about pragmatism and hours saved.

I don't think I buy that argument; the same can be said about adding risk and further delay to an already late release. It's unfortunate that the first 1.12 release won't be perfect, but we're way past the point where non-regression bugs can be included.

@gopherbot gopherbot closed this in 15b4c71 Feb 26, 2019

@mvdan

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@gopherbot please backport to 1.12

@gopherbot

This comment has been minimized.

Copy link

commented Feb 28, 2019

Backport issue(s) opened: #30464 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.