runtime, cmd/compile: deeply nested array interaction leads to fatal error and segfault #29264
Comments
bradfitz
changed the title
|
Fun. Nice find. /cc @randall77 @aclements @josharian @ianlancetaylor @griesemer @dr2chase @cherrymui |
bradfitz
added
the
NeedsFix
|
Works in Go 1.8, Go 1.9, Go 1.10. Fails in Go 1.11 and Go tip. So this could use a backport to Go 1.11 once fixed at tip. |
|
Btw: due to the nature of the bug I had issues using a debugger to figure out more. |
|
Apparently the compiler gives up at depth > 100 when creating the type symbol? So we end up using the wrong type descriptor? https://go.googlesource.com/go/+/master/src/cmd/compile/internal/gc/fmt.go#1752 From the stack trace, all nested printValue's value's type pointer is 0xe9b40, which should not happen, unless the compiler creates a self-referential type. |
|
Change the 100 to 200, it works. Not sure what we should do... |
|
@cherrymui That limit (100) is only to avoid endless recursions (cyclic types) due to the lack of a better mechanism (the fmt.go file in the compiler needs to be thrown out at some point...). Increasing the value to 200 is fine but of course doesn't solve the problem. Interesting that it worked in the past. In any case, the compiler shouldn't crash but gracefully exit. |
|
I agree that changing 100 to 200 doesn't solve the problem. The depth limit is introduced in CL https://go-review.googlesource.com/c/go/+/38797. Before it had a different way of recursion detection, by putting a marker on the type. The compiler doesn't crash. It mis-compiles the code and the program crashes at run time, due to incorrect type descriptor. |
|
The compiler rejecting too-deep types and exiting gracefully SGTM. |
As specified in line dwarf/godwarf/type.go:507 the typeCache entry should always be set before recursive calls to readType to avoid infite recursion. Most code in readType already does this but some of the code added later to handle Go types was wrong. Fix this bug and also fix the String and Size methods of Type so that they handle recursive types "correctly" (i.e. they don't recur forever). No test is added for this since all legitimate uses of cyclical types were already handled correctly and the malformed types emitted by the go compiler will probably be removed in 1.12. See: golang/go#29264 Fixes go-delve#1444
|
Change https://golang.org/cl/154583 mentions this issue: |
As specified in line dwarf/godwarf/type.go:507 the typeCache entry should always be set before recursive calls to readType to avoid infite recursion. Most code in readType already does this but some of the code added later to handle Go types was wrong. Fix this bug and also fix the String and Size methods of Type so that they handle recursive types "correctly" (i.e. they don't recur forever). No test is added for this since all legitimate uses of cyclical types were already handled correctly and the malformed types emitted by the go compiler will probably be removed in 1.12. See: golang/go#29264 Fixes #1444
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?Reproduceable on Win10, Ubuntu 18 and playground
go envOutputWhat did you do?
Manually created a deeply nested array (exactly 102 layers) for security testing an encoding function. Interacting with the nested array leads to several issues related to reflection:
https://play.golang.org/p/bbI3nbNprvi
In a different scenario it lead to a segfault:
I'm not sure how or if this could be exploited as it is probably an edgecase. Sadly, I'm far from a golang pro and don't know how to investigate further or if this is even "intended" behaviour. Maybe someone can shed a bit more light on what exactly the issue is?
The text was updated successfully, but these errors were encountered: