crypto/x509: frequent panics when doing HTTPS requests on Darwin #30763
Labels
FrozenDueToAge
NeedsInvestigation
Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
OS-Darwin
release-blocker
Milestone
Comments
|
Does this happen in 1.11 too ? |
agnivade
added
OS-Darwin
NeedsInvestigation
agnivade
changed the title
|
/cc @FiloSottile @agl for crypto. @ianlancetaylor for cgo. |
|
I was using Go 1.11.x for quite a while and never saw this issue. Will try to reproduce on Go 1.11 later today.
|
|
Sorry for the late response. I was unable to reproduce this with Go 1.11.5 on OS X 10.11.6 (15G22010). Go 1.12 makes using HTTPS requests quite useless as just about every third one produces a panic. |
|
I bisected it down to f6be1cf which apparently introduced the issue. |
|
Here's the output of the original |
|
Thanks for the bisect ! Tentatively mark it as a release blocker because this seems like a basic behavior. Also probably warrants a backport. @FiloSottile, feel free to decide otherwise. |
marques-work
added a commit
to gocd-contrib/docker-golang-image
that referenced
this issue
Mar 19, 2019
…hen building darwin binaries See golang/go#30763 for details.
|
@agnivade @FiloSottile Will a fix for this be going on in the next release? |
|
Note that per dupe of #30889 this appears to affect any https connection initiated from golang (reproduced here with a clean install of go1.12.2 darwin/amd64 on OS-X 10.11.6 (15G22010)). Suggest changing the title to note the rather broader effect? |
leonklingele
changed the title
|
Hey, is anyone trying to fix HTTPS on OS X in any Go stable release? :) |
|
I suggest to revert f6be1cf until someone familiar with Objective-C finds the time to fix whatever went wrong with that change. |
|
Minimal reproducible: package main
import (
"net/http"
)
func main() {
resp, err := http.Get("https://golang.org/")
if err != nil {
panic(err)
}
println(resp.StatusCode, http.StatusText(resp.StatusCode))
}$ go run main.go
fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x1 addr=0x173e20 pc=0x7fff9dcad4dd]
runtime stack:
runtime.throw(0x12c375a, 0x2a)
/usr/local/Cellar/go/1.12.5/libexec/src/runtime/panic.go:617 +0x72
runtime.sigpanic()
/usr/local/Cellar/go/1.12.5/libexec/src/runtime/signal_unix.go:374 +0x4a9
[etc. see above] |
bradfitz
changed the title
|
It's hard to tell without symbols, but it should be a duplicate of #28092 (comment). |
|
Change https://golang.org/cl/178537 mentions this issue: |
|
This should be now fixed at tip. Please test it with https://golang.org/dl/gotip and report back. If it works, I am going to file this for cherry-picking. |
|
Thanks for the fix, @FiloSottile! https://golang.org/cl/178537 has resolved the issue! 🎉 Tests also pass. |
|
Change https://golang.org/cl/179339 mentions this issue: |
|
Change https://golang.org/cl/179340 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
Jun 7, 2019
…cy on macOS CFDictionaryGetValueIfPresent does not take ownership of the value, so releasing the properties dictionary before passing the value to CFEqual can crash. Not really clear why this works most of the time. See https://developer.apple.com/library/archive/documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html Fixes #32281 Updates #28092 Updates #30763 Change-Id: I5ee7ca276b753a48abc3aedfb78b8af68b448dd4 Reviewed-on: https://go-review.googlesource.com/c/go/+/178537 Reviewed-by: Adam Langley <agl@golang.org> (cherry picked from commit a3d4655) Reviewed-on: https://go-review.googlesource.com/c/go/+/179340 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot
pushed a commit
that referenced
this issue
Jun 7, 2019
…cy on macOS CFDictionaryGetValueIfPresent does not take ownership of the value, so releasing the properties dictionary before passing the value to CFEqual can crash. Not really clear why this works most of the time. See https://developer.apple.com/library/archive/documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html Fixes #32282 Updates #28092 Updates #30763 Change-Id: I5ee7ca276b753a48abc3aedfb78b8af68b448dd4 Reviewed-on: https://go-review.googlesource.com/c/go/+/178537 Reviewed-by: Adam Langley <agl@golang.org> (cherry picked from commit a3d4655) Reviewed-on: https://go-review.googlesource.com/c/go/+/179339 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
|
Change https://golang.org/cl/227037 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
May 7, 2020
+----------------------------------------------------------------------+ | Hello, if you are reading this and run macOS, please test this code: | | | | $ GO111MODULE=on go get golang.org/dl/gotip@latest | | $ gotip download | | $ GODEBUG=x509roots=1 gotip test crypto/x509 -v -run TestSystemRoots | +----------------------------------------------------------------------+ We currently have two code paths to extract system roots on macOS: one uses cgo to invoke a maze of Security.framework APIs; the other is a horrible fallback that runs "/usr/bin/security verify-cert" on every root that has custom policies to check if it's trusted for SSL. The fallback is not only terrifying because it shells out to a binary, but also because it lets in certificates that are not trusted roots but are signed by trusted roots, and because it applies some filters (EKUs and expiration) only to roots with custom policies, as the others are not passed to verify-cert. The other code path, of course, requires cgo, so can't be used when cross-compiling and involves a large ball of C. It's all a mess, and it broke oh-so-many times (#14514, #16532, #19436, #20990, #21416, #24437, #24652, #25649, #26073, #27958, #28025, #28092, #29497, #30471, #30672, #30763, #30889, #32891, #38215, #38365, ...). Since macOS does not have a stable syscall ABI, we already dynamically link and invoke libSystem.dylib regardless of cgo availability (#17490). How that works is that functions in package syscall (like syscall.Open) take the address of assembly trampolines (like libc_open_trampoline) that jump to symbols imported with cgo_import_dynamic (like libc_open), and pass them along with arguments to syscall.syscall (which is implemented as runtime.syscall_syscall). syscall_syscall informs the scheduler and profiler, and then uses asmcgocall to switch to a system stack and invoke runtime.syscall. The latter is an assembly trampoline that unpacks the Go ABI arguments passed to syscall.syscall, finally calls the remote function, and puts the return value on the Go stack. (This last bit is the part that cgo compiles from a C wrapper.) We can do something similar to link and invoke Security.framework! The one difference is that runtime.syscall and friends check errors based on the errno convention, which Security doesn't follow, so I added runtime.syscallNoErr which just skips interpreting the return value. We only need a variant with six arguments because the calling convention is register-based, and extra arguments simply zero out some registers. That's plumbed through as crypto/x509/internal/macOS.syscall. The rest of that package is a set of wrappers for Security.framework and Core Foundation functions, like syscall is for libSystem. In theory, as long as macOS respects ABI backwards compatibility (a.k.a. as long as binaries built for a previous OS version keep running) this should be stable, as the final result is not different from what a C compiler would make. (One exception might be dictionary key strings, which we make our own copy of instead of using the dynamic symbol. If they change the value of those strings things might break. But why would they.) Finally, I rewrote the crypto/x509 cgo logic in Go using those wrappers. It works! I tried to make it match 1:1 the old logic, so that root_darwin_amd64.go can be reviewed by comparing it to root_cgo_darwin_amd64.go. The only difference is that we do proper error handling now, and assume that if there is no error the return values are there, while before we'd just check for nil pointers and move on. I kept the cgo logic to help with review and testing, but we should delete it once we are confident the new code works. The nocgo logic is gone and we shall never speak of it again. Fixes #32604 Fixes #19561 Fixes #38365 Awakens Cthulhu Change-Id: Id850962bad667f71e3af594bdfebbbb1edfbcbb4 Reviewed-on: https://go-review.googlesource.com/c/go/+/227037 Reviewed-by: Katie Hockman <katie@golang.org>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
EDIT:
It turned out this not only affects
go get, but any HTTPS requests on Darwin. See the full conversation in this thread.Run the following command to
get get -ua bunch of apps and libraries (doesn't really matter which ones you get, just make sure you get a lot so this reproduces quicker):$ bash -x -c 'go get -u golang.org/x/tools/cmd/goimports ; go get -u golang.org/x/tools/cmd/gorename ; go get -u github.com/sqs/goreturns ; go get -u github.com/mdempsky/gocode ; go get -u github.com/alecthomas/gometalinter ; go get -u github.com/mgechev/revive ; go get -u github.com/zmb3/gogetdoc ; go get -u github.com/zmb3/goaddimport ; go get -u github.com/rogpeppe/godef ; go get -u golang.org/x/tools/cmd/guru ; go get -u github.com/fatih/gomodifytags ; go get -u github.com/tpng/gopkgs ; go get -u github.com/ramya-rao-a/go-outline'What did you expect to see?
go get -ushould succeed for every one of them.What did you see instead?
A panic from time to time. Example output when running above command, two panic were issued:
The text was updated successfully, but these errors were encountered: