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/cgo: type confusion between CGO and CoreFoundation types on Go1.10 #24161

Closed
akalin-keybase opened this issue Feb 27, 2018 · 21 comments

Comments

@akalin-keybase
Copy link

commented Feb 27, 2018

Please answer these questions before submitting your issue. Thanks!

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

1.10

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/akalin-keybase/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/akalin-keybase/go"
GORACE=""
GOROOT="/Users/akalin-keybase/homebrew/Cellar/go/1.10/libexec"
GOTMPDIR=""
GOTOOLDIR="/Users/akalin-keybase/homebrew/Cellar/go/1.10/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
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/f7/k3hkmgcx71d2n0vz0fp2662r0000gq/T/go-build917425438=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

What did you expect to see?

The following file should compile:

// +build darwin

package kext

/*
#cgo LDFLAGS: -framework CoreFoundation -framework IOKit

#include <CoreFoundation/CoreFoundation.h>
*/
import "C"

func bugRepro() {
	arr := returnsCFArrayRef()
	// If arr is replaced with C.CFArrayRef(arr), compilation succeeds.
	C.CFArrayCreateCopy(nil, arr)
}

returnsCFArrayRef is a function in another file in the same package that looks like

func returnsCFArrayRef() C.CFArrayRef {
  // returns C.CFArrayRef(0) or C.CFArrayRef(nil) depending on go version.
}

A full example is on https://gist.github.com/akalin-keybase/6d075a61e683a4b2e2424638308e95c5 .

What did you see instead?

go install ./...
# github.com/keybase/go-kext
./file2.go:22: cannot use arr (type _Ctype_CFArrayRef) as type *_Ctype_struct___CFArray in argument to func literal

Compilation succeeds with go 1.9.2.

This is likely related to the go 1.10 changes that made some C types map to uintptr.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

Did you read https://golang.org/doc/go1.10#cgo ? Starting at "Cgo now translates some C types".

@akalin-keybase

This comment has been minimized.

Copy link
Author

commented Feb 27, 2018

Yes, that's not the problem here. The problem is that a variable ostensibly of type C.CFArrayRef (since it was assigned from a function that returns C.CFArrayRef) gives a compile error when passed to a function that expect a parameter of type C.CFArrayRef. This should work regardless of whether C.CFArrayRef is mapped to uintptr or unsafe.Pointer.

In fact, merely mentioning the C.CFArrayRef somewhere else in the file suffices to make compilation succeed, which seems to suggest that the original compilation error isn't the fault of the code itself.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

@bradfitz bradfitz added this to the Go1.11 milestone Feb 27, 2018

@randall77

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

