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
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@akalin-keybase
Copy link

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
Copy link
Contributor

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

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 27, 2018
@akalin-keybase
Copy link
Author

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
Copy link
Contributor

/cc @randall77 @ianlancetaylor

@bradfitz bradfitz added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Feb 27, 2018
@bradfitz bradfitz added this to the Go1.11 milestone Feb 27, 2018
@randall77
Copy link
Contributor

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
Copy link
Author

akalin-keybase 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
Copy link
Contributor

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.

@btoews
Copy link

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

btoews added a commit to github/certstore that referenced this issue Apr 17, 2018
@FiloSottile FiloSottile modified the milestones: Go1.10.2, Go1.11 Apr 24, 2018
@FiloSottile
Copy link
Contributor

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

@gopherbot
Copy link

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
Copy link

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

@gopherbot
Copy link

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

@randall77
Copy link
Contributor

@gopherbot open some more backport issues please.

@gopherbot
Copy link

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
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
Copy link

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
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
Copy link

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
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
Copy link
Member

mvdan 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

@btoews
Copy link

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

@btoews
Copy link

btoews 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
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
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 pushed a commit to vjsamuel/fsevents that referenced this issue Oct 27, 2018
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 pushed a commit to vjsamuel/fsevents that referenced this issue Oct 27, 2018
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 pushed a commit to elastic/fsevents that referenced this issue Oct 29, 2018
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
Copy link

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
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Jan 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

10 participants