text/template: should t.init() be executed before t.muTmpl.Lock() in AddParseTree() method? #48436
Description
What version of Go are you using (go version)?
$ go version go version go1.16.8 windows/amd64
Does this issue reproduce with the latest release?
yes for go1.17.1 and 1.16.8, but not in 1.16.7
What operating system and processor architecture are you using (go env)?
go env Output
$ go env (not relevant)
What did you do?
I read the CL "add lock for Template.tmpl to fix data race" https://golang.org/cl/348580. I'm a little confused about the following function fix:
// AddParseTree associates the argument parse tree with the template t, giving
// it the specified name. If the template has not been defined, this tree becomes
// its definition. If it has been defined and already has that name, the existing
// definition is replaced; otherwise a new template is created, defined, and returned.
func (t *Template) AddParseTree(name string, tree *parse.Tree) (*Template, error) {
t.muTmpl.Lock()
defer t.muTmpl.Unlock()
t.init()
nt := t
if name != t.name {
nt = t.New(name)
}
// Even if nt == t, we need to install it in the common.tmpl map.
if t.associate(nt, tree) || nt.Tree == nil {
nt.Tree = tree
}
return nt, nil
}As you can see, it executes t.muTmpl.Lock() before t.init().
The init() method and the common structure are listed below.
// init guarantees that t has a valid common structure.
func (t *Template) init() {
if t.common == nil {
c := new(common)
c.tmpl = make(map[string]*Template)
c.parseFuncs = make(FuncMap)
c.execFuncs = make(map[string]reflect.Value)
t.common = c
}
}
// common holds the information shared by related templates.
type common struct {
tmpl map[string]*Template // Map from name to defined templates.
muTmpl sync.RWMutex // protects tmpl
option option
// We use two maps, one for parsing and one for execution.
// This separation makes the API cleaner since it doesn't
// expose reflection to the client.
muFuncs sync.RWMutex // protects parseFuncs and execFuncs
parseFuncs FuncMap
execFuncs map[string]reflect.Value
}In the init() method, if t.common is nil, the t.common will be initialized, including the default initialization of muTmpl.
Now return to the AddParseTree() method.
Because it executes t.init(), which means that t.common may be nil, executing t.muTmpl.Lock() first may produce nil panic. So in this method, should t.init() be executed before t.muTmpl.Lock()?
A simple reproduction procedure:
package main
import (
"testing"
"text/template"
)
func TestAddParseTreeInitAndLock(t *testing.T) {
template.Must(new(template.Template).AddParseTree("go", nil))
}
What did you expect to see?
normal exit
What did you see instead?
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference