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 [1.10 backport] #25036

Closed
gopherbot opened this issue Apr 24, 2018 · 9 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone

Comments

@gopherbot
Copy link

@FiloSottile requested issue #24161 to be considered for backport to the next 1.10 minor release.

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

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Apr 24, 2018
@gopherbot gopherbot added this to the Go1.10.2 milestone Apr 24, 2018
@andybons andybons modified the milestones: Go1.10.2, Go1.10.3 Apr 27, 2018
@FiloSottile FiloSottile modified the milestones: Go1.10.3, Go1.10.4 Jun 7, 2018
@gopherbot
Copy link
Author

Change https://golang.org/cl/122818 mentions this issue: [release-branch.go1.10] cmd/cgo: check function argument/return types for bad C pointer types

@gopherbot
Copy link
Author

Change https://golang.org/cl/124218 mentions this issue: [release-branch.go1.10] misc/cgo: fix darwin test, again

@gopherbot
Copy link
Author

Change https://golang.org/cl/124217 mentions this issue: [release-branch.go1.10] misc/cgo: fix test on iOS

@gopherbot
Copy link
Author

Change https://golang.org/cl/124215 mentions this issue: [release-branch.go1.10] cmd/cgo: fix cgo bad typedefs

@gopherbot
Copy link
Author

Change https://golang.org/cl/124216 mentions this issue: [release-branch.go1.10] misc/cgo/test: fix issue 24161 test for 1.11 and earlier

@gopherbot
Copy link
Author

Change https://golang.org/cl/128095 mentions this issue: [release-branch.go1.10] cmd/cgo: make sure we FinishType everything

@gopherbot
Copy link
Author

Change https://golang.org/cl/128096 mentions this issue: [release-branch.go1.10] cmd/cgo: stop expanding typedefs once we reach __builtin types

@gopherbot
Copy link
Author

Closed by merging 48016b8 to release-branch.go1.10.

@gopherbot
Copy link
Author

Closed by merging 424f4ea to release-branch.go1.10.

gopherbot pushed a commit that referenced this issue Aug 8, 2018
… for bad C pointer types

We need to determine whether arguments to and return values from C
functions are "bad" typedef'd pointer types which need to be uintptr
on the Go side.

The type of those arguments are not specified explicitly. As a result,
we never look through the C declarations for the GetTypeID functions
associated with that type, and never realize that they are bad.
However, in another function in the same package there might be an
explicit reference. Then we end up with the declaration being uintptr
in one file and *struct{...} in another file. Badness ensues.

Fix this by doing a 2-pass algorithm. In the first pass, we run as
normal, but record all the argument and result types we see. In the
second pass, we include those argument types also when reading the C
types.

Update #25036

Change-Id: I8d727e73a2fbc88cb9d9899f8719ae405f59f753
Reviewed-on: https://go-review.googlesource.com/122575
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit 20803e0f52809fa6088285c1c87246642df2b62d)
Reviewed-on: https://go-review.googlesource.com/122818
gopherbot pushed a commit that referenced this issue Aug 8, 2018
Two fixes:

1) Typedefs of the bad typedefs should also not be rewritten to the
   underlying type.  They shouldn't just be uintptr, though, they should
   retain the C naming structure.  For example, in C:

   typedef const __CFString * CFStringRef;
   typedef CFStringRef SecKeyAlgorithm;

   we want the Go:

   type _Ctype_CFStringRef uintptr
   type _Ctype_SecKeyAlgorithm = _Ctype_CFStringRef

2) We need more types than just function arguments/return values.
   At least we need types of global variables, so when we see a reference to:

   extern const SecKeyAlgorithm kSecKeyAlgorithmECDSASignatureDigestX962SHA1;

   we know that we need to investigate the type SecKeyAlgorithm.
   Might as well just find every typedef and check the badness of all of them.
   This requires looping until a fixed point of known types is reached.
   Usually it takes just 2 iterations, sometimes 3.

Update #25036

Change-Id: I32ca7e48eb4d4133c6242e91d1879636f5224ea9
Reviewed-on: https://go-review.googlesource.com/123177
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/124215
gopherbot pushed a commit that referenced this issue Aug 8, 2018
…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 #25036

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>
Reviewed-on: https://go-review.googlesource.com/124216
gopherbot pushed a commit that referenced this issue Aug 8, 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 #25036

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>
Reviewed-on: https://go-review.googlesource.com/124217
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Aug 8, 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 #25036

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>
Reviewed-on: https://go-review.googlesource.com/124218
gopherbot pushed a commit that referenced this issue Aug 8, 2018
Ensure that we call FinishType on all the types added to the ptrs map.
We only add a key to ptrKeys once. Once we FinishType for that key,
we'll never look at that key again. But we can add a new type under that
key later, and we'll never finish it.

Make sure we add the key to the ptrKeys list every time we make the list
of types for that key non-empty.

This makes sure we FinishType each pointer type exactly once.

Update #25036

Change-Id: Iad86150d516fcfac167591daf5a26c38bec7d143
Reviewed-on: https://go-review.googlesource.com/126275
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/128095
gopherbot pushed a commit that referenced this issue Aug 8, 2018
…h __builtin types

Expanding __builtin types (__builtin_va_list, particularly) leads
to problems because they are expanded by the compiler itself - the
expansions are not generated by anything in a .h file. The types
a __builtin type expand to are thus very confusing to cgo.

See CL 126275.

Fixes #25036.

Change-Id: I66eb6a4f27f652f1b934ba702f580f6daa62a566
Reviewed-on: https://go-review.googlesource.com/127156
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/128096
@golang golang locked and limited conversation to collaborators Aug 8, 2019
@dmitshur dmitshur added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Dec 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

4 participants