text/template: Fix nil pointer dereference

text/template: Fix nil pointer dereference

Handle *parse.IdentifierNode in evalArg(). This
bug happens when evaluating a field or method
on an object returned from a template function
(e.g. {{ MyFunction.Field1 }}))

[bradfitz, 2013-03-22 15:55:29]

Please add a test.

[r, 2013-03-22 16:33:52]

NOT LGTM
This is a can of worms that has been opened and closed multiple times.
If you're getting a crash, we can fix that, but you should not be assuming
a field is a function to be invoked unless it's the argument to an explicit
"call" action. Otherwise it's impossible to distinguish a function value from
a function call, and things get hopelessly confusing.

[Hierro, 2013-03-27 16:18:42]

To make things more clear, I've written this test:


package template

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

type Object struct {
	Attribute string
}

func (o *Object) Function() string {
	return "Function"
}

func test7712050(t *testing.T, tmpl1 string, tmpl2 string) {
	o := &Object{"Attribute"}
	// Create template which uses the object passed as an argument
	t1 := template.New("t1")
	t1 = template.Must(t1.Parse(tmpl1))
	var out1 bytes.Buffer
	t1.Execute(&out1, o)
	s1 := string(out1.Bytes())

	// Create another template which gets the object from a function
	t2 := template.New("t2")
	t2.Funcs(template.FuncMap{
		"GetObject": func() interface{} {
			return o
		},
	})
	t2 = template.Must(t2.Parse(tmpl2))
	var out2 bytes.Buffer
	t2.Execute(&out2, o)
	s2 := string(out2.Bytes())
	if s1 != s2 {
		t.Error("%s != %s", s1, s2)
	}
}

func Test7712050Attribute(t *testing.T) {
	test7712050(t, "{{ .Attribute }}", "{{ GetObject.Attribute }}")
}

func Test7712050Func(t *testing.T) {
	test7712050(t, "{{ .Function }}", "{{ GetObject.Function }}")
}

