x/mobile: Issues converting Data to []byte in Swift

[SimplyDanny]

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

$ go version
go version go1.12.9 darwin/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
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12.9/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12.9/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/gg/29c0x6691455vgkxwl2hrpgc0000gn/T/go-build354245371=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

The Go source code

package gocode

import "fmt"

type Bytes struct {
	elements []byte
}

func NewBytes(elements []byte) *Bytes {
	fmt.Println("Constructor: ", elements)
	return &Bytes{elements: elements}
}

func (bytes *Bytes) GetElements() []byte {
	fmt.Println("Getter: ", bytes.elements)
	return bytes.elements
}

is compiled into an iOS framework using $GOPATH/bin/gomobile bind -target ios -o Gocode.framework gocode.
In a minimal iOS application there is the Swift code

import Foundation
import Gocode

let data = Data(repeating: 0, count: 8)
print("Data: \([UInt8](data))")
let bytes = GocodeNewBytes(data)!
print("Elements: \([UInt8](bytes.getElements()!))")
print("Data: \([UInt8](data))")

which can be compiled and run just fine. However, the printed values are not what I would expect.

What did you expect to see?

Data: [0, 0, 0, 0, 0, 0, 0, 0]
Constructor:  [0 0 0 0 0 0 0 0]
Getter:  [0 0 0 0 0 0 0 0]
Elements: [0, 0, 0, 0, 0, 0, 0, 0]
Data: [0, 0, 0, 0, 0, 0, 0, 0]

What did you see instead?

Data: [0, 0, 0, 0, 0, 0, 0, 0]
Constructor:  [0 0 0 0 0 0 0 0]
Getter:  [184 144 247 1 1 0 0 0]
Elements: [184, 144, 247, 1, 1, 0, 0, 0]
Data: [0, 0, 0, 0, 0, 0, 0, 0]

The data is correctly passed to the Go functions and back to Swift, but internally constructing the Bytes object it somehow changes. If I rewrite the constructor in Go as

func NewBytes(elements []byte) *Bytes {
	fmt.Println("Constructor: ", elements)
	tmp := make([]byte, len(elements))
	copy(tmp, elements)
	return &Bytes{elements: tmp}
}

it works as anticipated. I'm not sure whether this is a bug or expected but unpleasant behavior due to Go's and Swift's handling of values and references. If the latter is true I really have no idea how to solve this issue on the Swift side in order to utilize Go libraries using []byte at any interface function.

[bcmills]

CC @hyangah @eliasnaur @steeve

My guess (but it's just a guess) would be that the gomobile bindings are passing in a slice that aliases Swift-owned memory, and (because objects in Go's heap are not visible to the Swift reference counters) that memory is getting freed upon return.

In other words: I suspect a classic use-after-free bug, obscured by the use of two garbage collectors.

The usual fix for such a bug is to extend the aliased object for the lifetime of the alias. (In a reference-counted language, that would imply pinning the Data reference for the lifetime of the returned Bytes object.)

Or, you can copy passed-in slices rather than retaining references.

[steeve]

Indeed, a NSData is copied before being passed to Go, and then freed, according to https://github.com/golang/mobile/blob/e8b3e6111d02c5a0a25ea00257319a0c01c12806/bind/objc/seq_darwin.m.support#L191

[SimplyDanny]

Strangely enough, the following code prints the correct results in all 5 cases:

var data = Data(repeating: 0, count: 8)
let mutableData = NSMutableData(bytes: &data, length: 8)
print("Data: \(Array(UnsafeBufferPointer(start: mutableData.bytes.assumingMemoryBound(to: UInt8.self), count: 8)))")
let bytes = GocodeNewBytes(mutableData as Data)!
print("Elements: \([UInt8](bytes.getElements()!))")
print("Data: \([UInt8](data))")

This is interesting since now NSMutableData is used which executes the branch where the data is actually not copied.

[SimplyDanny]

After some more investigation I am now pretty sure what the actual problematic behavior is. These are the steps happening in my small example from the issue description:

