cmd/compile: compiler assumes `[2]T` does not alias with `struct{a, b T}` which might be allowed by the `unsafe` package #65535

Jorropo · 2024-02-06T01:46:26Z

Go version

go version devel go1.23-6076edc55c Mon Feb 5 20:59:15 2024 +0000 linux/amd64

Output of `go env` in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/hugo/.cache/go-build'
GOENV='/home/hugo/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/hugo/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/hugo/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/hugo/k/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/hugo/k/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.23-6076edc55c Mon Feb 5 20:59:15 2024 +0000'
GCCGO='/usr/bin/gccgo'
GOAMD64='v3'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1276548902=/tmp/go-build -gno-record-gcc-switches'

What did you do?

While investing #65495 I noticed that the compiler makes assumptions about aliasing of structs which are not documented in unsafe.

See this piece of code:

package main

import "unsafe"
import "fmt"

type S0 struct{ a, b S1 }
type S1 struct{ a, b S2 }
type S2 struct{ a, b S3 }
type S3 struct{ a, b S4 }
type S4 struct{ a, b S5 }
type S5 struct{ a, b S6 }
type S6 struct{ a, b S7 }
type S7 struct{ a, b S8 }
type S8 struct{ a, b int }

//go:noinline
func Copy(a, b *S0) {
	*a = *b
}

func main() {
	var arr [3]S1
	arr[0].a.a.a.a.a.a.a.a = 42
	arr[1].a.a.a.a.a.a.a.a = 43
	arr[2].a.a.a.a.a.a.a.a = 44
	Copy((*S0)(unsafe.Pointer(&arr[1])), (*S0)(unsafe.Pointer(&arr[0])))
	fmt.Println(arr[2].a.a.a.a.a.a.a.a)
}

What did you see happen?

What did you expect to see?

The text was updated successfully, but these errors were encountered:

Jorropo · 2024-02-06T01:52:19Z

This happen because the compiler use a non saving copy to handle the copy:

  TEXT command-line-arguments.Copy(SB), ABIInternal
  MOVQ AX, DI
  MOVQ BX, SI
  MOVL $512, CX
  REP
  MOVSQ
  RET

I would guess duffcopy has the same issue.

It seems the compiler assumes structs are either not aliased at all or completely aliased (a == b then copy is a no-op).
It's unclear to me why unsafe's doc would disallow this.

(1) Conversion of a *T1 to Pointer to *T2.
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.

(*S0)(unsafe.Pointer(&arr[1])) is legal because &arr[1] point to 2 S1 which is just as large and has the same memory layout as S0.
Is also legal for the same reasons (*S0)(unsafe.Pointer(&arr[0])) (it now points to 3 S1 which is larger than 1 S0).

I don't know if this could be a bug in unsafe's docs, or in the compiler.

I don't know if it possible to manifest this bug without using unsafe.

Jorropo · 2024-02-06T02:04:30Z

I found this, so it looks like a sentence is needed in unsafe:

go/src/cmd/compile/internal/ssagen/ssa.go

Lines 1382 to 1399 in 6076edc

    
           // Normally, when moving Go values of type T from one location to another, 
        
           // we don't need to worry about partial overlaps. The two Ts must either be 
        
           // in disjoint (nonoverlapping) memory or in exactly the same location. 
        
           // There are 2 cases where this isn't true: 
        
           //  1) Using unsafe you can arrange partial overlaps. 
        
           //  2) Since Go 1.17, you can use a cast from a slice to a ptr-to-array. 
        
           //     https://go.dev/ref/spec#Conversions_from_slice_to_array_pointer 
        
           //     This feature can be used to construct partial overlaps of array types. 
        
           //       var a [3]int 
        
           //       p := (*[2]int)(a[:]) 
        
           //       q := (*[2]int)(a[1:]) 
        
           //       *p = *q 
        
           // We don't care about solving 1. Or at least, we haven't historically 
        
           // and no one has complained. 
        
           // For 2, we need to ensure that if there might be partial overlap, 
        
           // then we can't use OpMove; we must use memmove instead. 
        
           // (memmove handles partial overlap by copying in the correct 
        
           // direction. OpMove does not.)

randall77 · 2024-02-06T02:27:04Z

This conversion already violates unsafe rules:

(*S0)(unsafe.Pointer(&arr[1]))

This violates rule 1, casting from a smaller to a larger pointed-to type.

Jorropo · 2024-02-06T02:48:59Z

