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

cmd/compile: value assignment to pointer converted from unsafe pointer exceeds structure boundary on 32-bit arm #56979

Closed
database64128 opened this issue Nov 29, 2022 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge

Comments

@database64128
Copy link
Contributor

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

$ go version
go version go1.19.3 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/database64128/.cache/go-build"
GOENV="/home/database64128/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/database64128/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/database64128/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.3"
GCCGO="gccgo"
GOAMD64="v3"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1941230580=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Full minimal reproduction code: https://go.dev/play/p/r2c8IBscDxV (Can only be reproduced on 32-bit ARM)

Most relevant snippet:

type addressFamily byte

const (
	addressFamilyNone addressFamily = iota
	addressFamilyNetip
	addressFamilyDomain
)

type netipAddrHeader struct {
	hi uint64
	lo uint64
	z  unsafe.Pointer
}

type Addr struct {
	_    [0]func()
	addr netipAddrHeader
	port uint16
	af   addressFamily
}

// AddrFromIPPort returns an Addr from the provided netip.AddrPort.
func AddrFromIPPort(addrPort netip.AddrPort) Addr {
	addr := Addr{af: addressFamilyNetip}
	*(*netip.AddrPort)(unsafe.Pointer(&addr)) = addrPort
	return addr
}

Sorry that this example is a bit complex. I don't have 32-bit ARM devices to test it, so I'm sticking to the original code that can absolutely trigger the problem.

What did you expect to see?

The AddrFromIPPort function works on all platforms.

What did you see instead?

The function works fine on amd64 and arm64. On 32-bit ARM, *(*netip.AddrPort)(unsafe.Pointer(&addr)) = addrPort overwrites the next byte af and causes the returned addr to have af set to 0.

The above code snippet was taken out from https://github.com/database64128/shadowsocks-go/blob/main/conn/addr.go. A user reported the issue (database64128/shadowsocks-go#30) and confirmed that my fix (database64128/shadowsocks-go@ea642fe) works:

---
 conn/addr.go | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/conn/addr.go b/conn/addr.go
index 7e0abd6..9003e6c 100644
--- a/conn/addr.go
+++ b/conn/addr.go
@@ -222,10 +222,10 @@ func (a *Addr) UnmarshalText(text []byte) error {
 }
 
 // AddrFromIPPort returns an Addr from the provided netip.AddrPort.
-func AddrFromIPPort(addrPort netip.AddrPort) Addr {
-	addr := Addr{af: addressFamilyNetip}
+func AddrFromIPPort(addrPort netip.AddrPort) (addr Addr) {
 	*(*netip.AddrPort)(unsafe.Pointer(&addr)) = addrPort
-	return addr
+	addr.af = addressFamilyNetip
+	return
 }
 
 // AddrFromDomainPort returns an Addr from the provided domain name and port number.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 29, 2022
@randall77
Copy link
Contributor

This is unsupported behavior and is not expected to work.
A netip.AddrPort is 16 bytes on 32-bit platforms. (12 for a netip.Addr, 2 for a uint16, 2 for padding.) The offset of af in your Addr is byte 14.
I'm not sure what the ultimate goal is here, but you'll need to find another way. Possibly duplicate the padding (add a _ uint16 after port in your Addr). But the real fix would be to not do unsafe stuff here. Why do you need to build your own data structures instead of just using netip.AddrPort/netip.Addr directly?

@cuonglm
Copy link
Member

cuonglm commented Nov 29, 2022

@randall77 isn't the size is 24 on 32-bit platforms? 20 for netip.Addr (2 uint64 + 1 pointer), 2 for a uint16, 2 for padding.

@database64128 your conversion violates the 1st rule of unsafe.Pointer conversion https://pkg.go.dev/unsafe#Pointer

Provided that T2 is no larger than T1 and that the two share an equivalent memory layout, this conversion allows reinterpreting data of one type as data of another type

Your Addr and netip.AddrPort have the same size, but do not have the same memory layout.

@randall77
Copy link
Contributor

@cuonglm Yeah sorry, I can't do math.

@database64128
Copy link
Contributor Author

database64128 commented Nov 29, 2022

Why do you need to build your own data structures instead of just using netip.AddrPort/netip.Addr directly?

This custom address type is used in places where the address is either a domain name or an IP. It used to consist of a netip.AddrPort and a string and take up 6 words on 64-bit platforms. I recently made the change to remove the string field and reuse the same space to store either a domain name or an IP. This reduced the size down to 4 words. The trick was documented in the comments of the type:

// For space efficiency, the IP address and the domain string share the same space.
// The [netip.Addr] is stored in its original layout.
// The domain string's data pointer is stored in the ip.z field.
// Its length is stored at the beginning of the structure.
// This is essentially an unsafe "enum".

your conversion violates the 1st rule of unsafe.Pointer conversion

the two share an equivalent memory layout

It's not clear to me what "equivalent" means. I think this needs to be clarified. And I still don't see how the memory layout is different. Can you elaborate on that?

Here's the memory layout of netip.AddrPort:

type uint128 struct {
	hi uint64
	lo uint64
}

type Addr struct {
	addr uint128
	z    *intern.Value
}

type AddrPort struct {
	ip   Addr
	port uint16
}

If you change my Addr type from:

type Addr struct {
	_    [0]func()
	addr netipAddrHeader
	port uint16
	af   addressFamily
}

to:

type Addr struct {
	_  [0]func()
	ap netip.AddrPort
	af addressFamily
}

It still has the same memory layout. And a pointer to an Addr value is equivalent to a pointer to its ap field.

@randall77
Copy link
Contributor

It does not have the same memory layout. The padding is different.
A netip.AddrPort is, flattened, {uint64, uint64, pointer, uint16, 2 bytes of padding }
So your first Addr is {uint64, uint64, pointer, uint16, uint8, 1 byte of padding }
The second Addr is {uint64, uint64, pointer, uint16, 2 bytes of padding, uint8, 3 byte of padding }

@ianlancetaylor
Copy link
Contributor

Closing because this is not a bug in Go.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Nov 29, 2022
@database64128
Copy link
Contributor Author

Thanks. I understand that this is undefined behavior. But still, I feel like this doesn't have to be like this way. The unaligned af byte won't get overwritten on amd64 and arm64. Is there any particular reason for 32-bit arm to not behave like its 64-bit counterparts?

@randall77
Copy link
Contributor

The same behavior will happen on 64 bit for larger structs (add 2 more uint64 fields at the start).
The difference is that copies are done differently for "small" structs and "large" structs, and the structs involved are small on 64 bit but large on 32 bit (because a uint64 field has cost 1 on 64 bit but cost 2 on 32 bit).
"small" structs are copied field by field. "large" structs are copied with memcpy.

@golang golang locked and limited conversation to collaborators Nov 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

5 participants