func Test7712050Both(t *testing.T) {
	test7712050(t, "{{ .Attribute }} {{ .Function }}", "{{ GetObject.Attribute }}
{{ GetObject.Function }}")
}

Running it with the current go tip I get this result:

=== RUN Test7712050Attribute
--- PASS: Test7712050Attribute (0.00 seconds)
panic: runtime error: invalid memory address or nil pointer dereference
[recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x98 pc=0x490d73]

goroutine 3 [running]:
testing.func·004()
	/home/fiam/go/src/pkg/testing/testing.go:341 +0xbc
template.errRecover(0x7f97d3b5ee40)
	/home/fiam/gocode/src/template/exec.go:100 +0xb7
template.(*state).evalArg(0xc200088500, 0x5248a0, 0xc20007a330, 0x160, 0x0, ...)
	/home/fiam/gocode/src/template/exec.go:624 +0x413
template.(*state).evalChainNode(0xc200088500, 0x5248a0, 0xc20007a330, 0x160,
0xc2000884c0, ...)
	/home/fiam/gocode/src/template/exec.go:410 +0xbc
template.(*state).evalCommand(0xc200088500, 0x5248a0, 0xc20007a330, 0x160,
0xc20007b690, ...)
	/home/fiam/gocode/src/template/exec.go:348 +0x2b0
template.(*state).evalPipeline(0xc200088500, 0x5248a0, 0xc20007a330, 0x160,
0xc20006b320, ...)
	/home/fiam/gocode/src/template/exec.go:324 +0x150
template.(*state).walk(0xc200088500, 0x5248a0, 0xc20007a330, 0x160,
0xc20006b230, ...)
	/home/fiam/gocode/src/template/exec.go:159 +0x105
template.(*state).walk(0xc200088500, 0x5248a0, 0xc20007a330, 0x160,
0xc20006b280, ...)
	/home/fiam/gocode/src/template/exec.go:167 +0x6f1
template.(*Template).Execute(0xc200088480, 0xc20007b600, 0xc20006e460, 0x5248a0,
0xc20007a330, ...)
	/home/fiam/gocode/src/template/exec.go:147 +0x2e5
test4.test7712050(0xc20007c090, 0x56b9d0, 0x10, 0x5743d0, 0x19, ...)
	/home/fiam/gocode/src/test4/7712050_test.go:35 +0x3a5
test4.Test7712050Attribute(0xc20007c090)
	/home/fiam/gocode/src/test4/7712050_test.go:43 +0x50
testing.tRunner(0xc20007c090, 0x61f160)
	/home/fiam/go/src/pkg/testing/testing.go:346 +0x8a
created by testing.RunTests
	/home/fiam/go/src/pkg/testing/testing.go:426 +0x86b

goroutine 1 [chan receive]:
testing.RunTests(0x5861e8, 0x61f160, 0x3, 0x3, 0x1, ...)
	/home/fiam/go/src/pkg/testing/testing.go:427 +0x88e
testing.Main(0x5861e8, 0x61f160, 0x3, 0x3, 0x623740, ...)
	/home/fiam/go/src/pkg/testing/testing.go:358 +0x8a
main.main()
	test4/_test/_testmain.go:47 +0x9a
exit status 2
FAIL	test4	0.089s

The nil pointer dereference happens on line 626 in exec.go, because typ is nil
when switch typ.Kind() is reached. It is my understanding that in this code path
with typ == nil, n can only represent a function call (I might be wrong on that,
since the template pkg is quite complex), so with the following patch both this
test and all the tests in text/template pass:

diff -r d040d5f08d5d src/pkg/text/template/exec.go
--- a/src/pkg/text/template/exec.go	Wed Mar 27 05:59:06 2013 -0700
+++ b/src/pkg/text/template/exec.go	Wed Mar 27 17:16:36 2013 +0100
@@ -619,6 +619,10 @@
 		return s.validateType(s.evalVariableNode(dot, arg, nil, zero), typ)
 	case *parse.PipeNode:
 		return s.validateType(s.evalPipeline(dot, arg), typ)
+	case *parse.IdentifierNode:
+		if typ == nil {
+			return s.evalFunction(dot, arg, arg, nil, zero)
+		}
 	}
 	switch typ.Kind() {
 	case reflect.Bool:


=== RUN Test7712050Attribute
--- PASS: Test7712050Attribute (0.00 seconds)
=== RUN Test7712050Func
--- PASS: Test7712050Func (0.00 seconds)
=== RUN Test7712050Both
--- PASS: Test7712050Both (0.00 seconds)
PASS
ok  	test4	0.037s


Regards,
Alberto

[r, 2013-03-27 17:03:03]

As I said before, you may have a crash and there is clearly a bug in
the template executor but the fix is not correct. GetObject is a
function you must invoke. You cannot write
  f.x
where f is a function that must be called. You must use the "call"
builtin to call the function and with the value it returns, you can
address the field.

Now that you have provided an example of the crash I will look into
the correct fix.

-rob

[Hierro, 2013-03-27 18:48:06]

Hi Rob,

Could you please clarify why using "call" is required? I was under the
impression that call was only needed when you wanted to pass arguments. Take a
look at the following test:

package template

import (
	"fmt"
	"bytes"
	"template"
	"testing"
)

type Object struct {
	Attribute string
}

func (o *Object) Function() string {
	return "Function"
}

func (o *Object) Object() *Object {
	return o
}

func execute(t *testing.T, o *Object, tmpl string) string {
	t1 := template.New("t1")
	t1.Funcs(template.FuncMap{
		"GetObject": func() interface{} {
			return o
		},
	})
	t1 = template.Must(t1.Parse(tmpl))
	var out1 bytes.Buffer
	t1.Execute(&out1, o)
	return string(out1.Bytes())
}

func testTemplates(t *testing.T, tmpl1 string, tmpl2 string) {
	o := &Object{"Attribute"}
	s1 := execute(t, o, tmpl1)
	s2 := execute(t, o, tmpl2)
	if s1 != s2 {
		t.Errorf("\"%s\" != \"%s\" (templates were \"%s\" and \"%s\"", s1, s2, tmpl1,
tmpl2)
	} else {
	    fmt.Printf("Templates \"%s\" and \"%s\" produced and equal result: \"%s\"
\n", tmpl1, tmpl2, s1)
	}
}

func Test7712050(t *testing.T) {
	testTemplates(t, "{{ .Attribute }}", "{{ .Object.Attribute }}")
	testTemplates(t, "{{ .Function }}", "{{ .Object.Function }}")
	testTemplates(t, "{{ .Attribute }}", "{{ GetObject.Attribute }}")
	testTemplates(t, "{{ .Function }}", "{{ GetObject.Function }}")
}

=== RUN Test7712050
Templates "{{ .Attribute }}" and "{{ .Object.Attribute }}" produced and equal
result: "Attribute" 
Templates "{{ .Function }}" and "{{ .Object.Function }}" produced and equal
result: "Function" 
--- PASS: Test7712050 (0.00 seconds)
panic: runtime error: invalid memory address or nil pointer dereference
[recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x98 pc=0x490ee3]

goroutine 3 [running]:
testing.func·004()
	/home/fiam/go/src/pkg/testing/testing.go:341 +0xbc
template.errRecover(0x7f231bd0adc8)
	/home/fiam/gocode/src/template/exec.go:100 +0xb7
template.(*state).evalArg(0xc20007f740, 0x52cca0, 0xc200068610, 0x160, 0x0, ...)
	/home/fiam/gocode/src/template/exec.go:628 +0x413
template.(*state).evalChainNode(0xc20007f740, 0x52cca0, 0xc200068610, 0x160,
0xc20007f700, ...)
	/home/fiam/gocode/src/template/exec.go:410 +0xbc
template.(*state).evalCommand(0xc20007f740, 0x52cca0, 0xc200068610, 0x160,
0xc2000972a0, ...)
	/home/fiam/gocode/src/template/exec.go:348 +0x2b0
template.(*state).evalPipeline(0xc20007f740, 0x52cca0, 0xc200068610, 0x160,
0xc20006c4b0, ...)
	/home/fiam/gocode/src/template/exec.go:324 +0x150
template.(*state).walk(0xc20007f740, 0x52cca0, 0xc200068610, 0x160,
0xc20006c230, ...)
	/home/fiam/gocode/src/template/exec.go:159 +0x105
template.(*state).walk(0xc20007f740, 0x52cca0, 0xc200068610, 0x160,
0xc20006c280, ...)
	/home/fiam/gocode/src/template/exec.go:167 +0x6f1
template.(*Template).Execute(0xc20007f6c0, 0xc20006f9c0, 0xc20007e380, 0x52cca0,
0xc200068610, ...)
	/home/fiam/gocode/src/template/exec.go:147 +0x2e5
test4.execute(0xc200086000, 0xc200068610, 0x575430, 0x19, 0xc200068690, ...)
	/home/fiam/gocode/src/test4/7712050_test.go:31 +0x1ec
test4.testTemplates(0xc200086000, 0x56ca70, 0x10, 0x575430, 0x19, ...)
	/home/fiam/gocode/src/test4/7712050_test.go:38 +0xd0
test4.Test7712050(0xc200086000)
	/home/fiam/gocode/src/test4/7712050_test.go:49 +0xc2
testing.tRunner(0xc200086000, 0x61da80)
	/home/fiam/go/src/pkg/testing/testing.go:346 +0x8a
created by testing.RunTests
	/home/fiam/go/src/pkg/testing/testing.go:426 +0x86b

goroutine 1 [chan receive]:
testing.RunTests(0x587290, 0x61da80, 0x1, 0x1, 0x1, ...)
	/home/fiam/go/src/pkg/testing/testing.go:427 +0x88e
testing.Main(0x587290, 0x61da80, 0x1, 0x1, 0x625600, ...)
	/home/fiam/go/src/pkg/testing/testing.go:358 +0x8a
main.main()
	test4/_test/_testmain.go:43 +0x9a
exit status 2
FAIL	test4	0.028s

Calling the function on the object without using the call builtin (e.g.
.Object.Function ) seems to work just fine. The problem happens when evaluating
a pipeline with the first element being an identifier (GetObject in this case),
because the executor doesn't seem to be prepared to deal with that. Keep in mind
that the crash happens when evalArg() tries to evaluate "GetObject", not when
evalArg() tries to evaluate "Function".

Since any fields/functions on the object passed to the template must be prefixed
with a dot, I think that if the executor finds an identifier without a dot in it
MUST be a function which was previously added using template.Funcs(). In fact,
looking at like parse.go:591 (function term()) it seems that the parser assumes
exactly that:

       case itemIdentifier:
                if !t.hasFunction(token.val) {
                        t.errorf("function %q not defined", token.val)
                }   
                return NewIdentifier(token.val).SetPos(token.pos)

While the fix I provided might not be correct, the code in parse.go suggest that
the test cases in this message should pass without any modifications (e.g.
without adding "call").

Regards,
Alberto

[r, 2013-03-27 20:03:05]

https://codereview.appspot.com/7861046 shows the way. You need
parentheses to chain a function call.

-rob

[r, 2013-03-27 20:03:40]

Also see issue 3999.

-rob

[r, 2013-03-27 20:11:21]

On a tangential note, you need to start checking the error returns
from template.Execute.


-rob

[r, 2013-03-27 20:14:42]

The use of templates is incorrect, but there is a bug in the template
package, fixed at https://codereview.appspot.com/7861046/ .

To avoid ambiguity in execution, in a function invocation starting a
field chain, such as fn.field1.field2,
the function must be parenthesized: (fn).field1.field2.  It's
essentially the same issue as distinguishing a function value from a
function call in Go, made more confusing by the autoevaluations
performed by the template package.

-rob

[Hierro, 2013-03-27 21:00:22]

On 2013/03/27 20:11:21, r wrote:
> On a tangential note, you need to start checking the error returns
> from template.Execute.
> 
> 
> -rob

I do check them, but the point of that test was demostrating the nil pointer
deference. No need to complicate the code with (in that case) useless error
checking.

Regards,
Alberto

[Hierro, 2013-03-27 21:05:17]

> To avoid ambiguity in execution, in a function invocation starting a
> field chain, such as fn.field1.field2,
> the function must be parenthesized: (fn).field1.field2.  It's
> essentially the same issue as distinguishing a function value from a
> function call in Go, made more confusing by the autoevaluations
> performed by the template package.

Regarding templates, I really can't see any ambiguity in fn.field1.field2, since
functions don't have attributes. Also, .field1.fn.field2 is correctly executed
without any parenthesis. I think that's pretty inconsistent and not intuitive at
all. Parenthesis should be either always required or never required, and the
fact that the pipe starts with a field or a function should be irrelevant.

Regards,
Alberto

[Hierro, 2013-03-27 21:10:58]

On 2013/03/27 20:03:40, r wrote:
> Also see issue 3999.
> 
> -rob

https://codereview.appspot.com/3999/ says there's no such issue.

Regards,
Alberto

[dave_cheney.net, 2013-03-27 21:13:13]

Try golang.org/issue/3999

On 28/03/2013, at 8:10, alberto.garcia.hierro@gmail.com wrote:

> On 2013/03/27 20:03:40, r wrote:
>> Also see issue 3999.
> 
>> -rob
> 
> https://codereview.appspot.com/3999/ says there's no such issue.
> 
> Regards,
> Alberto
> 
> https://codereview.appspot.com/7712050/
> 
> -- 
> 
> ---You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
> 

[Hierro, 2013-03-27 21:27:07]

On 2013/03/27 21:13:13, dfc wrote:
> Try golang.org/issue/3999

Thanks!