]> Cypherpunks repositories - gostls13.git/commitdiff
html/template: revert "avoid race when escaping updates template"
authorIan Lance Taylor <iant@golang.org>
Fri, 22 Jan 2021 22:54:23 +0000 (14:54 -0800)
committerIan Lance Taylor <iant@golang.org>
Mon, 25 Jan 2021 20:41:05 +0000 (20:41 +0000)
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 <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
src/html/template/exec_test.go
src/html/template/template.go

index cd6b78a1a90fdd7fd185fbb1b330cb266696d19b..7d1bef1782a9cc0be2a97179acb080a5ab7afad3 100644 (file)
@@ -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(`<a href="/x?p={{"'a<b'"}}">`)
+       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 . }}`
index 1ff7e1f7a04413010cfae40396b887bfae96ddb4..69312d36fdb361ec73b66a712840f902e778a962 100644 (file)
@@ -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]
 }