Skip to content

cmd/compile: passing method on value receiver causes memory corruption #18410

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.

Sign up for GitHub

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
Closed
dsnet opened this issue Dec 21, 2016 · 12 comments
Closed

cmd/compile: passing method on value receiver causes memory corruption #18410

dsnet opened this issue Dec 21, 2016 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone
Go1.8

Comments

@dsnet
Copy link
Member

dsnet commented Dec 21, 2016 •
edited
Loading

Using go1.7.4, but seems to have been present since go1.3
OS: Linux carbonite 3.13.0-37-generic #64-Ubuntu SMP Mon Sep 22 21:28:38 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

Consider the following code:

type X struct {
	A, B []byte
}

func (x X) Print() {
	fmt.Printf("%p %p\n", x.A, x.B)
}

func caller(f func()) {
	f()
}

func main() {
	caller(X{A: []byte{}}.Print)
}

Currently this prints:

0x516cf0 0xc420064000

I expect this to print:

0x516cf0 0x0

The Print method is declared on a value receiver. In the main function we define a struct literal of type X and explicitly set the A field to be some value, but we expect the B field to be implicitly be zeroed out. The Print method of that struct literal is passed to another function as a callback.

When the callback is called, it shows that the B field is not zeroed and contains a pointer to some arbitrary piece of memory.

\cc @randall77 @dr2chase @paranoiacblack
@dsnet dsnet added this to the Go1.8Maybe milestone Dec 21, 2016
@dsnet dsnet added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 21, 2016
@dsnet
Copy link
Member Author

dsnet commented Dec 21, 2016

Marking as Go1.8maybe for now since this seems significant (discovered in production code). Feel free to mark as Go1.9.

@griesemer
Copy link
Contributor

I don't think we can fix this at this stage of Go1.8. Also, it's not a regression (looks like the bug was there in 1.4). Finally, there's a work-around (provide an explicit closure):

package main

import (
	"fmt"
)

type X struct {
	A, B *int
}

func (t X) Print() {
	fmt.Printf("%p %p\n", t.A, t.B)
}

func caller(f func()) {
	f()
}

func main() {
	caller(X{A: nil}.Print)
	caller(func() { X{A: nil}.Print() })
}

@griesemer griesemer modified the milestones: Go1.9, Go1.8Maybe Dec 21, 2016
@odeke-em
Copy link
Member

odeke-em commented Dec 22, 2016 •
edited
Loading

Hold up a second, on the playground running go1.7.4, the program prints out the right output that @dsnet expects ie https://play.golang.org/p/X6YL_-v-TB therefore I wonder what's really different, perhaps an OS thing?
screen shot 2016-12-22 at 2 59 59 am

but in deed it fails on tip on OSX

$ go run X6YL_-v-TB.go 
0x1128a80 0xc420018360
runtime.Version: devel +0937441 Tue Dec 20 02:44:01 2016 -0800
$ uname -a
Darwin Emmanuels-MBP-2 15.6.0 Darwin Kernel Version 15.6.0: Mon Aug 29 20:21:34 PDT 2016; root:xnu-3248.60.11~1/RELEASE_X86_64 x86_64

screen shot 2016-12-22 at 3 07 08 am

@dgryski
Copy link
Contributor

dgryski commented Dec 22, 2016

@odeke-em That might have more to do with stack layout and the NaCl calling convention.

@minux
Copy link
Member

minux commented Dec 22, 2016 via email

@dgryski
Copy link
Contributor

dgryski commented Dec 22, 2016

@minux When I was playing with this earlier, it's looked like stack allocated objects aren't cleared, but heap allocated ones were.

@dgryski
Copy link
Contributor

dgryski commented Dec 23, 2016

This can be used to overwrite the stack and control the return address:

Code here: https://play.golang.org/p/V37IeKl5pN

<dgryski@kamek[18410] \ʕ◔ϖ◔ʔ/ > gdb ./18410 
GNU gdb (Ubuntu 7.11.90.20161005-0ubuntu1) 7.11.90.20161005-git
Copyright (C) 2016 Free Software Foundation, Inc.
...
Reading symbols from ./18410...done.
(gdb) r
Starting program: /home/dgryski/Dropbox/GITS/gocode/src/github.com/dgryski/18410/18410 
[New LWP 7931]
[New LWP 7932]
[New LWP 7933]
[New LWP 7934]
842350461344 842350461344

