Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

html/template: data race in Execute #39807

Open
bep opened this issue Jun 24, 2020 · 27 comments · May be fixed by #41638
Open

html/template: data race in Execute #39807

bep opened this issue Jun 24, 2020 · 27 comments · May be fixed by #41638
Labels
Milestone

Comments

@bep
Copy link
Contributor

@bep bep commented Jun 24, 2020

go version go1.14.3 darwin/amd64

Building the below program with go build -race and then running it fails:

package main

import (
	"fmt"
	"html/template"
	"io/ioutil"
	"log"
	"sync"
)

func main() {
	var wg sync.WaitGroup

	jsTempl := `
{{- define "jstempl" -}}
var foo = "bar";
{{- end -}}
<script type="application/javascript">
{{ template "jstempl" $ }}
</script>
`

	tpl := template.New("")
	_, err := tpl.New("templ.html").Parse(jsTempl)
	if err != nil {
		log.Fatal(err)
	}

	const numTemplates = 20

	for i := 0; i < numTemplates; i++ {
		_, err = tpl.New(fmt.Sprintf("main%d.html", i)).Parse(`{{ template "templ.html" . }}`)
		if err != nil {
			log.Fatal(err)
		}
	}

	for i := 1; i <= 10; i++ {
		wg.Add(1)
		go func() {
			defer wg.Done()
			for j := 0; j < numTemplates; j++ {
				templ := tpl.Lookup(fmt.Sprintf("main%d.html", j))
				if err := templ.Execute(ioutil.Discard, nil); err != nil {
					log.Fatal(err)
				}

			}
		}()
	}

	wg.Wait()
}
WARNING: DATA RACE

WARNING: DATA RACE
Write at 0x00c00011c540 by goroutine 32:
  runtime.mapassign_faststr()
      /Users/bep/dev/go/dump/go/src/runtime/map_faststr.go:202 +0x0
  text/template.(*Template).associate()
      /Users/bep/dev/go/dump/go/src/text/template/template.go:227 +0x1a8
  text/template.(*Template).AddParseTree()
      /Users/bep/dev/go/dump/go/src/text/template/template.go:133 +0x2cc
  html/template.(*escaper).commit()
      /Users/bep/dev/go/dump/go/src/html/template/escape.go:810 +0x2c5
  html/template.escapeTemplate()
      /Users/bep/dev/go/dump/go/src/html/template/escape.go:38 +0x324
  html/template.(*Template).escape()
      /Users/bep/dev/go/dump/go/src/html/template/template.go:102 +0x349
  html/template.(*Template).Execute()
      /Users/bep/dev/go/dump/go/src/html/template/template.go:119 +0x3c
  main.main.func1()
      /Users/bep/dev/go/bep/temp/main.go:46 +0x177

Previous read at 0x00c00011c540 by goroutine 28:
runtime.mapaccess1_faststr()
/Users/bep/dev/go/dump/go/src/runtime/map_faststr.go:12 +0x0
text/template.(*state).walkTemplate()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:399 +0x13b
text/template.(*state).walk()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:268 +0x2be
text/template.(*state).walk()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:263 +0x195
text/template.(*Template).execute()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:220 +0x2fb
text/template.(*Template).Execute()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:203 +0xd2
html/template.(*Template).Execute()
/Users/bep/dev/go/dump/go/src/html/template/template.go:122 +0x7f
main.main.func1()
/Users/bep/dev/go/bep/temp/main.go:46 +0x177

Goroutine 32 (running) created at:
main.main()
/Users/bep/dev/go/bep/temp/main.go:42 +0x2de

Goroutine 28 (running) created at:
main.main()
/Users/bep/dev/go/bep/temp/main.go:42 +0x2de