I did thought about that.
The correct version would be (*S0)(unsafe.Add(unsafe.Pointer(&arr), unsafe.Sizeof(S1{})) it's more verbose and I was lazy.

randall77 · 2024-02-06T04:32:48Z

Reinterpreting a [2]T to a struct{a, b T} is definitely stretching what is allowed by the unsafe rules. But I don't see any rule explicitly disallowing it.

mknyszek · 2024-02-14T21:46:41Z

In triage, we agree that the kind of conversion mentioned in this issue is a bit of a stretch. You could argue that converting [2]T to a struct{a, b T} is incorrectly assuming equivalent memory layouts, especially since the spec (technically) allows for things like reordering struct fields.

mknyszek · 2024-02-14T21:48:48Z

We should perhaps consider clarifying this case in unsafe, in which case this is really more an unsafe package issue as it was originally, but I retitled it to more directly described what @Jorropo discovered.

qiulaidongfeng · 2024-02-15T01:27:08Z

What about this code that treats any as [2]uintptr?（Currently any is equivalent to a structure of two Pointers, thus converted no problem or rely on existing compiler behavior?）
https://gitee.com/qiulaidongfeng/arena/blob/master/buf.go#L150

randall77 · 2024-02-15T03:22:40Z

@qiulaidongfeng That would almost certainly fall under the same non-guarantee. Layouts of multi-word builtin types (interfaces, slices, strings, complex) should not be guaranteed to be interchangeable by unsafe recasting.

gopherbot added compiler/runtime Issues related to the Go compiler and/or runtime. Documentation labels Feb 6, 2024

Jorropo added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 6, 2024

Jorropo changed the title ~~cmd/compile,unsafe: inproper documentation of when aliasing pointers is illegal or misscompilation~~ unsafe: inproper documentation of when aliasing pointers is illegal Feb 6, 2024

Jorropo removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. compiler/runtime Issues related to the Go compiler and/or runtime. labels Feb 6, 2024

dr2chase added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 8, 2024

mknyszek changed the title ~~unsafe: inproper documentation of when aliasing pointers is illegal~~ unsafe: improper documentation of when aliasing pointers is illegal Feb 14, 2024

mknyszek changed the title ~~unsafe: improper documentation of when aliasing pointers is illegal~~ cmd/compile: compiler assumes [2]T does not alias with struct{a, b T} which might be allowed by the unsafe package Feb 14, 2024

mknyszek added this to the Backlog milestone Feb 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile: compiler assumes `[2]T` does not alias with `struct{a, b T}` which might be allowed by the `unsafe` package #65535

cmd/compile: compiler assumes `[2]T` does not alias with `struct{a, b T}` which might be allowed by the `unsafe` package #65535

Jorropo commented Feb 6, 2024

Jorropo commented Feb 6, 2024 •

edited

Jorropo commented Feb 6, 2024

randall77 commented Feb 6, 2024

Jorropo commented Feb 6, 2024 •

edited

randall77 commented Feb 6, 2024

mknyszek commented Feb 14, 2024

mknyszek commented Feb 14, 2024

qiulaidongfeng commented Feb 15, 2024 •

edited

randall77 commented Feb 15, 2024

cmd/compile: compiler assumes [2]T does not alias with struct{a, b T} which might be allowed by the unsafe package #65535

cmd/compile: compiler assumes [2]T does not alias with struct{a, b T} which might be allowed by the unsafe package #65535

Comments

Jorropo commented Feb 6, 2024

Go version

Output of go env in your module/workspace:

What did you do?

What did you see happen?

What did you expect to see?

Jorropo commented Feb 6, 2024 • edited

Jorropo commented Feb 6, 2024

randall77 commented Feb 6, 2024

Jorropo commented Feb 6, 2024 • edited

randall77 commented Feb 6, 2024

mknyszek commented Feb 14, 2024

mknyszek commented Feb 14, 2024

qiulaidongfeng commented Feb 15, 2024 • edited

randall77 commented Feb 15, 2024

cmd/compile: compiler assumes `[2]T` does not alias with `struct{a, b T}` which might be allowed by the `unsafe` package #65535

cmd/compile: compiler assumes `[2]T` does not alias with `struct{a, b T}` which might be allowed by the `unsafe` package #65535

Output of `go env` in your module/workspace:

Jorropo commented Feb 6, 2024 •

edited

Jorropo commented Feb 6, 2024 •

edited

qiulaidongfeng commented Feb 15, 2024 •

edited