text/template: should t.init() be executed before t.muTmpl.Lock() in AddParseTree() method?

[And-ZJ]

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

[ianlancetaylor]

Thanks. I don't know how much sense that makes, but it used to work, so I guess we should let it keep working.

[ianlancetaylor]

@gopherbot Please open backports to 1.16 and 1.17

Fix regression in AddParseTree due to earlier fix for #39807.

[gopherbot]

Backport issue(s) opened: #48443 (for 1.16), #48444 (for 1.17).

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

[gopherbot]

Change https://golang.org/cl/350730 mentions this issue: text/template: initialize template before locking it

[gopherbot]

Change https://golang.org/cl/351115 mentions this issue: [release-branch.go1.17] text/template: initialize template before locking it

[gopherbot]

Change https://golang.org/cl/351116 mentions this issue: [release-branch.go1.16] text/template: initialize template before locking it