1. The data object passed into GocodeNewBytes is copied here because it is not of type NSMutableData. A pointer to the copied array is assigned to a nbyteslice struct.
2. The nbyteslice structs lands subsequently in the toSlice function, which does not copy the underlying data of the nbyteslice struct. A pointer to the array forms the data source of a newly created []byte object.
3. This []byte object is actually passed to the GocodeNewBytes function and stored in the constructed object as a field.
4. Eventually, in some code generated here the array created in step 1 by copying the data object is freed. That means, the data source of the []byte object stored in the GocodeNewBytes object is simply removed.

It is the same like doing the following in Go:

p := C.malloc(4)
arr := (*[1 << 30]byte)(p)[:4:4]
// Put some values into 'arr'.
b := NewBytes(arr)
// Values of 'b.elements' are okay.
C.free(p)
// Now 'b.elements' could be anything.

In the cgo wiki it is explicitly stated that ...

... [i]t is important to keep in mind that the Go garbage collector will not interact with this data, and that if it is freed from the C side of things, the behavior of any Go code using the slice is nondeterministic.

So from my understanding, this nondeterminism is exactly the root cause of this issue.

The following solutions come into my mind:

• Always copy sequences before passing them to functions, because it cannot be known whether a Go function just uses its arguments or stores them somewhere.
• Go libraries need to take care that only copies of arguments are stored.
• NSMutableData should always be used instead of Data/NSData as the correspondence to Go's []byte because NSMutableData is passed thought the Go code as it is without creating and freeing copies, making the Swift/Objective-C side responsible for garbage collection.
• Document this behavior very well in gomobile to make users aware of it.

[steeve]

Good one !

[steeve]

From what I'm reading online:

let d = Data() // should be NSData
var d2 = Data() // should be NSMutableData

I'm not able to confirm this yet, however....

It makes me wonder, though, wether it's gomobile's job to ensure one does not mutable a []byte passed from ObjC/Swift code. Since the underlying data structure is never backed by Go, either it is copied into Go's memory, or not copied/freed at all. @eliasnaur, is there a reason not to do that ?

[eliasnaur]

On Sun Sep 8, 2019 at 6:37 AM Steeve Morin wrote: From what I'm reading online: ``` let d = Data() // should be NSData var d2 = Data() // should be NSMutableData ``` I'm not able to confirm this yet, however.... It makes me wonder, though, wether it's gomobile's job to ensure one does not mutable a `[]byte` passed from ObjC/Swift code. Since the underlying data structure is never backed by Go, either it is copied into Go's memory, or not copied/freed at all. @eliasnaur, is there a reason not to do that ?

The intention behind not copying []byte is to enable io.Reader-like interfaces: func ReadData(d []byte) error { // Fill in d. d[...] = ... } Would then be callable with a native byte array. In Java: byte[] d = new byte[100]; go.Pkg.readData(d); // Use d ... Additionally, avoiding the copy leads to performance improvements for very large byte arrays. See also #12113. Now, non-copying byte slices that are only valid during the cross language call are problematic for several reasons: - The static lifetime leads to tricky bugs when a byte slice is retained for longer than the underlying data lives. It seems to me that's what this issue is about. - Confusingly, returning a byte slice from a function will result in a copy, because gomobile doesn't have anywhere to tell the other side to release the byte slice data. In retrospect, the current design is probably a mistake. Perhaps all byte slices should be copied, and a special type, say golang.org/x/mobile/bind.ByteSlice, added that allows for zero-copy transfer of data across the language barrier. ByteSlice can use the usual gomobile reference counting and will not release the underlying byte array or slice memory until its finalizer runs. In other words, ByteSlice will provide dynamic and correct lifetimes of zero-copy memory, replacing the Cgo-like static lifetimes. @steeve, did that answer your question?

[steeve]

@eliasnaur it does!

Actually I was wondering why gobind would copy at all.
I'm actually thinking []byte passed to Go functions should never be copied, and it's the responsibility of the callee to copy if they want to retain that data.
Or perhaps do what JNI does with GetByteArrayElements, saying wether the buffer was modified or not. But it's getting complex...