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

text/template: stack overflow #15618

Closed
dvyukov opened this issue May 9, 2016 · 13 comments
Closed

text/template: stack overflow #15618

dvyukov opened this issue May 9, 2016 · 13 comments
Assignees
Milestone

Comments

@dvyukov
Copy link
Member

@dvyukov dvyukov commented May 9, 2016

go version devel +149ac34 Mon May 9 17:50:29 2016 +0000 linux/amd64

The following program causes stack overflow. Documentation suggests that block calls itself rather than the top-level "" template, so there should be no recursion.

package main

import (
    "text/template"
    "io/ioutil"
)

func main() {
    template.Must(template.New("").Parse(`{{block "".}}{{end}}`)).Execute(ioutil.Discard, nil)
}
runtime: goroutine stack exceeds 1000000000-byte limit
fatal error: stack overflow

runtime stack:
runtime.throw(0x50efd4, 0xe)
    src/runtime/panic.go:566 +0x8b
runtime.newstack()
    src/runtime/stack.go:1035 +0x3ef
runtime.morestack()
    src/runtime/asm_amd64.s:366 +0x7f

goroutine 1 [running]:
text/template.(*state).evalCommand(0xc440100588, 0x0, 0x0, 0x0, 0xc4200ac300, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    src/text/template/exec.go:407 fp=0xc440100298 sp=0xc440100290
text/template.(*state).evalPipeline(0xc440100588, 0x0, 0x0, 0x0, 0xc4200ca0a0, 0x0, 0x0, 0x0)
    src/text/template/exec.go:389 +0xdd fp=0xc440100380 sp=0xc440100298
text/template.(*state).walkTemplate(0xc440100588, 0x0, 0x0, 0x0, 0xc4200c8180)
    src/text/template/exec.go:367 +0xe4 fp=0xc440100428 sp=0xc440100380
text/template.(*state).walk(0xc440100588, 0x0, 0x0, 0x0, 0x58f460, 0xc4200c8180)
    src/text/template/exec.go:232 +0x22f fp=0xc4401004a8 sp=0xc440100428
text/template.(*state).walk(0xc440100588, 0x0, 0x0, 0x0, 0x58f220, 0xc4200ac2d0)
    src/text/template/exec.go:227 +0x130 fp=0xc440100528 sp=0xc4401004a8
text/template.(*state).walkTemplate(0xc440100730, 0x0, 0x0, 0x0, 0xc4200c8180)
    src/text/template/exec.go:372 +0x1e4 fp=0xc4401005d0 sp=0xc440100528
text/template.(*state).walk(0xc440100730, 0x0, 0x0, 0x0, 0x58f460, 0xc4200c8180)
    src/text/template/exec.go:232 +0x22f fp=0xc440100650 sp=0xc4401005d0
text/template.(*state).walk(0xc440100730, 0x0, 0x0, 0x0, 0x58f220, 0xc4200ac2d0)
    src/text/template/exec.go:227 +0x130 fp=0xc4401006d0 sp=0xc440100650
text/template.(*state).walkTemplate(0xc4401008d8, 0x0, 0x0, 0x0, 0xc4200c8180)
    src/text/template/exec.go:372 +0x1e4 fp=0xc440100778 sp=0xc4401006d0
text/template.(*state).walk(0xc4401008d8, 0x0, 0x0, 0x0, 0x58f460, 0xc4200c8180)
    src/text/template/exec.go:232 +0x22f fp=0xc4401007f8 sp=0xc440100778
text/template.(*state).walk(0xc4401008d8, 0x0, 0x0, 0x0, 0x58f220, 0xc4200ac2d0)
    src/text/template/exec.go:227 +0x130 fp=0xc440100878 sp=0xc4401007f8
text/template.(*state).walkTemplate(0xc440100a80, 0x0, 0x0, 0x0, 0xc4200c8180)
    src/text/template/exec.go:372 +0x1e4 fp=0xc440100920 sp=0xc440100878
text/template.(*state).walk(0xc440100a80, 0x0, 0x0, 0x0, 0x58f460, 0xc4200c8180)
    src/text/template/exec.go:232 +0x22f fp=0xc4401009a0 sp=0xc440100920
text/template.(*state).walk(0xc440100a80, 0x0, 0x0, 0x0, 0x58f220, 0xc4200ac2d0)
    src/text/template/exec.go:227 +0x130 fp=0xc440100a20 sp=0xc4401009a0

Found with go-fuzz.

@bradfitz bradfitz added this to the Go1.7Maybe milestone May 9, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 9, 2016

@rsc
Copy link
Contributor

@rsc rsc commented May 9, 2016

The dot in {{block "" .}} seems to be a red herring. Changing the dot to 1 still breaks. Similarly changing either "" to "x" (but not both) avoids the recursion. It's unclear to me whether the recursion is correct.

Leaving investigation for @robpike or @adg.

@adg adg self-assigned this May 9, 2016
@adg adg added NeedsFix and removed NeedsInvestigation labels May 9, 2016
@adg adg modified the milestones: Go1.7, Go1.7Maybe May 9, 2016
@beikege
Copy link

@beikege beikege commented May 10, 2016

go 1.6.2

package main

import (
    "text/template"
    "io/ioutil"
)

func main() {
    template.Must(template.New("a").Parse(`{{template "a"}}`)).Execute(ioutil.Discard, nil)
}
package main

import (
    "text/template"
    "io/ioutil"
)

func main() {
    t := template.New("")
    template.Must(t.New("a").Parse(`{{template "b"}}`))
    template.Must(t.New("b").Parse(`{{template "a"}}`))
    t.ExecuteTemplate(ioutil.Discard, "a", nil)
}
runtime: goroutine stack exceeds 1000000000-byte limit
fatal error: stack overflow
@robpike
Copy link
Contributor

@robpike robpike commented May 10, 2016

These both look like legitimate recursions to me. The true question is whether the recursion should be detected and I'm undecided about that.

@dvyukov
Copy link
Member Author

@dvyukov dvyukov commented May 10, 2016

@robpike Why does {{block "".}}{{end}} call the top-level "" template rather than itself? If it would call itself, there would be no recursion since the block template is empty.

@adg adg modified the milestones: Unplanned, Go1.7 May 10, 2016
@adg adg added NeedsDecision and removed NeedsFix labels May 10, 2016
@adg
Copy link
Contributor

@adg adg commented May 10, 2016

{{block "" .}} is just wrong, so the code should fail somehow but I'm not sure exactly how or when. It's possible that this stack overflow is an acceptable failure mode.

A block defines a template, so we could make it so that it is not possible for a block to re-define the template in which the block appears. But even if that were done, it would still be possible to construct a cycle of nested templates if you try hard enough.

@dvyukov
Copy link
Member Author

@dvyukov dvyukov commented May 10, 2016

For context, go-fuzz constantly hits recursive template invocations. The simplest case is when template "foo" contains {{template "foo"}}. I had to filter out all inputs that contain string "template". It's a user input error, but it manifests as hard program crash. I hoped that I will be at least tests blocks. But now go-fuzz discovered a way to cause recursion using blocks as well.

Overriding an existing template with block sounds good.
But note that it not just overdefines, because it calls the top-level template which looks really bogus.

It is possible to trigger recursion using blocks? E.g. if block "foo" calls block "bar" and vise versa. Or it will fail because bar is not refined yet? If it is not possible to trigger recursion this way then I will be able to test at least blocks.

@adg
Copy link
Contributor

@adg adg commented May 10, 2016

You can create recursion with blocks:

{{define "foo"}}
{{block "bar" .}}{{end}}
{{end}}
{{define "bar"}}
{{block "foo" .}}{{end}}
{{end}}

But you can also trigger recursion with functions:

func foo() { bar() }
func bar() { foo() }

What does go-fuzz do to avoid that? Or has it just not run into it yet?

@dvyukov
Copy link
Member Author

@dvyukov dvyukov commented May 10, 2016

I don't know. Maybe it run into it. I looked at some reports, but I also instantly deleted lots of them.

@adg
Copy link
Contributor

@adg adg commented May 10, 2016

I'm going to close this because I don't think there's anything practical we should do about it. Thanks for the report.

@adg adg closed this May 10, 2016
@robpike
Copy link
Contributor

@robpike robpike commented May 11, 2016

Well, the evaluator could barf if the stack depth is unreasonably large. Maybe that's better than running the machine out of memory.

@adg
Copy link
Contributor

@adg adg commented May 12, 2016

Sent a CL

@adg adg reopened this May 12, 2016
@gopherbot
Copy link

@gopherbot gopherbot commented May 12, 2016

CL https://golang.org/cl/23091 mentions this issue.

@gopherbot gopherbot closed this in eb69476 May 12, 2016
@golang golang locked and limited conversation to collaborators May 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.