Thread 1 "18410" received signal SIGSEGV, Segmentation fault.
runtime.memmove () at /home/dgryski/work/src/cvs/go/src/runtime/memmove_amd64.s:147
147		RET
(gdb) bt 10
#0  runtime.memmove () at /home/dgryski/work/src/cvs/go/src/runtime/memmove_amd64.s:147
#1  0x4141414141414141 in ?? ()
#2  0x4141414141414141 in ?? ()
#3  0x4141414141414141 in ?? ()
#4  0x4141414141414141 in ?? ()
#5  0x4141414141414141 in ?? ()
#6  0x4141414141414141 in ?? ()
#7  0x4141414141414141 in ?? ()
#8  0x4141414141414141 in ?? ()
#9  0x4141414141414141 in ?? ()
(More stack frames follow...)

Playground output:

272801680 272630000
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0xffffffff addr=0x0 pc=0x8c3ac]

goroutine 1 [running]:
panic(0x9b700, 0x1040a038)
	/usr/local/go/src/runtime/panic.go:500 +0x720
main.(X).(main.setB)-fm(0x10440000, 0x40000, 0x40000, 0x0)
	/tmp/sandbox133984929/main.go:21 +0x1a0
main.caller(0x10429f6c, 0x104000f0)
	/tmp/sandbox133984929/main.go:17 +0xc0
main.main()
	/tmp/sandbox133984929/main.go:21 +0xa0

Program exited.

@minux minux added the Security label Dec 23, 2016
@minux
Copy link
Member

minux commented Dec 23, 2016 via email

@dgryski
Copy link
Contributor

dgryski commented Dec 23, 2016

No, but it probably affects appengine which is still on 1.6.3.

@dr2chase
Copy link
Contributor

Problem has (something) to do with treatment of STRUCTLIT under a CALLPART, vs under AS to a target.

.   CALLFUNC-list
.   .   CALLPART l(25) esc(no) tc(1) main.X.Print FUNC-func()
.   .   .   STRUCTLIT l(25) tc(1) main.X
.   .   .   .   TYPE main.X u(1) a(true) l(7) x(0) class(PEXTERN) tc(1) type=main.X main.X main.X
.   .   .   STRUCTLIT-list
.   .   .   .   STRUCTKEY l(25) x(0) main.A
.   .   .   .   .   ADDR l(25) esc(h) tc(1) PTR64-*int
.   .   .   .   .   .   NAME-main.i u(2) a(true) g(1) l(22) x(0) class(PAUTOHEAP) f(1) esc(h) tc(1) addrtaken used(true) int

.   .   .   .   STRUCTKEY l(25) x(16) main.C
.   .   .   .   .   ADDR l(25) esc(h) tc(1) PTR64-*int
.   .   .   .   .   .   NAME-main.j u(2) a(true) g(2) l(22) x(0) class(PAUTOHEAP) f(1) esc(h) tc(1) addrtaken used(true) int
.   .   .   NAME-main.Print u(1) a(true) l(25) x(0)

@gopherbot
Copy link
Contributor

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

@rsc rsc modified the milestones: Go1.8, Go1.9 Jan 6, 2017
@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit that referenced this issue Jan 25, 2017
@dr2chase @bradfitz
[release-branch.go1.7] cmd/compile: rewrite literal.method to ensure …
93468ef 
…full initialization

CALLPART of STRUCTLIT did not check for incomplete initialization
of struct; modify PTRLIT treatment to force zeroing.

Test for structlit, believe this might have also failed for
arraylit.

Fixes #18410.

Change-Id: I511abf8ef850e300996d40568944665714efe1fc
Reviewed-on: https://go-review.googlesource.com/34622
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-on: https://go-review.googlesource.com/35636
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@josharian josharian mentioned this issue Jan 25, 2017
Closed
@dseevr dseevr mentioned this issue Jan 26, 2017
Merged
@golang golang locked and limited conversation to collaborators Jan 24, 2018
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. Security
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
8 participants
@rsc @minux @dgryski @dr2chase @odeke-em @dsnet @griesemer @gopherbot