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
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@dvyukov
Copy link
Member

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 commented May 9, 2016

/cc @adg @robpike

@rsc
Copy link
Contributor

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.

@rsc rsc added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 9, 2016
@adg adg self-assigned this May 9, 2016
@adg adg added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 9, 2016
@adg adg modified the milestones: Go1.7, Go1.7Maybe May 9, 2016
@beikege
Copy link

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 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 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 Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels May 10, 2016
@adg
Copy link
Contributor

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 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 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 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 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 as completed May 10, 2016
@robpike
Copy link
Contributor

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 commented May 12, 2016

Sent a CL

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

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

@golang golang locked and limited conversation to collaborators May 12, 2017
@rsc rsc unassigned adg Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants