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

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

Open
SimplyDanny opened this issue Aug 20, 2019 · 8 comments

Comments

@SimplyDanny
Copy link

commented Aug 20, 2019

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.

@gopherbot gopherbot added this to the Unreleased milestone Aug 20, 2019

@gopherbot gopherbot added the mobile label Aug 20, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

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

This comment has been minimized.

Copy link
Author

commented Aug 22, 2019

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

This comment has been minimized.

Copy link
Author

commented Sep 8, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

Good one !

@steeve

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

SimplyDanny added a commit to SimplyDanny/passforios that referenced this issue Sep 8, 2019
Add extension to convert objects of type Data to instances of NSMutab…
…leData

This process is necessary because of an issue (golang/go#33745) in gomobile. Passing bare Data objects to Go functions leads to nondeterministic behavior.
@steeve

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

@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...

SimplyDanny added a commit to SimplyDanny/passforios that referenced this issue Sep 14, 2019
Add extension to convert objects of type Data to instances of NSMutab…
…leData

This process is necessary because of an issue (golang/go#33745) in gomobile. Passing bare Data objects to Go functions leads to nondeterministic behavior.
SimplyDanny added a commit to SimplyDanny/passforios that referenced this issue Sep 14, 2019
Add extension to convert objects of type Data to instances of NSMutab…
…leData

This process is necessary because of an issue (golang/go#33745) in gomobile. Passing bare Data objects to Go functions leads to nondeterministic behavior.
mssun added a commit to mssun/passforios that referenced this issue Sep 15, 2019
Add extension to convert objects of type Data to instances of NSMutab…
…leData

This process is necessary because of an issue (golang/go#33745) in gomobile. Passing bare Data objects to Go functions leads to nondeterministic behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.