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

runtime, cmd/compile: deeply nested array interaction leads to fatal error and segfault #29264

Closed
reijin90 opened this issue Dec 14, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@reijin90
Copy link

commented Dec 14, 2018

What version of Go are you using (go version)?

$ go version go1.11.3.windows-amd64 (linux too)

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 env Output
$ go env

What 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:

fatal error: bulkBarrierPreWrite: unaligned arguments

goroutine 1 [running]:
runtime.throw(0x10cc6f, 0x28)
	/usr/local/go/src/runtime/panic.go:608 +0x80 fp=0x4655e0 sp=0x4655c0 pc=0x50fe0
runtime.bulkBarrierPreWrite(0x40cdc0, 0x10730e, 0xc, 0x0)
	/usr/local/go/src/runtime/mbitmap.go:593 +0x5e0 fp=0x465638 sp=0x4655e0 pc=0x346e0
runtime.typedmemmove(0xe9b40, 0x40cdc0, 0x10730e, 0x0)
	/usr/local/go/src/runtime/mbarrier.go:161 +0xe0 fp=0x465658 sp=0x465638 pc=0x32fc0
reflect.typedmemmove(0xe9b40, 0x40cdc0, 0x10730e, 0x0)
	/usr/local/go/src/runtime/mbarrier.go:186 +0x40 fp=0x465670 sp=0x465658 pc=0x33020
reflect.packEface(0xe9b40, 0x10730e, 0x197, 0x0, 0x1, 0x1)
	/usr/local/go/src/reflect/value.go:119 +0xa0 fp=0x465698 sp=0x465670 pc=0xbd300
reflect.valueInterface(0xe9b40, 0x10730e, 0x197, 0x1, 0xc, 0xe9b40)
	/usr/local/go/src/reflect/value.go:1008 +0x100 fp=0x4656c8 sp=0x465698 pc=0xbedc0
reflect.Value.Interface(0xe9b40, 0x10730e, 0x197, 0x0, 0x4, 0xc)
	/usr/local/go/src/reflect/value.go:978 +0x40 fp=0x4656e8 sp=0x4656c8 pc=0xbec60

https://play.golang.org/p/bbI3nbNprvi

In a different scenario it lead to a segfault:

unexpected fault address 0xffffffffffffffff
fatal error: fault
[signal 0xc0000005 code=0x0 addr=0xffffffffffffffff pc=0x4aaf0e]

goroutine 1 [running]:
runtime.throw(0x559e95, 0x5)
	C:/Go/src/runtime/panic.go:608 +0x79 fp=0xc000073158 sp=0xc000073128 pc=0x42bba9
runtime.sigpanic()
	C:/Go/src/runtime/signal_windows.go:207 +0x139 fp=0xc000073188 sp=0xc000073158 pc=0x43c7c9
reflect.Value.Len(0x51fe40, 0x2b202820736d6861, 0x197, 0xc0000528c0)
	C:/Go/src/reflect/value.go:1080 +0x17e fp=0xc0000731b0 sp=0xc000073188 pc=0x4aaf0e
makeSliceWriter.func1(0x51fe40, 0x2b202820736d6861, 0x197, 0xc00008c000, 0x0, 0x0)
	C:/my/code/encode.go:516 +0x6b fp=0xc000073210 sp=0xc0000731b0 pc=0x4fef5b
makeSliceWriter.func1(0x51fe40, 0x5599dd, 0x197, 0xc00008c000, 0x0, 0x0)
	C:/my/code/encode.go:518 +0xf1 fp=0xc000073270 sp=0xc000073210 pc=0x4fefe1
makeSliceWriter.func1(0x51fe40, 0xc00003e560, 0x197, 0xc00008c000, 0x0, 0x0)
	C:/my/code/encode.go:518 +0xf1 fp=0xc0000732d0 sp=0xc000073270 pc=0x4fefe1
makeSliceWriter.func1(0x51fe40, 0xc000049140, 0x197, 0xc00008c000, 0x0, 0x0)
	C:/my/code/encode.go:518 +0xf1 fp=0xc000073330 sp=0xc0000732d0 pc=0x4fefe1
makeSliceWriter.func1(0x51fe40, 0xc000049120, 0x197, 0xc00008c000, 0x0, 0x0)
	C:/my/code/encode.go:518 +0xf1 fp=0xc000073390 sp=0xc000073330 pc=0x4fefe1

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?

@bradfitz bradfitz changed the title Deeply nested array interaction leads to fatal error and segfault runtime, cmd/compile: deeply nested array interaction leads to fatal error and segfault Dec 14, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

@bradfitz bradfitz added the NeedsFix label Dec 14, 2018

@bradfitz bradfitz added this to the Go1.12 milestone Dec 14, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

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.

@reijin90

This comment has been minimized.

Copy link
Author

commented Dec 14, 2018

Btw: due to the nature of the bug I had issues using a debugger to figure out more.
I'd be more than willing to do more on this, as this is a byproduct of my thesis and seems promising to actually be exploitable somehow (even if it's just a DoS). Let me know if I can be of any help :)

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

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

        if depth > 100 {
                return "<...>"
        }

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.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

Change the 100 to 200, it works. Not sure what we should do...

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

@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.

@griesemer griesemer self-assigned this Dec 14, 2018

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

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.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

The compiler rejecting too-deep types and exiting gracefully SGTM.

aarzilli added a commit to aarzilli/delve that referenced this issue Dec 17, 2018

dwarf/godwarf: tolerate cyclical type DIEs
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
@gopherbot

This comment has been minimized.

Copy link

commented Dec 17, 2018

Change https://golang.org/cl/154583 mentions this issue: cmd/compile: increase nesting depth limit for type descriptors

@gopherbot gopherbot closed this in 99e4ddd Dec 18, 2018

derekparker added a commit to go-delve/delve that referenced this issue Jan 4, 2019

dwarf/godwarf: tolerate cyclical type DIEs
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.