==================
WARNING: DATA RACE
Write at 0x00c0000ac0b0 by goroutine 32:
text/template.(*Template).associate()
/Users/bep/dev/go/dump/go/src/text/template/template.go:227 +0x1bd
text/template.(*Template).AddParseTree()
/Users/bep/dev/go/dump/go/src/text/template/template.go:133 +0x2cc
html/template.(*escaper).commit()
/Users/bep/dev/go/dump/go/src/html/template/escape.go:810 +0x2c5
html/template.escapeTemplate()
/Users/bep/dev/go/dump/go/src/html/template/escape.go:38 +0x324
html/template.(*Template).escape()
/Users/bep/dev/go/dump/go/src/html/template/template.go:102 +0x349
html/template.(*Template).Execute()
/Users/bep/dev/go/dump/go/src/html/template/template.go:119 +0x3c
main.main.func1()
/Users/bep/dev/go/bep/temp/main.go:46 +0x177

Previous read at 0x00c0000ac0b0 by goroutine 28:
text/template.(*state).walkTemplate()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:399 +0x14e
text/template.(*state).walk()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:268 +0x2be
text/template.(*state).walk()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:263 +0x195
text/template.(*state).walkTemplate()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:413 +0x3cd
text/template.(*state).walk()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:268 +0x2be
text/template.(*state).walk()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:263 +0x195
text/template.(*Template).execute()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:220 +0x2fb
text/template.(*Template).Execute()
/Users/bep/dev/go/dump/go/src/text/template/exec.go:203 +0xd2
html/template.(*Template).Execute()
/Users/bep/dev/go/dump/go/src/html/template/template.go:122 +0x7f
main.main.func1()
/Users/bep/dev/go/bep/temp/main.go:46 +0x177

Goroutine 32 (running) created at:
main.main()
/Users/bep/dev/go/bep/temp/main.go:42 +0x2de

Goroutine 28 (running) created at:
main.main()
/Users/bep/dev/go/bep/temp/main.go:42 +0x2de

Found 2 data race(s)


@davecheney
Copy link
Contributor

@davecheney davecheney commented Jun 24, 2020

@bep thanks for the issue. Are you able to check Go 1.13 and see if it is affected? (This will determine if this is a blocker for 1.15 or not)

/cc @dmitshur

@bep
Copy link
Contributor Author

@bep bep commented Jun 24, 2020

It also fails on go1.13.12.

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Jun 24, 2020

/cc @empijei

@cagedmantis cagedmantis added this to the Backlog milestone Jun 24, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Jun 24, 2020

@empijei
Copy link
Contributor

@empijei empijei commented Jun 26, 2020

Some additional info:

  • This does not affect "text/template"
  • This also affects ExecuteTemplate so it is not just an issue with the lookup+execute calls.
  • This does not happen if the templates are executed at least once before the concurrent execution. This is probably due to a lazy computation since the race happens in a escapeTemplate call, which is only executed once per template.
  • My best guess is that the computation is lazy because it involves calling nested templates but I'd need to take a better look at this. The escape should be happening while holding a lock or should be executed at parse time if possible.

As a quick workaround running the templates once before they are run concurrently addresses the issue (see code below), but we need to properly fix this.

Thanks for reporting.

package main

import (
	"fmt"
	"html/template"
	"io/ioutil"
	"log"
	"sync"
)

func main() {
	var wg sync.WaitGroup

	jsTempl := `
{{- define "jstempl" -}}
var foo = "bar";
{{- end -}}
<script type="application/javascript">
{{ template "jstempl" $ }}
</script>
`

	tpl := template.New("")
	_, err := tpl.New("templ.html").Parse(jsTempl)
	if err != nil {
		log.Fatal(err)
	}

	const numTemplates = 20

	for i := 0; i < numTemplates; i++ {
		_, err = tpl.New(fmt.Sprintf("main%d.html", i)).Parse(`{{ template "templ.html" . }}`)
		if err != nil {
			log.Fatal(err)
		}
	}

        // Temporary workaround to prevent the race.
	for j := 0; j < numTemplates; j++ {
		if err := tpl.ExecuteTemplate(ioutil.Discard, fmt.Sprintf("main%d.html", j), nil); err != nil {
			log.Fatal(err)
		}
	}

	for i := 1; i <= 10; i++ {
		wg.Add(1)
		go func() {
			defer wg.Done()
			for j := 0; j < numTemplates; j++ {
				if err := tpl.ExecuteTemplate(ioutil.Discard, fmt.Sprintf("main%d.html", j), nil); err != nil {
					log.Fatal(err)
				}
			}
		}()
	}

	wg.Wait()
}
@andreasf
Copy link

@andreasf andreasf commented Jul 23, 2020

This appears to be the same data race causing gohugoio/hugo#7293, where it is triggered from text/template.

Hugo have a temporary fork of text/template included in their codebase. I attempted to fix the data race in the fork: gohugoio/hugo#7507. They'd rather have it upstreamed though.

andreasf added a commit to andreasf/go that referenced this issue Jul 23, 2020
@andreasf
Copy link

@andreasf andreasf commented Jul 23, 2020

I pulled the patch out of Hugo's fork of text/template and changed it a bit:

  • Separate lock for tmpl instead of reusing muFunc—not sure if that's best
  • Use lock everywhere tmpl is accessed

Personally, I'd prefer a single lock as it's simpler and safer. What do you think, should I file a PR?

@empijei
Copy link
Contributor

@empijei empijei commented Aug 8, 2020

Do you have a proposal to fix the race described in this bug report? If so please do send a CL my way 😄

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 26, 2020

Change https://golang.org/cl/257817 mentions this issue: text/template: add lock for Template.tmpl to fix data race

andreasf added a commit to andreasf/go that referenced this issue Sep 26, 2020
This adds a new lock protecting "tmpl".

Thanks to @bep for providing the test case.

Fixes golang#39807
andreasf added a commit to andreasf/go that referenced this issue Sep 28, 2020
This adds a new lock protecting "tmpl".

Thanks to @bep for providing the test case.

Fixes golang#39807
andreasf added a commit to andreasf/go that referenced this issue Sep 28, 2020
This adds a new lock protecting "tmpl".

Thanks to @bep for providing the test case.

Fixes golang#39807
andreasf added a commit to andreasf/go that referenced this issue Sep 29, 2020
This adds a new lock protecting "tmpl".

Fixes golang#39807
andreasf added a commit to andreasf/go that referenced this issue Sep 29, 2020
This adds a new lock protecting "tmpl".

Fixes golang#39807
@rsc
Copy link
Contributor

@rsc rsc commented Oct 6, 2020

It looks to me like html/template's nameSpace.mu needs to be an RWLock and must be RLock'ed during t.text.Execute.
t.escape will need to RLock to decide if escaping is necessary and then RUnlock/Lock if so.

@bep
Copy link
Contributor Author

@bep bep commented Dec 2, 2020

I guess this will not get fixed in Go 1.16?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 3, 2020

The problem in text/template seems different to me. That seems to require calling AddParseTree concurrently, but that method is not documented as supporting concurrent calls. Maybe we should change that, but it doesn't seem related to this problem.

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 3, 2020

Change https://golang.org/cl/274450 mentions this issue: html/template: avoid race when escaping updates template

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 11, 2020

See previously #2439, which was apparently the same problem but regressed due to the lack of a regression test (which is understandable given that the race detector debuted over a year later).

@bep
Copy link
Contributor Author

@bep bep commented Dec 12, 2020

