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

Jump to bottom

runtime: fmt.Print non-empty go:notinheap values crashes in runtime.typedmemmove #48399

Closed
eliasnaur opened this issue Sep 15, 2021 · 5 comments
Closed

runtime: fmt.Print non-empty go:notinheap values crashes in runtime.typedmemmove #48399

eliasnaur opened this issue Sep 15, 2021 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.18

Comments

@eliasnaur
Copy link
Contributor

See https://play.golang.org/p/N-IOr9IXNa-, found while investigating #42018. Note that printing the empty S1 type works, whereas the non-empty S2 crashes:

unexpected fault address 0xdead
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x1 addr=0xdead pc=0x45b709]

goroutine 1 [running]:
runtime.throw({0x493a6e, 0xc00009a908})
	/usr/local/go-faketime/src/runtime/panic.go:1198 +0x71 fp=0xc00009a8a8 sp=0xc00009a878 pc=0x42f771
runtime.sigpanic()
	/usr/local/go-faketime/src/runtime/signal_unix.go:742 +0x2f6 fp=0xc00009a8f8 sp=0xc00009a8a8 pc=0x442fd6
runtime.memmove()
	/usr/local/go-faketime/src/runtime/memmove_amd64.s:181 +0x129 fp=0xc00009a900 sp=0xc00009a8f8 pc=0x45b709
runtime.typedmemmove(0x4890a0, 0xc00010a018, 0xdead)
	/usr/local/go-faketime/src/runtime/mbarrier.go:171 +0x65 fp=0xc00009a938 sp=0xc00009a900 pc=0x4102e5
reflect.typedmemmove(0xc00009a978, 0x203000, 0x400)
	/usr/local/go-faketime/src/runtime/mbarrier.go:187 +0x19 fp=0xc00009a960 sp=0xc00009a938 pc=0x4563b9
reflect.packEface({0x4890a0, 0xdead, 0x7fbf031fffff})
	/usr/local/go-faketime/src/reflect/value.go:123 +0x65 fp=0xc00009a9b0 sp=0xc00009a960 pc=0x46d925
reflect.valueInterface({0x4890a0, 0xdead, 0x43a260}, 0x2)
	/usr/local/go-faketime/src/reflect/value.go:1381 +0xbc fp=0xc00009a9f0 sp=0xc00009a9b0 pc=0x46f4bc
reflect.Value.Interface(...)
	/usr/local/go-faketime/src/reflect/value.go:1351
fmt.(*pp).printValue(0xc00010e000, {0x4890a0, 0xdead, 0x45872e}, 0x76, 0x1)
	/usr/local/go-faketime/src/fmt/print.go:722 +0xa5 fp=0xc00009abd8 sp=0xc00009a9f0 pc=0x47bd65
fmt.(*pp).printValue(0xc00010e000, {0x482440, 0xc00010a010, 0xc8}, 0x76, 0x0)
	/usr/local/go-faketime/src/fmt/print.go:876 +0xce5 fp=0xc00009adc0 sp=0xc00009abd8 pc=0x47c9a5
fmt.(*pp).printArg(0xc00010e000, {0x482440, 0xc00010a010}, 0x76)
	/usr/local/go-faketime/src/fmt/print.go:712 +0x74c fp=0xc00009ae60 sp=0xc00009adc0 pc=0x47bc2c
fmt.(*pp).doPrintln(0xc00010e000, {0xc00009af50, 0x1, 0xc00009af10})
	/usr/local/go-faketime/src/fmt/print.go:1169 +0x149 fp=0xc00009aed0 sp=0xc00009ae60 pc=0x47dc49
fmt.Fprintln({0x4b0560, 0xc000100008}, {0xc00009af50, 0x1, 0x1})
	/usr/local/go-faketime/src/fmt/print.go:264 +0x4f fp=0xc00009af20 sp=0xc00009aed0 pc=0x478b8f
fmt.Println(...)
	/usr/local/go-faketime/src/fmt/print.go:274
main.main()
	/tmp/sandbox2459380511/prog.go:18 +0x9f fp=0xc00009af80 sp=0xc00009af20 pc=0x47df3f
runtime.main()
	/usr/local/go-faketime/src/runtime/proc.go:255 +0x213 fp=0xc00009afe0 sp=0xc00009af80 pc=0x431db3
runtime.goexit()
	/usr/local/go-faketime/src/runtime/asm_amd64.s:1581 +0x1 fp=0xc00009afe8 sp=0xc00009afe0 pc=0x45a7c1

This happes with go tip as well.

CC @randall77
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 15, 2021
@randall77
Copy link
Contributor

fmt calls reflect.Value.Elem on a Value that has a notinheap pointer in it.
I think we just need to panic in Elem if we're trying to load a notinheap pointer. Otherwise, we'd put a bad pointer in an interface like is happening here.

@randall77 randall77 self-assigned this Sep 15, 2021
@randall77 randall77 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 Sep 15, 2021
@randall77 randall77 added this to the Go1.18 milestone Sep 15, 2021
@gopherbot
Copy link

Change https://golang.org/cl/350153 mentions this issue: reflect: make Elem panic if trying to dereference a notinheap pointer

@randall77
Copy link
Contributor

I've updated that CL, and as part of that update, it doesn't actually fix this issue anymore. It fixes a more subtle issue regarding what kind of pointers are allowed in the data field of an interface.

I'm not sure there's anything to fix here. fmt is trying to make a copy of the value you passed to it, and that copy fails because the pointer is invalid.
If the copy didn't fail, the dereference required to load the field value surely would.

gopherbot pushed a commit that referenced this issue Oct 15, 2021
@randall77
reflect: make Elem panic on bad notinheap pointers
8331f25 
This CL fixes the subtle issue that Elem can promote a
not-in-heap pointer, which could be any bit pattern, into an
unsafe.Pointer, which the garbage collector can see. If that
resulting value is bad, it can crash the GC.

Make sure that we don't introduce bad pointers that way. We can
make Elem() panic, because any such bad pointers are in the Go heap,
and not-in-heap pointers are not allowed to point into the Go heap.

Update #48399

Change-Id: Ieaf35a611b16b4dfb5e907e229ed4a2aed30e18c
Reviewed-on: https://go-review.googlesource.com/c/go/+/350153
Trust: Keith Randall <khr@golang.org>
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@mknyszek
Copy link
Contributor

@randall77 Anything else to do here? Based on your last comment, it seems like this should be closed?

@randall77
Copy link
Contributor

Yes, I'll close.

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
5 participants
@eliasnaur @mknyszek @randall77 @gopherbot @seankhliao