From 5a4db102b21489c39b3a654e06cc25155432a38a Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Wed, 2 Dec 2020 19:18:46 -0800 Subject: html/template: avoid race when escaping updates template Fixes #39807 Change-Id: Icf384f800e2541bc753507daa3a9bc7e5d1c3f79 Reviewed-on: https://go-review.googlesource.com/c/go/+/274450 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Go Bot Reviewed-by: Roberto Clapis Reviewed-by: Russ Cox --- src/html/template/exec_test.go | 70 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) (limited to 'src/html/template/exec_test.go') diff --git a/src/html/template/exec_test.go b/src/html/template/exec_test.go index 232945a0bb..eb00824260 100644 --- a/src/html/template/exec_test.go +++ b/src/html/template/exec_test.go @@ -14,6 +14,7 @@ import ( "io" "reflect" "strings" + "sync" "testing" "text/template" ) @@ -1706,3 +1707,72 @@ func TestIssue31810(t *testing.T) { t.Errorf("%s got %q, expected %q", textCall, b.String(), "result") } } + +// Issue 39807. There was a race applying escapeTemplate. + +const raceText = ` +{{- define "jstempl" -}} +var v = "v"; +{{- end -}} + +` + +func TestEscapeRace(t *testing.T) { + tmpl := New("") + _, err := tmpl.New("templ.html").Parse(raceText) + if err != nil { + t.Fatal(err) + } + const count = 20 + for i := 0; i < count; i++ { + _, err := tmpl.New(fmt.Sprintf("x%d.html", i)).Parse(`{{ template "templ.html" .}}`) + if err != nil { + t.Fatal(err) + } + } + + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < count; j++ { + sub := tmpl.Lookup(fmt.Sprintf("x%d.html", j)) + if err := sub.Execute(io.Discard, nil); err != nil { + t.Error(err) + } + } + }() + } + wg.Wait() +} + +func TestRecursiveExecute(t *testing.T) { + tmpl := New("") + + recur := func() (HTML, error) { + var sb strings.Builder + if err := tmpl.ExecuteTemplate(&sb, "subroutine", nil); err != nil { + t.Fatal(err) + } + return HTML(sb.String()), nil + } + + m := FuncMap{ + "recur": recur, + } + + top, err := tmpl.New("x.html").Funcs(m).Parse(`{{recur}}`) + if err != nil { + t.Fatal(err) + } + _, err = tmpl.New("subroutine").Parse(``) + if err != nil { + t.Fatal(err) + } + if err := top.Execute(io.Discard, nil); err != nil { + t.Fatal(err) + } +} -- cgit v1.3 From e60cffa4ca9ae726d96b53817d82d98402017772 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Mon, 21 Dec 2020 11:21:59 -0800 Subject: html/template: attach functions to namespace The text/template functions are stored in a data structure shared by all related templates, so do the same with the original, unwrapped, functions on the html/template side. For #39807 Fixes #43295 Change-Id: I9f64a0a601f1151c863a2833b5be2baf649b6cef Reviewed-on: https://go-review.googlesource.com/c/go/+/279492 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Go Bot Reviewed-by: Emmanuel Odeke --- src/html/template/exec_test.go | 20 ++++++++++++++++++ src/html/template/template.go | 46 +++++++++++++++++++++++++----------------- 2 files changed, 47 insertions(+), 19 deletions(-) (limited to 'src/html/template/exec_test.go') diff --git a/src/html/template/exec_test.go b/src/html/template/exec_test.go index eb00824260..cd6b78a1a9 100644 --- a/src/html/template/exec_test.go +++ b/src/html/template/exec_test.go @@ -1776,3 +1776,23 @@ func TestRecursiveExecute(t *testing.T) { t.Fatal(err) } } + +// Issue 43295. +func TestTemplateFuncsAfterClone(t *testing.T) { + s := `{{ f . }}` + want := "test" + orig := New("orig").Funcs(map[string]interface{}{ + "f": func(in string) string { + return in + }, + }).New("child") + + overviewTmpl := Must(Must(orig.Clone()).Parse(s)) + var out strings.Builder + if err := overviewTmpl.Execute(&out, want); err != nil { + t.Fatal(err) + } + if got := out.String(); got != want { + t.Fatalf("got %q; want %q", got, want) + } +} diff --git a/src/html/template/template.go b/src/html/template/template.go index 09d71d43e2..1ff7e1f7a0 100644 --- a/src/html/template/template.go +++ b/src/html/template/template.go @@ -27,9 +27,7 @@ type Template struct { // template's in sync. text *template.Template // The underlying template's parse tree, updated to be HTML-safe. - Tree *parse.Tree - // The original functions, before wrapping. - funcMap FuncMap + Tree *parse.Tree *nameSpace // common to all associated templates } @@ -42,6 +40,8 @@ type nameSpace struct { set map[string]*Template escaped bool esc escaper + // The original functions, before wrapping. + funcMap FuncMap } // Templates returns a slice of the templates associated with t, including t @@ -260,7 +260,6 @@ func (t *Template) AddParseTree(name string, tree *parse.Tree) (*Template, error nil, text, text.Tree, - nil, t.nameSpace, } t.set[name] = ret @@ -287,14 +286,19 @@ func (t *Template) Clone() (*Template, error) { } ns := &nameSpace{set: make(map[string]*Template)} ns.esc = makeEscaper(ns) + if t.nameSpace.funcMap != nil { + ns.funcMap = make(FuncMap, len(t.nameSpace.funcMap)) + for name, fn := range t.nameSpace.funcMap { + ns.funcMap[name] = fn + } + } + wrapFuncs(ns, textClone, ns.funcMap) ret := &Template{ nil, textClone, textClone.Tree, - t.funcMap, ns, } - ret.wrapFuncs() ret.set[ret.Name()] = ret for _, x := range textClone.Templates() { name := x.Name() @@ -307,10 +311,8 @@ func (t *Template) Clone() (*Template, error) { nil, x, x.Tree, - src.funcMap, ret.nameSpace, } - tc.wrapFuncs() ret.set[name] = tc } // Return the template associated with the name of this template. @@ -325,7 +327,6 @@ func New(name string) *Template { nil, template.New(name), nil, - nil, ns, } tmpl.set[name] = tmpl @@ -351,7 +352,6 @@ func (t *Template) new(name string) *Template { nil, t.text.New(name), nil, - nil, t.nameSpace, } if existing, ok := tmpl.set[name]; ok { @@ -382,23 +382,31 @@ type FuncMap map[string]interface{} // type. However, it is legal to overwrite elements of the map. The return // value is the template, so calls can be chained. func (t *Template) Funcs(funcMap FuncMap) *Template { - t.funcMap = funcMap - t.wrapFuncs() + t.nameSpace.mu.Lock() + if t.nameSpace.funcMap == nil { + t.nameSpace.funcMap = make(FuncMap, len(funcMap)) + } + for name, fn := range funcMap { + t.nameSpace.funcMap[name] = fn + } + t.nameSpace.mu.Unlock() + + wrapFuncs(t.nameSpace, t.text, funcMap) return t } // wrapFuncs records the functions with text/template. We wrap them to // unlock the nameSpace. See TestRecursiveExecute for a test case. -func (t *Template) wrapFuncs() { - if len(t.funcMap) == 0 { +func wrapFuncs(ns *nameSpace, textTemplate *template.Template, funcMap FuncMap) { + if len(funcMap) == 0 { return } - tfuncs := make(template.FuncMap, len(t.funcMap)) - for name, fn := range t.funcMap { + tfuncs := make(template.FuncMap, len(funcMap)) + for name, fn := range funcMap { fnv := reflect.ValueOf(fn) wrapper := func(args []reflect.Value) []reflect.Value { - t.nameSpace.mu.RUnlock() - defer t.nameSpace.mu.RLock() + ns.mu.RUnlock() + defer ns.mu.RLock() if fnv.Type().IsVariadic() { return fnv.CallSlice(args) } else { @@ -408,7 +416,7 @@ func (t *Template) wrapFuncs() { wrapped := reflect.MakeFunc(fnv.Type(), wrapper) tfuncs[name] = wrapped.Interface() } - t.text.Funcs(tfuncs) + textTemplate.Funcs(tfuncs) } // Delims sets the action delimiters to the specified strings, to be used in -- cgit v1.3 From 3d85c69a0bf67adec57b76511ccc5e5b0ba9cdf4 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Fri, 22 Jan 2021 14:54:23 -0800 Subject: html/template: revert "avoid race when escaping updates template" This reverts CLs 274450 and 279492, except for the new tests. The new race test is changed to skip, as it now fails. We can try again for 1.17. Original CL descriptions: html/template: attach functions to namespace The text/template functions are stored in a data structure shared by all related templates, so do the same with the original, unwrapped, functions on the html/template side. html/template: avoid race when escaping updates template For #39807 Fixes #43855 Change-Id: I2ce91321ada06ea496a982aefe170eb5af9ba847 Reviewed-on: https://go-review.googlesource.com/c/go/+/285957 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Go Bot Reviewed-by: Emmanuel Odeke --- src/html/template/exec_test.go | 35 +++++++++++++++ src/html/template/template.go | 96 ++++++------------------------------------ 2 files changed, 47 insertions(+), 84 deletions(-) (limited to 'src/html/template/exec_test.go') diff --git a/src/html/template/exec_test.go b/src/html/template/exec_test.go index cd6b78a1a9..7d1bef1782 100644 --- a/src/html/template/exec_test.go +++ b/src/html/template/exec_test.go @@ -1720,6 +1720,8 @@ var v = "v"; ` func TestEscapeRace(t *testing.T) { + t.Skip("this test currently fails with -race; see issue #39807") + tmpl := New("") _, err := tmpl.New("templ.html").Parse(raceText) if err != nil { @@ -1777,6 +1779,39 @@ func TestRecursiveExecute(t *testing.T) { } } +// recursiveInvoker is for TestRecursiveExecuteViaMethod. +type recursiveInvoker struct { + t *testing.T + tmpl *Template +} + +func (r *recursiveInvoker) Recur() (string, error) { + var sb strings.Builder + if err := r.tmpl.ExecuteTemplate(&sb, "subroutine", nil); err != nil { + r.t.Fatal(err) + } + return sb.String(), nil +} + +func TestRecursiveExecuteViaMethod(t *testing.T) { + tmpl := New("") + top, err := tmpl.New("x.html").Parse(`{{.Recur}}`) + if err != nil { + t.Fatal(err) + } + _, err = tmpl.New("subroutine").Parse(``) + if err != nil { + t.Fatal(err) + } + r := &recursiveInvoker{ + t: t, + tmpl: tmpl, + } + if err := top.Execute(io.Discard, r); err != nil { + t.Fatal(err) + } +} + // Issue 43295. func TestTemplateFuncsAfterClone(t *testing.T) { s := `{{ f . }}` diff --git a/src/html/template/template.go b/src/html/template/template.go index 1ff7e1f7a0..69312d36fd 100644 --- a/src/html/template/template.go +++ b/src/html/template/template.go @@ -11,7 +11,6 @@ import ( "os" "path" "path/filepath" - "reflect" "sync" "text/template" "text/template/parse" @@ -36,20 +35,18 @@ var escapeOK = fmt.Errorf("template escaped correctly") // nameSpace is the data structure shared by all templates in an association. type nameSpace struct { - mu sync.RWMutex + mu sync.Mutex set map[string]*Template escaped bool esc escaper - // The original functions, before wrapping. - funcMap FuncMap } // Templates returns a slice of the templates associated with t, including t // itself. func (t *Template) Templates() []*Template { ns := t.nameSpace - ns.mu.RLock() - defer ns.mu.RUnlock() + ns.mu.Lock() + defer ns.mu.Unlock() // Return a slice so we don't expose the map. m := make([]*Template, 0, len(ns.set)) for _, v := range ns.set { @@ -87,8 +84,8 @@ func (t *Template) checkCanParse() error { if t == nil { return nil } - t.nameSpace.mu.RLock() - defer t.nameSpace.mu.RUnlock() + t.nameSpace.mu.Lock() + defer t.nameSpace.mu.Unlock() if t.nameSpace.escaped { return fmt.Errorf("html/template: cannot Parse after Execute") } @@ -97,16 +94,6 @@ func (t *Template) checkCanParse() error { // escape escapes all associated templates. func (t *Template) escape() error { - t.nameSpace.mu.RLock() - escapeErr := t.escapeErr - t.nameSpace.mu.RUnlock() - if escapeErr != nil { - if escapeErr == escapeOK { - return nil - } - return escapeErr - } - t.nameSpace.mu.Lock() defer t.nameSpace.mu.Unlock() t.nameSpace.escaped = true @@ -134,8 +121,6 @@ func (t *Template) Execute(wr io.Writer, data interface{}) error { if err := t.escape(); err != nil { return err } - t.nameSpace.mu.RLock() - defer t.nameSpace.mu.RUnlock() return t.text.Execute(wr, data) } @@ -151,8 +136,6 @@ func (t *Template) ExecuteTemplate(wr io.Writer, name string, data interface{}) if err != nil { return err } - t.nameSpace.mu.RLock() - defer t.nameSpace.mu.RUnlock() return tmpl.text.Execute(wr, data) } @@ -160,27 +143,13 @@ func (t *Template) ExecuteTemplate(wr io.Writer, name string, data interface{}) // is escaped, or returns an error if it cannot be. It returns the named // template. func (t *Template) lookupAndEscapeTemplate(name string) (tmpl *Template, err error) { - t.nameSpace.mu.RLock() + t.nameSpace.mu.Lock() + defer t.nameSpace.mu.Unlock() + t.nameSpace.escaped = true tmpl = t.set[name] - var escapeErr error - if tmpl != nil { - escapeErr = tmpl.escapeErr - } - t.nameSpace.mu.RUnlock() - if tmpl == nil { return nil, fmt.Errorf("html/template: %q is undefined", name) } - if escapeErr != nil { - if escapeErr != escapeOK { - return nil, escapeErr - } - return tmpl, nil - } - - t.nameSpace.mu.Lock() - defer t.nameSpace.mu.Unlock() - t.nameSpace.escaped = true if tmpl.escapeErr != nil && tmpl.escapeErr != escapeOK { return nil, tmpl.escapeErr } @@ -286,13 +255,6 @@ func (t *Template) Clone() (*Template, error) { } ns := &nameSpace{set: make(map[string]*Template)} ns.esc = makeEscaper(ns) - if t.nameSpace.funcMap != nil { - ns.funcMap = make(FuncMap, len(t.nameSpace.funcMap)) - for name, fn := range t.nameSpace.funcMap { - ns.funcMap[name] = fn - } - } - wrapFuncs(ns, textClone, ns.funcMap) ret := &Template{ nil, textClone, @@ -307,13 +269,12 @@ func (t *Template) Clone() (*Template, error) { return nil, fmt.Errorf("html/template: cannot Clone %q after it has executed", t.Name()) } x.Tree = x.Tree.Copy() - tc := &Template{ + ret.set[name] = &Template{ nil, x, x.Tree, ret.nameSpace, } - ret.set[name] = tc } // Return the template associated with the name of this template. return ret.set[ret.Name()], nil @@ -382,43 +343,10 @@ type FuncMap map[string]interface{} // type. However, it is legal to overwrite elements of the map. The return // value is the template, so calls can be chained. func (t *Template) Funcs(funcMap FuncMap) *Template { - t.nameSpace.mu.Lock() - if t.nameSpace.funcMap == nil { - t.nameSpace.funcMap = make(FuncMap, len(funcMap)) - } - for name, fn := range funcMap { - t.nameSpace.funcMap[name] = fn - } - t.nameSpace.mu.Unlock() - - wrapFuncs(t.nameSpace, t.text, funcMap) + t.text.Funcs(template.FuncMap(funcMap)) return t } -// wrapFuncs records the functions with text/template. We wrap them to -// unlock the nameSpace. See TestRecursiveExecute for a test case. -func wrapFuncs(ns *nameSpace, textTemplate *template.Template, funcMap FuncMap) { - if len(funcMap) == 0 { - return - } - tfuncs := make(template.FuncMap, len(funcMap)) - for name, fn := range funcMap { - fnv := reflect.ValueOf(fn) - wrapper := func(args []reflect.Value) []reflect.Value { - ns.mu.RUnlock() - defer ns.mu.RLock() - if fnv.Type().IsVariadic() { - return fnv.CallSlice(args) - } else { - return fnv.Call(args) - } - } - wrapped := reflect.MakeFunc(fnv.Type(), wrapper) - tfuncs[name] = wrapped.Interface() - } - textTemplate.Funcs(tfuncs) -} - // Delims sets the action delimiters to the specified strings, to be used in // subsequent calls to Parse, ParseFiles, or ParseGlob. Nested template // definitions will inherit the settings. An empty delimiter stands for the @@ -432,8 +360,8 @@ func (t *Template) Delims(left, right string) *Template { // Lookup returns the template with the given name that is associated with t, // or nil if there is no such template. func (t *Template) Lookup(name string) *Template { - t.nameSpace.mu.RLock() - defer t.nameSpace.mu.RUnlock() + t.nameSpace.mu.Lock() + defer t.nameSpace.mu.Unlock() return t.set[name] } -- cgit v1.3