Question: is there an import "C" (and #include directive?) in the file in which returnsCFArrayRef is defined? Does it fix things to add that import and/or include?

@akalin-keybase

This comment has been minimized.

Copy link
Author

commented Feb 27, 2018

@randall77 yes, both import "C" and #include are already present; see https://gist.github.com/akalin-keybase/6d075a61e683a4b2e2424638308e95c5 ; the bug is present even with it.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.10.1 Feb 28, 2018

@odeke-em odeke-em changed the title cgo: Type confusion with go 1.10, cgo, and CoreFoundation types cmd/cgo: type confusion between CGO and CoreFoundation types on Go1.10 Mar 2, 2018

@andybons andybons modified the milestones: Go1.10.1, Go1.10.2 Mar 27, 2018

@randall77

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2018

This one is tricky.
In file2.go, their is a call f(x, y) where y's type is one of the bad typedefs.
That bad typedef name is not mentioned anywhere in the file. (y gets its type from a := assignment.)
So when cgo rewrites the call f(x, y) to func(_x typeof(x), _y typeof(y)) { ..cgochecks.. f(_x, _y) }(x, y), the typeof(y) is wrong, particularly because of this, from cmd/cgo:gcc.go:FuncArg:

		// C has much more relaxed rules than Go for
		// implicit type conversions. When the parameter
		// is type T defined as *X, simulate a little of the
		// laxness of C by making the argument *X instead of T.

But more generally, introducing typeof(y) when we never properly probed its badness is problematic. Even if we disabled the T->*X relaxation, we still generate the type declaration:

type _Ctype_CFArrayRef *_Ctype_struct___CFArray

Whereas when cgo processes file1.go, we get

type _Ctype_CFArrayRef uintptr

It happens to build successfully like that, but it sounds dangerous in general.

We could somehow re-run the part that asks gcc for type info, once we extract all the argument types from functions whose types we obtained from the first gcc run.

@mastahyeti

This comment has been minimized.

Copy link

commented Apr 17, 2018

Sorry to pile on here, but I noticed some additional weird behavior. The SecKeyAlgorithm type from Security.framework is defined as

typedef CFStringRef SecKeyAlgorithm CF_STRING_ENUM

Variables of this type can be passed to C functions without an issue. When you start using CFStringRefs though, you start getting errors about the SecKeyAlgorithm usage.

Here is a contrived example that doesn't really do anything.

  • example0.go calls some Security.framework function with a SecKeyAlgorithm argument. The build succeeds.
  • example1.go adds a function call that returns a CFStringRef which is then compared with an int. The compiler complains about this comparison.
  • example2.go adds var _ C.CFStringRef. The compiler no longer complains about the CFStringRef vs. int comparison, but now complains about our SecKeyAlgorithm argument being a _Ctype_CFStringRef instead of a *_Ctype_struct___CFString.
  • example3.go casts the SecKeyAlgorithm to an unsafe.Pointer and then to a *struct___CFString. The build succeeds.
mastahyeti added a commit to mastahyeti/certstore that referenced this issue Apr 17, 2018

@FiloSottile FiloSottile modified the milestones: Go1.10.2, Go1.11 Apr 24, 2018

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

@gopherbot please file this for backport against 1.10. This is a regression.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 24, 2018

Backport issue(s) opened: #25036 (for 1.10).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot

This comment has been minimized.

Copy link

commented Jul 7, 2018

Change https://golang.org/cl/122575 mentions this issue: cmd/cgo: check function argument types for bad C pointer types

@gopherbot gopherbot closed this in 94076fe Jul 9, 2018

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2018

Reopening, my fix doesn't fix all of this. @mastahyeti 's examples still don't work.

@randall77 randall77 reopened this Jul 9, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Jul 10, 2018

Change https://golang.org/cl/123177 mentions this issue: cmd/cgo: fix cgo bad typedefs

@gopherbot gopherbot closed this in b888a62 Jul 12, 2018

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

@gopherbot open some more backport issues please.

@gopherbot

This comment has been minimized.

Copy link

commented Jul 12, 2018

Change https://golang.org/cl/123715 mentions this issue: misc/cgo/test: fix issue 24161 test for 1.11 and earlier

gopherbot pushed a commit that referenced this issue Jul 13, 2018
misc/cgo/test: fix issue 24161 test for 1.11 and earlier
The test uses functions from C that were introduced in OSX 1.12.
Include stubs for those functions when compiling for 1.11 and earlier.
This test really a compile-time test, it doesn't matter much what the
executed code actually does.
Use a nasty #define hack to work around the fact that cgo doesn't
support static global variables.

Update #24161
Fixes #26355

Change-Id: Icf6f7bc9b6b36cacc81d5d0e033a2ebaff7e0298
Reviewed-on: https://go-review.googlesource.com/123715
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Jul 15, 2018

Change https://golang.org/cl/123959 mentions this issue: misc/cgo: fix test on iOS

gopherbot pushed a commit that referenced this issue Jul 15, 2018
misc/cgo: fix test on iOS
The test in CL 123715 doesn't work on iOS, it needs to use a different
version scheme to determine whether SecKeyAlgorithm and friends exist.
Restrict the old version test to OSX only.

The same problem occurs on iOS: the functions tested don't exist before
iOS 10.  But we don't have builders below iOS 10, so it isn't a big issue.
If we ever get older builders, or someone wants to run all.bash on an
old iOS, they'll need to figure out the right incantation.

Update #24161
Update #26355

Change-Id: Ia3ace86b00486dc172ed00c0c6d668a95565bff7
Reviewed-on: https://go-review.googlesource.com/123959
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Jul 16, 2018

Change https://golang.org/cl/124075 mentions this issue: misc/cgo: fix darwin test, again

gopherbot pushed a commit that referenced this issue Jul 16, 2018
misc/cgo: fix darwin test, again
TARGET_OS_OSX is the right macro, but it also was only introduced
in 1.12.  For 1.11 and earlier a reasonable substitution is
TARGET_OS_IPHONE == 0.

Update #24161
Update #26355

Change-Id: I5f43c463d14fada9ed1d83cc684c7ea05d94c5f3
Reviewed-on: https://go-review.googlesource.com/124075
Run-TryBot: Keith Randall <khr@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@mvdan

This comment has been minimized.

Copy link
Member

commented Jul 21, 2018

@randall77 it seems like the fix for this issue introduced a regression, so beware when backporting this for 1.10.4: #26517

@mastahyeti

This comment has been minimized.

Copy link

commented Sep 4, 2018

Go 1.11 breaks the workaround for the behavior I described above. Here is the output from compiling the example workaround:

$ go build ./example3.go
# command-line-arguments
./example3.go:19:223: cannot use nil as type _Ctype_SecKeyRef in argument to func literal
./example3.go:19:223: cannot use algo (type *_Ctype_struct___CFString) as type _Ctype_CFStringRef in argument to func literal
./example3.go:19:223: cannot use nil as type _Ctype_CFDataRef in argument to func literal

I don't think it will be possible to fix this in a way that works with 1.9, 1.10, and 1.11 without writing version-specific code and using build tags.

@mastahyeti

This comment has been minimized.

Copy link

commented Sep 4, 2018

I guess its not quite so dire. The only versions that would require special casing would be 1.10.0→1.10.3. I hadn't realized that there had been a patch release changing the behavior (1.10.4).

nathany added a commit to fsnotify/fsevents that referenced this issue Sep 5, 2018
Cannot use nil for CFType (Go 1.10+)
fixes #40

Cannot use nil for CFType (Go 1.10+), round 2

reducing code changes between Go versions

conversions can occur in older Go versions too

extract eventIDSinceNow constant to other files

Go-version specific files.

reworks #34 which resolves int overflow #31

minimize code differences between Go versions

Work around bizarre bugs in 1.10.3 (not 1.10.4)

golang/go#24161

use C.kCFAllocatorDefault instead of C.CFAllocatorRef(0)

thanks to @havoc-io for the tip

reduce duplication between go 1.10 before/after

thanks to C.kCFAllocatorDefault change
nathany added a commit to fsnotify/fsevents that referenced this issue Sep 5, 2018
Cannot use nil for CFType (Go 1.10+)
fixes #40

Cannot use nil for CFType (Go 1.10+), round 2

reducing code changes between Go versions

conversions can occur in older Go versions too

extract eventIDSinceNow constant to other files

Go-version specific files.

reworks #34 which resolves int overflow #31

minimize code differences between Go versions

Work around bizarre bugs in 1.10.3 (not 1.10.4)

golang/go#24161

use C.kCFAllocatorDefault instead of C.CFAllocatorRef(0)

thanks to @havoc-io for the tip

reduce duplication between go 1.10 before/after

thanks to C.kCFAllocatorDefault change
vjsamuel added a commit to vjsamuel/fsevents that referenced this issue Oct 27, 2018
Cannot use nil for CFType (Go 1.10+)
fixes fsnotify#40

Cannot use nil for CFType (Go 1.10+), round 2

reducing code changes between Go versions

conversions can occur in older Go versions too

extract eventIDSinceNow constant to other files

Go-version specific files.

reworks fsnotify#34 which resolves int overflow fsnotify#31

minimize code differences between Go versions

Work around bizarre bugs in 1.10.3 (not 1.10.4)

golang/go#24161

use C.kCFAllocatorDefault instead of C.CFAllocatorRef(0)

thanks to @havoc-io for the tip

reduce duplication between go 1.10 before/after

thanks to C.kCFAllocatorDefault change
@vjsamuel vjsamuel referenced this issue Oct 27, 2018
vjsamuel added a commit to vjsamuel/fsevents that referenced this issue Oct 27, 2018
Cannot use nil for CFType (Go 1.10+)
fixes fsnotify#40

Cannot use nil for CFType (Go 1.10+), round 2

reducing code changes between Go versions

conversions can occur in older Go versions too

extract eventIDSinceNow constant to other files

Go-version specific files.

reworks fsnotify#34 which resolves int overflow fsnotify#31

minimize code differences between Go versions

Work around bizarre bugs in 1.10.3 (not 1.10.4)

golang/go#24161

use C.kCFAllocatorDefault instead of C.CFAllocatorRef(0)

thanks to @havoc-io for the tip

reduce duplication between go 1.10 before/after

thanks to C.kCFAllocatorDefault change
andrewkroh added a commit to elastic/fsevents that referenced this issue Oct 29, 2018
Cannot use nil for CFType (Go 1.10+)
fixes fsnotify#40

Cannot use nil for CFType (Go 1.10+), round 2

reducing code changes between Go versions

conversions can occur in older Go versions too

extract eventIDSinceNow constant to other files

Go-version specific files.

reworks fsnotify#34 which resolves int overflow fsnotify#31

minimize code differences between Go versions

Work around bizarre bugs in 1.10.3 (not 1.10.4)

golang/go#24161

use C.kCFAllocatorDefault instead of C.CFAllocatorRef(0)

thanks to @havoc-io for the tip

reduce duplication between go 1.10 before/after

thanks to C.kCFAllocatorDefault change
@lzhaoGitHub

This comment has been minimized.

Copy link

commented Jan 8, 2019

github.com/pchain/vendor/github.com/rjeczalik/notify

vendor/github.com/rjeczalik/notify/watcher_fsevents_cgo.go:51:216: cannot use nil as type _Ctype_CFAllocatorRef in argument to func literal
vendor/github.com/rjeczalik/notify/watcher_fsevents_cgo.go:162:47: cannot use nil as type _Ctype_CFAllocatorRef in argument to _Cfunc_CFStringCreateWithCStringNoCopy
vendor/github.com/rjeczalik/notify/watcher_fsevents_cgo.go:163:225: cannot use nil as type _Ctype_CFAllocatorRef in argument to func literal
make: *** [build] Error 2

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

@lzhaoGitHub This issue is closed. If you are having a problem, please open a new issue with full details, including filling out the issue template. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.