@bcmills sure, but you (and with you I don't mean specifically you) cannot at the sam time say that data races are always a serious bug which need fixing and then brush this on to a future release when there is a working fix in place.

This whole situation means that I need to somehow maintain a fix for this in a fork, which is not something I do with joy and certainly not something I get paid to do.

/cc @spf13

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 13, 2020

@bep

you (and with you I don't mean specifically you) cannot at the sam time say that data races are always a serious bug which need fixing and then brush this on to a future release when there is a working fix in place.

I understand that the length of time this has been open is frustrating, but I don't see anybody saying this.

Are you able to check whether https://golang.org/cl/274450 fixes the problems that you are seeing?

@dmitshur dmitshur modified the milestones: Backlog, Go1.16 Dec 17, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 21, 2020

Change https://golang.org/cl/279492 mentions this issue: html/template: attach wrapped functions to namespace

gopherbot pushed a commit that referenced this issue Jan 7, 2021
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 <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>
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 22, 2021

I think we're going to have to roll back the fix for this. I don't see a way to avoid #43855.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 22, 2021

Change https://golang.org/cl/285957 mentions this issue: html/template: revert "avoid race when escaping updates template"

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 25, 2021

The fix was rolled back, so reopening this issue.

gopherbot pushed a commit that referenced this issue Jan 25, 2021
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>
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 25, 2021

The basic issue here is that html/template expects to be able to call the text/template Template methods Execute and AddParseTree concurrently. We can in principle fix this by adding locking to html/template, but it's hard because user-defined template functions, including calls to data type methods, can themselves call Execute and, therefore, AddParseTree.

My attempt to fix this, following the suggestion at #39807 (comment), failed because nothing released the read lock held during a method call, so if the method call called Execute, it led to a deadlock.

Given html/template's needs, I think we kind of do have to fix this in text/template, along the lines of https://golang.org/cl/257817. It's certainly an abstraction violation to change text/template to fix an html/template problem. But given that html/template needs to concurrently call Execute and AddParseTree, I don't yet see a different workable approach.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.16, Go1.17 Jan 25, 2021
@bcmills
Copy link
Member

@bcmills bcmills commented Jan 25, 2021

Following my comment in https://go-review.googlesource.com/c/go/+/274450/4/src/html/template/template.go#138, I think it is possible to fix this race by escaping all of the templates at the first call to Execute, guarded by a sync.Once. But an attempt to do that apparently causes TestAddParseTreeHTML to fail.

I suspect that the failure observed in TestAddParseTreeHTML could be addressed by escaping the templates in topological (instead of arbitrary) order.

However, I suspect that that failure indicates a fundamental inconsistency in the API: if the template b cannot be escaped outside the context of its invocation within template a, doesn't that imply that the same template might need to be escaped multiple times, in different ways depending on the context? But the map data structures in the html/template package do not appear to include that context as part of the map key — so either the failure to escape the template in the test is spurious, or a different test ought to reveal a condition in which the nested template is mis-escaped.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 25, 2021

I don't think there is any inconsistency in the API. When a top-level template is escaped, all the {{template NAME}} invocations are rewritten to invoke a template escaped in the proper context. That escaping is indeed context-dependent, the escaped template is given a context-dependent name (see the mangle method in context.go), and the escaped template is not marked in a way that is seen if the template is invoked at the top level or in some other context.

A topological sort might work, if we don't care about the extra work.

@bep
Copy link
Contributor Author

@bep bep commented Jan 30, 2021

@ianlancetaylor if a building is about to tip over, you put some support pillars in place. I reviewed a perfectly fine CR (https://golang.org/cl/257817) that would have made a great pillar until you could get a proper foundation in place. Would you have released Go 1.16 with a similar bug in the net/http package?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 31, 2021

If the net/http bug had existed for several releases, and if it were this late in the release cycle, then, yes, I would not have blocked Go 1.16 from being released with such a bug.

@bep
Copy link
Contributor Author

@bep bep commented Feb 13, 2021

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 13, 2021

@bep I'm sorry you are upset about this issue. I understand that it is frustrating. It's frustrating to me too. I've probably put 40 hours into fixing this issue, and so far I have failed. That's a lot of time for me.

That said, the comment you mention is different. I'm not blocking 1.16 because of that bug. And, to use your metaphor, that CL is not a case of putting in support pillars until there is a proper foundation. That CL is, I believe, the correct fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

12 participants