From a171e155000d1f40a6e467e56b4af3a237613c1f Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Mon, 17 Dec 2018 11:33:42 -0800 Subject: [release-branch.go1.11] cmd/compile: generate interface method expression wrapper for error.Error A prior optimization (https://golang.org/cl/106175) removed the generation of unnecessary method expression wrappers, but also eliminated the generation of the wrapper for error.Error which was still required. Special-case error type in the optimization. Fixes #29307 Change-Id: I54c8afc88a2c6d1906afa2d09c68a0a3f3e2f1e3 Reviewed-on: https://go-review.googlesource.com/c/154578 Reviewed-by: Brad Fitzpatrick (cherry picked from commit a1aafd8b28ada0d40e2cb25fb0762ae171eec558) Reviewed-on: https://go-review.googlesource.com/c/154579 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Robert Griesemer --- src/cmd/compile/internal/gc/subr.go | 5 +++-- test/fixedbugs/issue29304.go | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 test/fixedbugs/issue29304.go diff --git a/src/cmd/compile/internal/gc/subr.go b/src/cmd/compile/internal/gc/subr.go index c926b147de..622699e4ae 100644 --- a/src/cmd/compile/internal/gc/subr.go +++ b/src/cmd/compile/internal/gc/subr.go @@ -1627,8 +1627,9 @@ func genwrapper(rcvr *types.Type, method *types.Field, newnam *types.Sym) { return } - // Only generate I.M wrappers for I in I's own package. - if rcvr.IsInterface() && rcvr.Sym != nil && rcvr.Sym.Pkg != localpkg { + // Only generate I.M wrappers for I in I's own package + // but keep doing it for error.Error (was issue #29304). + if rcvr.IsInterface() && rcvr.Sym != nil && rcvr.Sym.Pkg != localpkg && rcvr != types.Errortype { return } diff --git a/test/fixedbugs/issue29304.go b/test/fixedbugs/issue29304.go new file mode 100644 index 0000000000..47bc99f9ca --- /dev/null +++ b/test/fixedbugs/issue29304.go @@ -0,0 +1,19 @@ +// run + +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Verify that relocation target go.builtin.error.Error +// is defined and the code links and runs correctly. + +package main + +import "errors" + +func main() { + err := errors.New("foo") + if error.Error(err) != "foo" { + panic("FAILED") + } +} -- cgit v1.2.3-54-g00ecf From f500f13d72de3ca3201ec2678a7f88524353bede Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Fri, 7 Dec 2018 00:07:43 +0000 Subject: [release-branch.go1.11] runtime: don't clear lockedExt on locked M when G exits When a locked M has its G exit without calling UnlockOSThread, then lockedExt on it was getting cleared. Unfortunately, this meant that during P handoff, if a new M was started, it might get forked (on most OSes besides Windows) from the locked M, which could have kernel state attached to it. To solve this, just don't clear lockedExt. At the point where the locked M has its G exit, it will also exit in accordance with the LockOSThread API. So, we can safely assume that it's lockedExt state will no longer be used. For the case of the main thread where it just gets wedged instead of exiting, it's probably better for it to keep the locked marker since it more accurately represents its state. Fixed #28986. Change-Id: I7d3d71dd65bcb873e9758086d2cbcb9a06429b0f Reviewed-on: https://go-review.googlesource.com/c/155117 Run-TryBot: Michael Knyszek TryBot-Result: Gobot Gobot Reviewed-by: Bryan C. Mills --- src/runtime/proc.go | 5 +- src/runtime/proc_test.go | 6 ++ src/runtime/testdata/testprog/gettid.go | 29 -------- src/runtime/testdata/testprog/gettid_none.go | 15 ---- src/runtime/testdata/testprog/lockosthread.go | 99 ++++++++++++++++++++++++++ src/runtime/testdata/testprog/syscalls.go | 54 ++++++++++++++ src/runtime/testdata/testprog/syscalls_none.go | 27 +++++++ 7 files changed, 190 insertions(+), 45 deletions(-) delete mode 100644 src/runtime/testdata/testprog/gettid.go delete mode 100644 src/runtime/testdata/testprog/gettid_none.go create mode 100644 src/runtime/testdata/testprog/syscalls.go create mode 100644 src/runtime/testdata/testprog/syscalls_none.go diff --git a/src/runtime/proc.go b/src/runtime/proc.go index f82014eb92..e8495d1a10 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -2774,7 +2774,6 @@ func goexit0(gp *g) { print("invalid m->lockedInt = ", _g_.m.lockedInt, "\n") throw("internal lockOSThread error") } - _g_.m.lockedExt = 0 gfput(_g_.m.p.ptr(), gp) if locked { // The goroutine may have locked this thread because @@ -2785,6 +2784,10 @@ func goexit0(gp *g) { // the thread. if GOOS != "plan9" { // See golang.org/issue/22227. gogo(&_g_.m.g0.sched) + } else { + // Clear lockedExt on plan9 since we may end up re-using + // this thread. + _g_.m.lockedExt = 0 } } schedule() diff --git a/src/runtime/proc_test.go b/src/runtime/proc_test.go index ad325987ac..e6947d5849 100644 --- a/src/runtime/proc_test.go +++ b/src/runtime/proc_test.go @@ -885,6 +885,12 @@ func TestLockOSThreadNesting(t *testing.T) { func TestLockOSThreadExit(t *testing.T) { testLockOSThreadExit(t, "testprog") + + want := "OK\n" + output := runTestProg(t, "testprog", "LockOSThreadAvoidsStatePropagation", "GOMAXPROCS=1") + if output != want { + t.Errorf("want %s, got %s\n", want, output) + } } func testLockOSThreadExit(t *testing.T, prog string) { diff --git a/src/runtime/testdata/testprog/gettid.go b/src/runtime/testdata/testprog/gettid.go deleted file mode 100644 index 1b3e29ab08..0000000000 --- a/src/runtime/testdata/testprog/gettid.go +++ /dev/null @@ -1,29 +0,0 @@ -// Copyright 2017 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// +build linux - -package main - -import ( - "bytes" - "fmt" - "io/ioutil" - "os" - "syscall" -) - -func gettid() int { - return syscall.Gettid() -} - -func tidExists(tid int) (exists, supported bool) { - stat, err := ioutil.ReadFile(fmt.Sprintf("/proc/self/task/%d/stat", tid)) - if os.IsNotExist(err) { - return false, true - } - // Check if it's a zombie thread. - state := bytes.Fields(stat)[2] - return !(len(state) == 1 && state[0] == 'Z'), true -} diff --git a/src/runtime/testdata/testprog/gettid_none.go b/src/runtime/testdata/testprog/gettid_none.go deleted file mode 100644 index 036db87e10..0000000000 --- a/src/runtime/testdata/testprog/gettid_none.go +++ /dev/null @@ -1,15 +0,0 @@ -// Copyright 2017 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// +build !linux - -package main - -func gettid() int { - return 0 -} - -func tidExists(tid int) (exists, supported bool) { - return false, false -} diff --git a/src/runtime/testdata/testprog/lockosthread.go b/src/runtime/testdata/testprog/lockosthread.go index 88c0d12e4c..5119cf8131 100644 --- a/src/runtime/testdata/testprog/lockosthread.go +++ b/src/runtime/testdata/testprog/lockosthread.go @@ -24,6 +24,12 @@ func init() { runtime.LockOSThread() }) register("LockOSThreadAlt", LockOSThreadAlt) + + registerInit("LockOSThreadAvoidsStatePropagation", func() { + // Lock the OS thread now so main runs on the main thread. + runtime.LockOSThread() + }) + register("LockOSThreadAvoidsStatePropagation", LockOSThreadAvoidsStatePropagation) } func LockOSThreadMain() { @@ -92,3 +98,96 @@ func LockOSThreadAlt() { ok: println("OK") } + +func LockOSThreadAvoidsStatePropagation() { + // This test is similar to LockOSThreadAlt in that it will detect if a thread + // which should have died is still running. However, rather than do this with + // thread IDs, it does this by unsharing state on that thread. This way, it + // also detects whether new threads were cloned from the dead thread, and not + // from a clean thread. Cloning from a locked thread is undesirable since + // cloned threads will inherit potentially unwanted OS state. + // + // unshareFs, getcwd, and chdir("/tmp") are only guaranteed to work on + // Linux, so on other platforms this just checks that the runtime doesn't + // do anything terrible. + // + // This is running locked to the main OS thread. + + // GOMAXPROCS=1 makes this fail much more reliably if a tainted thread is + // cloned from. + if runtime.GOMAXPROCS(-1) != 1 { + println("requires GOMAXPROCS=1") + os.Exit(1) + } + + if err := chdir("/"); err != nil { + println("failed to chdir:", err.Error()) + os.Exit(1) + } + // On systems other than Linux, cwd == "". + cwd, err := getcwd() + if err != nil { + println("failed to get cwd:", err.Error()) + os.Exit(1) + } + if cwd != "" && cwd != "/" { + println("unexpected cwd", cwd, " wanted /") + os.Exit(1) + } + + ready := make(chan bool, 1) + go func() { + // This goroutine must be running on a new thread. + runtime.LockOSThread() + + // Unshare details about the FS, like the CWD, with + // the rest of the process on this thread. + // On systems other than Linux, this is a no-op. + if err := unshareFs(); err != nil { + println("failed to unshare fs:", err.Error()) + os.Exit(1) + } + // Chdir to somewhere else on this thread. + // On systems other than Linux, this is a no-op. + if err := chdir("/tmp"); err != nil { + println("failed to chdir:", err.Error()) + os.Exit(1) + } + + // The state on this thread is now considered "tainted", but it + // should no longer be observable in any other context. + + ready <- true + // Exit with the thread locked. + }() + <-ready + + // Spawn yet another goroutine and lock it. Since GOMAXPROCS=1, if + // for some reason state from the (hopefully dead) locked thread above + // propagated into a newly created thread (via clone), or that thread + // is actually being re-used, then we should get scheduled on such a + // thread with high likelihood. + done := make(chan bool) + go func() { + runtime.LockOSThread() + + // Get the CWD and check if this is the same as the main thread's + // CWD. Every thread should share the same CWD. + // On systems other than Linux, wd == "". + wd, err := getcwd() + if err != nil { + println("failed to get cwd:", err.Error()) + os.Exit(1) + } + if wd != cwd { + println("bad state from old thread propagated after it should have died") + os.Exit(1) + } + <-done + + runtime.UnlockOSThread() + }() + done <- true + runtime.UnlockOSThread() + println("OK") +} diff --git a/src/runtime/testdata/testprog/syscalls.go b/src/runtime/testdata/testprog/syscalls.go new file mode 100644 index 0000000000..08284fc561 --- /dev/null +++ b/src/runtime/testdata/testprog/syscalls.go @@ -0,0 +1,54 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// +build linux + +package main + +import ( + "bytes" + "fmt" + "io/ioutil" + "os" + "syscall" +) + +func gettid() int { + return syscall.Gettid() +} + +func tidExists(tid int) (exists, supported bool) { + stat, err := ioutil.ReadFile(fmt.Sprintf("/proc/self/task/%d/stat", tid)) + if os.IsNotExist(err) { + return false, true + } + // Check if it's a zombie thread. + state := bytes.Fields(stat)[2] + return !(len(state) == 1 && state[0] == 'Z'), true +} + +func getcwd() (string, error) { + if !syscall.ImplementsGetwd { + return "", nil + } + // Use the syscall to get the current working directory. + // This is imperative for checking for OS thread state + // after an unshare since os.Getwd might just check the + // environment, or use some other mechanism. + var buf [4096]byte + n, err := syscall.Getcwd(buf[:]) + if err != nil { + return "", err + } + // Subtract one for null terminator. + return string(buf[:n-1]), nil +} + +func unshareFs() error { + return syscall.Unshare(syscall.CLONE_FS) +} + +func chdir(path string) error { + return syscall.Chdir(path) +} diff --git a/src/runtime/testdata/testprog/syscalls_none.go b/src/runtime/testdata/testprog/syscalls_none.go new file mode 100644 index 0000000000..7f8ded3994 --- /dev/null +++ b/src/runtime/testdata/testprog/syscalls_none.go @@ -0,0 +1,27 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// +build !linux + +package main + +func gettid() int { + return 0 +} + +func tidExists(tid int) (exists, supported bool) { + return false, false +} + +func getcwd() (string, error) { + return "", nil +} + +func unshareFs() error { + return nil +} + +func chdir(path string) error { + return nil +} -- cgit v1.2.3-54-g00ecf From 61b8817b8ea28fd56649db38b6f3499a0874e2f9 Mon Sep 17 00:00:00 2001 From: Ian Davis Date: Mon, 3 Sep 2018 11:20:23 +0100 Subject: [release-branch.go1.11] encoding/json: recover saved error context when unmarshalling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #29364 Change-Id: I270c56fd0d5ae8787a1293029aff3072f4f52f33 Reviewed-on: https://go-review.googlesource.com/132955 Reviewed-by: Daniel Martí Run-TryBot: Daniel Martí TryBot-Result: Gobot Gobot (cherry picked from commit 22afb3571c4bb6268664ecc5da4416ec58d3e060) Reviewed-on: https://go-review.googlesource.com/c/155377 Run-TryBot: Brad Fitzpatrick --- src/encoding/json/decode.go | 2 +- src/encoding/json/decode_test.go | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/encoding/json/decode.go b/src/encoding/json/decode.go index 7d235087e6..a563252ce7 100644 --- a/src/encoding/json/decode.go +++ b/src/encoding/json/decode.go @@ -179,7 +179,7 @@ func (d *decodeState) unmarshal(v interface{}) error { // test must be applied at the top level of the value. err := d.value(rv) if err != nil { - return err + return d.addErrorContext(err) } return d.savedError } diff --git a/src/encoding/json/decode_test.go b/src/encoding/json/decode_test.go index 5746ddf986..547f718cb1 100644 --- a/src/encoding/json/decode_test.go +++ b/src/encoding/json/decode_test.go @@ -41,6 +41,16 @@ type VOuter struct { V V } +type W struct { + S SS +} + +type SS string + +func (*SS) UnmarshalJSON(data []byte) error { + return &UnmarshalTypeError{Value: "number", Type: reflect.TypeOf(SS(""))} +} + // ifaceNumAsFloat64/ifaceNumAsNumber are used to test unmarshaling with and // without UseNumber var ifaceNumAsFloat64 = map[string]interface{}{ @@ -408,6 +418,7 @@ var unmarshalTests = []unmarshalTest{ {in: `{"X": 23}`, ptr: new(T), out: T{}, err: &UnmarshalTypeError{"number", reflect.TypeOf(""), 8, "T", "X"}}, {in: `{"x": 1}`, ptr: new(tx), out: tx{}}, {in: `{"x": 1}`, ptr: new(tx), out: tx{}}, {in: `{"x": 1}`, ptr: new(tx), err: fmt.Errorf("json: unknown field \"x\""), disallowUnknownFields: true}, + {in: `{"S": 23}`, ptr: new(W), out: W{}, err: &UnmarshalTypeError{"number", reflect.TypeOf(SS("")), 0, "W", "S"}}, {in: `{"F1":1,"F2":2,"F3":3}`, ptr: new(V), out: V{F1: float64(1), F2: int32(2), F3: Number("3")}}, {in: `{"F1":1,"F2":2,"F3":3}`, ptr: new(V), out: V{F1: Number("1"), F2: int32(2), F3: Number("3")}, useNumber: true}, {in: `{"k1":1,"k2":"s","k3":[1,2.0,3e-3],"k4":{"kk1":"s","kk2":2}}`, ptr: new(interface{}), out: ifaceNumAsFloat64}, -- cgit v1.2.3-54-g00ecf From b2c472f91e9ad8e8a302ad9fc963d4875bdfbae0 Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Tue, 25 Dec 2018 19:36:25 -0500 Subject: [release-branch.go1.11] cmd/compile: fix MIPS SGTconst-with-shift rules (SGTconst [c] (SRLconst _ [d])) && 0 <= int32(c) && uint32(d) <= 31 && 1<<(32-uint32(d)) <= int32(c) -> (MOVWconst [1]) This rule is problematic. 1<<(32-uint32(d)) <= int32(c) meant to say that it is true if c is greater than the largest possible value of the right shift. But when d==1, 1<<(32-1) is negative and results in the wrong comparison. Rewrite the rules in a more direct way. Updates #29402. Fixes #29442. Change-Id: I5940fc9538d9bc3a4bcae8aa34672867540dc60e Reviewed-on: https://go-review.googlesource.com/c/155798 Run-TryBot: Cherry Zhang TryBot-Result: Gobot Gobot Reviewed-by: Keith Randall (cherry picked from commit 6a64efc25004175e198e75191e215a7b1a08a2fa) Reviewed-on: https://go-review.googlesource.com/c/155799 Reviewed-by: David Chase --- src/cmd/compile/internal/ssa/gen/MIPS.rules | 4 ++-- src/cmd/compile/internal/ssa/gen/MIPS64.rules | 4 ++-- src/cmd/compile/internal/ssa/rewriteMIPS.go | 8 ++++---- src/cmd/compile/internal/ssa/rewriteMIPS64.go | 8 ++++---- test/fixedbugs/issue29402.go | 23 +++++++++++++++++++++++ 5 files changed, 35 insertions(+), 12 deletions(-) create mode 100644 test/fixedbugs/issue29402.go diff --git a/src/cmd/compile/internal/ssa/gen/MIPS.rules b/src/cmd/compile/internal/ssa/gen/MIPS.rules index 098e19c8a8..db9c5bc638 100644 --- a/src/cmd/compile/internal/ssa/gen/MIPS.rules +++ b/src/cmd/compile/internal/ssa/gen/MIPS.rules @@ -670,8 +670,8 @@ (SGTUconst [c] (MOVHUreg _)) && 0xffff < uint32(c) -> (MOVWconst [1]) (SGTconst [c] (ANDconst [m] _)) && 0 <= int32(m) && int32(m) < int32(c) -> (MOVWconst [1]) (SGTUconst [c] (ANDconst [m] _)) && uint32(m) < uint32(c) -> (MOVWconst [1]) -(SGTconst [c] (SRLconst _ [d])) && 0 <= int32(c) && uint32(d) <= 31 && 1<<(32-uint32(d)) <= int32(c) -> (MOVWconst [1]) -(SGTUconst [c] (SRLconst _ [d])) && uint32(d) <= 31 && 1<<(32-uint32(d)) <= uint32(c) -> (MOVWconst [1]) +(SGTconst [c] (SRLconst _ [d])) && 0 <= int32(c) && uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c) -> (MOVWconst [1]) +(SGTUconst [c] (SRLconst _ [d])) && uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c) -> (MOVWconst [1]) // absorb constants into branches (EQ (MOVWconst [0]) yes no) -> (First nil yes no) diff --git a/src/cmd/compile/internal/ssa/gen/MIPS64.rules b/src/cmd/compile/internal/ssa/gen/MIPS64.rules index 70f4f0d616..9c16c35438 100644 --- a/src/cmd/compile/internal/ssa/gen/MIPS64.rules +++ b/src/cmd/compile/internal/ssa/gen/MIPS64.rules @@ -667,8 +667,8 @@ (SGTconst [c] (MOVWUreg _)) && c < 0 -> (MOVVconst [0]) (SGTconst [c] (ANDconst [m] _)) && 0 <= m && m < c -> (MOVVconst [1]) (SGTUconst [c] (ANDconst [m] _)) && uint64(m) < uint64(c) -> (MOVVconst [1]) -(SGTconst [c] (SRLVconst _ [d])) && 0 <= c && 0 < d && d <= 63 && 1< (MOVVconst [1]) -(SGTUconst [c] (SRLVconst _ [d])) && 0 < d && d <= 63 && 1< (MOVVconst [1]) +(SGTconst [c] (SRLVconst _ [d])) && 0 <= c && 0 < d && d <= 63 && 0xffffffffffffffff>>uint64(d) < uint64(c) -> (MOVVconst [1]) +(SGTUconst [c] (SRLVconst _ [d])) && 0 < d && d <= 63 && 0xffffffffffffffff>>uint64(d) < uint64(c) -> (MOVVconst [1]) // absorb constants into branches (EQ (MOVVconst [0]) yes no) -> (First nil yes no) diff --git a/src/cmd/compile/internal/ssa/rewriteMIPS.go b/src/cmd/compile/internal/ssa/rewriteMIPS.go index 231949644e..85ec732623 100644 --- a/src/cmd/compile/internal/ssa/rewriteMIPS.go +++ b/src/cmd/compile/internal/ssa/rewriteMIPS.go @@ -5623,7 +5623,7 @@ func rewriteValueMIPS_OpMIPSSGTUconst_0(v *Value) bool { return true } // match: (SGTUconst [c] (SRLconst _ [d])) - // cond: uint32(d) <= 31 && 1<<(32-uint32(d)) <= uint32(c) + // cond: uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c) // result: (MOVWconst [1]) for { c := v.AuxInt @@ -5632,7 +5632,7 @@ func rewriteValueMIPS_OpMIPSSGTUconst_0(v *Value) bool { break } d := v_0.AuxInt - if !(uint32(d) <= 31 && 1<<(32-uint32(d)) <= uint32(c)) { + if !(uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c)) { break } v.reset(OpMIPSMOVWconst) @@ -5860,7 +5860,7 @@ func rewriteValueMIPS_OpMIPSSGTconst_10(v *Value) bool { return true } // match: (SGTconst [c] (SRLconst _ [d])) - // cond: 0 <= int32(c) && uint32(d) <= 31 && 1<<(32-uint32(d)) <= int32(c) + // cond: 0 <= int32(c) && uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c) // result: (MOVWconst [1]) for { c := v.AuxInt @@ -5869,7 +5869,7 @@ func rewriteValueMIPS_OpMIPSSGTconst_10(v *Value) bool { break } d := v_0.AuxInt - if !(0 <= int32(c) && uint32(d) <= 31 && 1<<(32-uint32(d)) <= int32(c)) { + if !(0 <= int32(c) && uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c)) { break } v.reset(OpMIPSMOVWconst) diff --git a/src/cmd/compile/internal/ssa/rewriteMIPS64.go b/src/cmd/compile/internal/ssa/rewriteMIPS64.go index 9cd0050e26..d123652c7b 100644 --- a/src/cmd/compile/internal/ssa/rewriteMIPS64.go +++ b/src/cmd/compile/internal/ssa/rewriteMIPS64.go @@ -6003,7 +6003,7 @@ func rewriteValueMIPS64_OpMIPS64SGTUconst_0(v *Value) bool { return true } // match: (SGTUconst [c] (SRLVconst _ [d])) - // cond: 0 < d && d <= 63 && 1<>uint64(d) < uint64(c) // result: (MOVVconst [1]) for { c := v.AuxInt @@ -6012,7 +6012,7 @@ func rewriteValueMIPS64_OpMIPS64SGTUconst_0(v *Value) bool { break } d := v_0.AuxInt - if !(0 < d && d <= 63 && 1<>uint64(d) < uint64(c)) { break } v.reset(OpMIPS64MOVVconst) @@ -6221,7 +6221,7 @@ func rewriteValueMIPS64_OpMIPS64SGTconst_10(v *Value) bool { return true } // match: (SGTconst [c] (SRLVconst _ [d])) - // cond: 0 <= c && 0 < d && d <= 63 && 1<>uint64(d) < uint64(c) // result: (MOVVconst [1]) for { c := v.AuxInt @@ -6230,7 +6230,7 @@ func rewriteValueMIPS64_OpMIPS64SGTconst_10(v *Value) bool { break } d := v_0.AuxInt - if !(0 <= c && 0 < d && d <= 63 && 1<>uint64(d) < uint64(c)) { break } v.reset(OpMIPS64MOVVconst) diff --git a/test/fixedbugs/issue29402.go b/test/fixedbugs/issue29402.go new file mode 100644 index 0000000000..8a1f959d84 --- /dev/null +++ b/test/fixedbugs/issue29402.go @@ -0,0 +1,23 @@ +// run + +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Issue 29402: wrong optimization of comparison of +// constant and shift on MIPS. + +package main + +//go:noinline +func F(s []int) bool { + half := len(s) / 2 + return half >= 0 +} + +func main() { + b := F([]int{1, 2, 3, 4}) + if !b { + panic("FAIL") + } +} -- cgit v1.2.3-54-g00ecf From c36b5322a7bcb268fa7a4b5ef67ce807745745a9 Mon Sep 17 00:00:00 2001 From: Inada Naoki Date: Thu, 27 Dec 2018 00:47:20 +0900 Subject: [release-branch.go1.11] runtime: skip stack barrier copy when there are no pointers After CL 31455, "go fun(n)" may put "n" to write barrier buffer when there are no pointers in fun's arguments. Updates #29565 Change-Id: Icfa42b8759ce8ad9267dcb3859c626feb6fda381 Reviewed-on: https://go-review.googlesource.com/c/155779 Reviewed-by: Keith Randall (cherry picked from commit 5372257e600989ab4cf742b35e3fa649cad3f45c) Reviewed-on: https://go-review.googlesource.com/c/156357 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- src/runtime/proc.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/runtime/proc.go b/src/runtime/proc.go index e8495d1a10..e309c505b4 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -3371,9 +3371,11 @@ func newproc1(fn *funcval, argp *uint8, narg int32, callergp *g, callerpc uintpt if writeBarrier.needed && !_g_.m.curg.gcscandone { f := findfunc(fn.fn) stkmap := (*stackmap)(funcdata(f, _FUNCDATA_ArgsPointerMaps)) - // We're in the prologue, so it's always stack map index 0. - bv := stackmapdata(stkmap, 0) - bulkBarrierBitmap(spArg, spArg, uintptr(narg), 0, bv.bytedata) + if stkmap.nbit > 0 { + // We're in the prologue, so it's always stack map index 0. + bv := stackmapdata(stkmap, 0) + bulkBarrierBitmap(spArg, spArg, uintptr(narg), 0, bv.bytedata) + } } } -- cgit v1.2.3-54-g00ecf From 84f1e9214470af0998b7774f8abc995c3bd06cbb Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Thu, 3 Jan 2019 11:03:19 -0800 Subject: [release-branch.go1.11] runtime: add test for go function argument scanning Derived from Naoki's reproducer. Update #29565 Change-Id: I1cbd33b38a2f74905dbc22c5ecbad4a87a24bdd1 Reviewed-on: https://go-review.googlesource.com/c/156122 Run-TryBot: Keith Randall Reviewed-by: Brad Fitzpatrick TryBot-Result: Gobot Gobot (cherry picked from commit af4320350b3a156de0d1cfa9845ab1e48dcbfefa) Reviewed-on: https://go-review.googlesource.com/c/156358 Run-TryBot: Ian Lance Taylor Reviewed-by: Ian Lance Taylor --- test/fixedbugs/issue29362.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 test/fixedbugs/issue29362.go diff --git a/test/fixedbugs/issue29362.go b/test/fixedbugs/issue29362.go new file mode 100644 index 0000000000..a8bd607c4a --- /dev/null +++ b/test/fixedbugs/issue29362.go @@ -0,0 +1,42 @@ +// run + +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Verify that we don't consider a Go'd function's +// arguments as pointers when they aren't. + +package main + +import ( + "unsafe" +) + +var badPtr uintptr + +var sink []byte + +func init() { + // Allocate large enough to use largeAlloc. + b := make([]byte, 1<<16-1) + sink = b // force heap allocation + // Any space between the object and the end of page is invalid to point to. + badPtr = uintptr(unsafe.Pointer(&b[len(b)-1])) + 1 +} + +var throttle = make(chan struct{}, 10) + +func noPointerArgs(a, b, c, d uintptr) { + sink = make([]byte, 4096) + <-throttle +} + +func main() { + const N = 1000 + for i := 0; i < N; i++ { + throttle <- struct{}{} + go noPointerArgs(badPtr, badPtr, badPtr, badPtr) + sink = make([]byte, 4096) + } +} -- cgit v1.2.3-54-g00ecf From 8455a848e7e98da060ec3979c9cf32c82bae9a20 Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Thu, 3 Jan 2019 12:13:53 -0800 Subject: [release-branch.go1.11] runtime: don't scan go'd function args past length of ptr bitmap Use the length of the bitmap to decide how much to pass to the write barrier, not the total length of the arguments. The test needs enough arguments so that two distinct bitmaps get interpreted as a single longer bitmap. Fixes #29565 Change-Id: I78f3f7f9ec89c2ad4678f0c52d3d3def9cac8e72 Reviewed-on: https://go-review.googlesource.com/c/156123 Run-TryBot: Keith Randall TryBot-Result: Gobot Gobot Reviewed-by: Austin Clements Reviewed-on: https://go-review.googlesource.com/c/156359 Reviewed-by: Ian Lance Taylor --- src/runtime/proc.go | 2 +- test/fixedbugs/issue29362b.go | 53 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 test/fixedbugs/issue29362b.go diff --git a/src/runtime/proc.go b/src/runtime/proc.go index e309c505b4..301d9213ff 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -3374,7 +3374,7 @@ func newproc1(fn *funcval, argp *uint8, narg int32, callergp *g, callerpc uintpt if stkmap.nbit > 0 { // We're in the prologue, so it's always stack map index 0. bv := stackmapdata(stkmap, 0) - bulkBarrierBitmap(spArg, spArg, uintptr(narg), 0, bv.bytedata) + bulkBarrierBitmap(spArg, spArg, uintptr(bv.n)*sys.PtrSize, 0, bv.bytedata) } } } diff --git a/test/fixedbugs/issue29362b.go b/test/fixedbugs/issue29362b.go new file mode 100644 index 0000000000..d1e3b4733f --- /dev/null +++ b/test/fixedbugs/issue29362b.go @@ -0,0 +1,53 @@ +// run + +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Verify that we don't consider a Go'd function's +// arguments as pointers when they aren't. + +package main + +import ( + "unsafe" +) + +var badPtr uintptr + +var sink []byte + +func init() { + // Allocate large enough to use largeAlloc. + b := make([]byte, 1<<16-1) + sink = b // force heap allocation + // Any space between the object and the end of page is invalid to point to. + badPtr = uintptr(unsafe.Pointer(&b[len(b)-1])) + 1 +} + +var throttle = make(chan struct{}, 10) + +// There are 2 arg bitmaps for this function, each with 2 bits. +// In the first, p and q are both live, so that bitmap is 11. +// In the second, only p is live, so that bitmap is 10. +// Bitmaps are byte aligned, so if the first bitmap is interpreted as +// extending across the entire argument area, we incorrectly concatenate +// the bitmaps and end up using 110000001. That bad bitmap causes a6 +// to be considered a pointer. +func noPointerArgs(p, q *byte, a0, a1, a2, a3, a4, a5, a6 uintptr) { + sink = make([]byte, 4096) + sinkptr = q + <-throttle + sinkptr = p +} + +var sinkptr *byte + +func main() { + const N = 1000 + for i := 0; i < N; i++ { + throttle <- struct{}{} + go noPointerArgs(nil, nil, badPtr, badPtr, badPtr, badPtr, badPtr, badPtr, badPtr) + sink = make([]byte, 4096) + } +} -- cgit v1.2.3-54-g00ecf From e39e43d7349555501080133bb426f1ead4b3ef97 Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Wed, 19 Sep 2018 14:45:45 +0200 Subject: [release-branch.go1.11] cmd/go: respect gcflags, ldflags in 'go test' Fixes bug introduced by https://golang.org/cl/129059 where gcflags='all=...' and ldflags='all=...' would not be applied to some packages built by 'go test'. LoadImport used to set gcflags/ldflags for the Package objects it created, in https://golang.org/cl/129059 this code was factored out to setToolFlags. The codepath of `go build` was updated to call setToolFlags appropriatley, but the codepath of `go test -c` wasn't, resulting in gcflags/ldflags being applied inconsistently when building tests. This commit changes TestPackagesFor to call setToolFlags on the package objects it creates. Fixes #28346 Change-Id: Idcbec0c989ee96ec066207184611f08818873e8d Reviewed-on: https://go-review.googlesource.com/c/136275 Run-TryBot: Jay Conrod TryBot-Result: Gobot Gobot Reviewed-by: Jay Conrod (cherry picked from commit 374546d800124e9ab4d51b75e335a71f866f3ef8) Reviewed-on: https://go-review.googlesource.com/c/156377 Run-TryBot: Alessandro Arzilli --- src/cmd/go/internal/load/test.go | 6 ++++++ src/cmd/go/testdata/script/gcflags_patterns.txt | 28 ++++++++++++++++--------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/src/cmd/go/internal/load/test.go b/src/cmd/go/internal/load/test.go index bb9568d07e..bd6f00bb66 100644 --- a/src/cmd/go/internal/load/test.go +++ b/src/cmd/go/internal/load/test.go @@ -227,6 +227,12 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag } } + allTestImports := make([]*Package, 0, len(pmain.Internal.Imports)+len(imports)+len(ximports)) + allTestImports = append(allTestImports, pmain.Internal.Imports...) + allTestImports = append(allTestImports, imports...) + allTestImports = append(allTestImports, ximports...) + setToolFlags(allTestImports...) + // Do initial scan for metadata needed for writing _testmain.go // Use that metadata to update the list of imports for package main. // The list of imports is used by recompileForTest and by the loop diff --git a/src/cmd/go/testdata/script/gcflags_patterns.txt b/src/cmd/go/testdata/script/gcflags_patterns.txt index fe2cf6f0fb..40f80b7d6e 100644 --- a/src/cmd/go/testdata/script/gcflags_patterns.txt +++ b/src/cmd/go/testdata/script/gcflags_patterns.txt @@ -2,24 +2,28 @@ # -gcflags=-e applies to named packages, not dependencies go build -n -v -gcflags=-e z1 z2 -stderr 'compile.* -e .*-p z1' -stderr 'compile.* -e .*-p z2' +stderr 'compile.* -e.* -p z1' +stderr 'compile.* -e.* -p z2' stderr 'compile.* -p y' -! stderr 'compile.* -e .*-p [^z]' +! stderr 'compile.* -e.* -p [^z]' # -gcflags can specify package=flags, and can be repeated; last match wins go build -n -v -gcflags=-e -gcflags=z1=-N z1 z2 -stderr 'compile.* -N .*-p z1' -! stderr 'compile.* -e .*-p z1' -! stderr 'compile.* -N .*-p z2' -stderr 'compile.* -e .*-p z2' +stderr 'compile.* -N.* -p z1' +! stderr 'compile.* -e.* -p z1' +! stderr 'compile.* -N.* -p z2' +stderr 'compile.* -e.* -p z2' stderr 'compile.* -p y' -! stderr 'compile.* -e .*-p [^z]' -! stderr 'compile.* -N .*-p [^z]' +! stderr 'compile.* -e.* -p [^z]' +! stderr 'compile.* -N.* -p [^z]' # -gcflags can have arbitrary spaces around the flags go build -n -v -gcflags=' z1 = -e ' z1 -stderr 'compile.* -e .*-p z1' +stderr 'compile.* -e.* -p z1' + +# -gcflags='all=-e' should apply to all packages, even with go test +go test -c -n -gcflags='all=-e' z1 +stderr 'compile.* -e.* -p z3 ' # -ldflags for implicit test package applies to test binary go test -c -n -gcflags=-N -ldflags=-X=x.y=z z1 @@ -58,11 +62,15 @@ import _ "z2" -- z1/z_test.go -- package z1_test import "testing" +import _ "z3" func Test(t *testing.T) {} -- z2/z.go -- package z2 +-- z3/z.go -- +package z3 + -- y/y.go -- package y -- cgit v1.2.3-54-g00ecf From cbf9140f8983380d3384d77fcf6b90c928af9c24 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Fri, 11 Jan 2019 14:26:24 -0800 Subject: [release-branch.go1.11] net: pass if at least one matching entry in TestLookupGmailTXT Updates #29698 Fixes #29700 Change-Id: I0531c0a274b120af8871aa2f5975744ff6c912a3 Reviewed-on: https://go-review.googlesource.com/c/157638 Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick (cherry picked from commit 7cbfa55b5d17c8deaecff05e4221f828467cfa97) Reviewed-on: https://go-review.googlesource.com/c/157639 --- src/net/lookup_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/net/lookup_test.go b/src/net/lookup_test.go index 5c66dfa260..f2adbc18c5 100644 --- a/src/net/lookup_test.go +++ b/src/net/lookup_test.go @@ -237,11 +237,16 @@ func TestLookupGmailTXT(t *testing.T) { if len(txts) == 0 { t.Error("got no record") } + found := false for _, txt := range txts { - if !strings.Contains(txt, tt.txt) || (!strings.HasSuffix(txt, tt.host) && !strings.HasSuffix(txt, tt.host+".")) { - t.Errorf("got %s; want a record containing %s, %s", txt, tt.txt, tt.host) + if strings.Contains(txt, tt.txt) && (strings.HasSuffix(txt, tt.host) || strings.HasSuffix(txt, tt.host+".")) { + found = true + break } } + if !found { + t.Errorf("got %v; want a record containing %s, %s", txts, tt.txt, tt.host) + } } } -- cgit v1.2.3-54-g00ecf From ddfb74ed26b77ce6f3346371b28db030c2391cc3 Mon Sep 17 00:00:00 2001 From: Yuval Pavel Zholkover Date: Tue, 8 Jan 2019 00:54:58 +0200 Subject: [release-branch.go1.11] runtime: disable GDB tests on freebsd on all GOARCH values The in-tree GDB is too old (6.1.1) on all the builders except the FreeBSD 12.0 one, where it was removed from the base system. Update #29508 Change-Id: Ib6091cd86440ea005f3f903549a0223a96621a6f Reviewed-on: https://go-review.googlesource.com/c/156717 Reviewed-by: Ian Lance Taylor Reviewed-by: Brad Fitzpatrick Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-on: https://go-review.googlesource.com/c/160800 Run-TryBot: Brad Fitzpatrick Reviewed-by: Yuval Pavel Zholkover --- src/runtime/runtime-gdb_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/runtime/runtime-gdb_test.go b/src/runtime/runtime-gdb_test.go index d9c6f6d22a..2bbcadb626 100644 --- a/src/runtime/runtime-gdb_test.go +++ b/src/runtime/runtime-gdb_test.go @@ -36,6 +36,8 @@ func checkGdbEnvironment(t *testing.T) { if runtime.GOARCH == "mips" { t.Skip("skipping gdb tests on linux/mips; see https://golang.org/issue/25939") } + case "freebsd": + t.Skip("skipping gdb tests on FreeBSD; see https://golang.org/issue/29508") } if final := os.Getenv("GOROOT_FINAL"); final != "" && runtime.GOROOT() != final { t.Skip("gdb test can fail with GOROOT_FINAL pending") -- cgit v1.2.3-54-g00ecf From eb0f2b3d27a896e4b832f2450490a2bbf72fbb6c Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 31 Jan 2019 20:17:12 +0000 Subject: [release-branch.go1.11] net/http, net/url: reject control characters in URLs Cherry pick of combined CL 159157 + CL 160178. Fixes #29923 Updates #27302 Updates #22907 Change-Id: I6de92c14284595a58321a4b4d53229285979b872 Reviewed-on: https://go-review.googlesource.com/c/160798 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- src/net/http/fs_test.go | 15 +++++++++++---- src/net/http/http.go | 11 +++++++++++ src/net/http/request.go | 7 ++++++- src/net/http/requestwrite_test.go | 11 +++++++++++ src/net/url/url.go | 15 +++++++++++++++ src/net/url/url_test.go | 23 ++++++++++++++++++++++- 6 files changed, 76 insertions(+), 6 deletions(-) diff --git a/src/net/http/fs_test.go b/src/net/http/fs_test.go index 255d215f3c..762e88b05f 100644 --- a/src/net/http/fs_test.go +++ b/src/net/http/fs_test.go @@ -583,16 +583,23 @@ func TestFileServerZeroByte(t *testing.T) { ts := httptest.NewServer(FileServer(Dir("."))) defer ts.Close() - res, err := Get(ts.URL + "/..\x00") + c, err := net.Dial("tcp", ts.Listener.Addr().String()) if err != nil { t.Fatal(err) } - b, err := ioutil.ReadAll(res.Body) + defer c.Close() + _, err = fmt.Fprintf(c, "GET /..\x00 HTTP/1.0\r\n\r\n") + if err != nil { + t.Fatal(err) + } + var got bytes.Buffer + bufr := bufio.NewReader(io.TeeReader(c, &got)) + res, err := ReadResponse(bufr, nil) if err != nil { - t.Fatal("reading Body:", err) + t.Fatal("ReadResponse: ", err) } if res.StatusCode == 200 { - t.Errorf("got status 200; want an error. Body is:\n%s", string(b)) + t.Errorf("got status 200; want an error. Body is:\n%s", got.Bytes()) } } diff --git a/src/net/http/http.go b/src/net/http/http.go index ce0eceb1de..07ca78dbc8 100644 --- a/src/net/http/http.go +++ b/src/net/http/http.go @@ -59,6 +59,17 @@ func isASCII(s string) bool { return true } +// stringContainsCTLByte reports whether s contains any ASCII control character. +func stringContainsCTLByte(s string) bool { + for i := 0; i < len(s); i++ { + b := s[i] + if b < ' ' || b == 0x7f { + return true + } + } + return false +} + func hexEscapeNonASCII(s string) string { newLen := 0 for i := 0; i < len(s); i++ { diff --git a/src/net/http/request.go b/src/net/http/request.go index a40b0a3cb8..e352386b08 100644 --- a/src/net/http/request.go +++ b/src/net/http/request.go @@ -545,7 +545,12 @@ func (r *Request) write(w io.Writer, usingProxy bool, extraHeaders Header, waitF // CONNECT requests normally give just the host and port, not a full URL. ruri = host } - // TODO(bradfitz): escape at least newlines in ruri? + if stringContainsCTLByte(ruri) { + return errors.New("net/http: can't write control character in Request.URL") + } + // TODO: validate r.Method too? At least it's less likely to + // come from an attacker (more likely to be a constant in + // code). // Wrap the writer in a bufio Writer if it's not already buffered. // Don't always call NewWriter, as that forces a bytes.Buffer diff --git a/src/net/http/requestwrite_test.go b/src/net/http/requestwrite_test.go index eb65b9f736..3daab4b8b7 100644 --- a/src/net/http/requestwrite_test.go +++ b/src/net/http/requestwrite_test.go @@ -512,6 +512,17 @@ var reqWriteTests = []reqWriteTest{ "User-Agent: Go-http-client/1.1\r\n" + "\r\n", }, + + 21: { + Req: Request{ + Method: "GET", + URL: &url.URL{ + Host: "www.example.com", + RawQuery: "new\nline", // or any CTL + }, + }, + WantError: errors.New("net/http: can't write control character in Request.URL"), + }, } func TestRequestWrite(t *testing.T) { diff --git a/src/net/url/url.go b/src/net/url/url.go index 80eb7a86c8..8d2a856699 100644 --- a/src/net/url/url.go +++ b/src/net/url/url.go @@ -494,6 +494,10 @@ func parse(rawurl string, viaRequest bool) (*URL, error) { var rest string var err error + if stringContainsCTLByte(rawurl) { + return nil, errors.New("net/url: invalid control character in URL") + } + if rawurl == "" && viaRequest { return nil, errors.New("empty url") } @@ -1114,3 +1118,14 @@ func validUserinfo(s string) bool { } return true } + +// stringContainsCTLByte reports whether s contains any ASCII control character. +func stringContainsCTLByte(s string) bool { + for i := 0; i < len(s); i++ { + b := s[i] + if b < ' ' || b == 0x7f { + return true + } + } + return false +} diff --git a/src/net/url/url_test.go b/src/net/url/url_test.go index 9043a844e8..369ea6cbd2 100644 --- a/src/net/url/url_test.go +++ b/src/net/url/url_test.go @@ -1738,8 +1738,29 @@ func TestNilUser(t *testing.T) { } func TestInvalidUserPassword(t *testing.T) { - _, err := Parse("http://us\ner:pass\nword@foo.com/") + _, err := Parse("http://user^:passwo^rd@foo.com/") if got, wantsub := fmt.Sprint(err), "net/url: invalid userinfo"; !strings.Contains(got, wantsub) { t.Errorf("error = %q; want substring %q", got, wantsub) } } + +func TestRejectControlCharacters(t *testing.T) { + tests := []string{ + "http://foo.com/?foo\nbar", + "http\r://foo.com/", + "http://foo\x7f.com/", + } + for _, s := range tests { + _, err := Parse(s) + const wantSub = "net/url: invalid control character in URL" + if got := fmt.Sprint(err); !strings.Contains(got, wantSub) { + t.Errorf("Parse(%q) error = %q; want substring %q", s, got, wantSub) + } + } + + // But don't reject non-ASCII CTLs, at least for now: + if _, err := Parse("http://foo.com/ctl\x80"); err != nil { + t.Errorf("error parsing URL with non-ASCII control byte: %v", err) + } + +} -- cgit v1.2.3-54-g00ecf From e104ebfeabf575d9e33bb56decd6eceb40f041ce Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Mon, 28 Jan 2019 12:31:55 -0800 Subject: [release-branch.go1.11] cmd/cgo: disable GCC 9 warnings triggered by cgo code GCC 9 has started emitting warnings when taking the address of a field in a packed struct may cause a misaligned pointer. We use packed structs in cgo to ensure that our field layout matches the C compiler's layout. Our pointers are always aligned, so disable the warning Updates #29962 Fixes #29967 Change-Id: I7e290a7cf694a2c2958529e340ebed9fcd62089c Reviewed-on: https://go-review.googlesource.com/c/159859 Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: Bryan C. Mills (cherry picked from commit f2a416b90ac68596ea05b97cefa8c72e7416e98f) Reviewed-on: https://go-review.googlesource.com/c/160449 Run-TryBot: Brad Fitzpatrick Reviewed-by: Brad Fitzpatrick --- src/cmd/cgo/out.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/cmd/cgo/out.go b/src/cmd/cgo/out.go index 89598c96e8..bbed5d598f 100644 --- a/src/cmd/cgo/out.go +++ b/src/cmd/cgo/out.go @@ -740,6 +740,10 @@ func (p *Package) writeExports(fgo2, fm, fgcc, fgcch io.Writer) { fmt.Fprintf(fgcc, "#include \n") fmt.Fprintf(fgcc, "#include \"_cgo_export.h\"\n\n") + // We use packed structs, but they are always aligned. + fmt.Fprintf(fgcc, "#pragma GCC diagnostic ignored \"-Wpragmas\"\n") + fmt.Fprintf(fgcc, "#pragma GCC diagnostic ignored \"-Waddress-of-packed-member\"\n") + fmt.Fprintf(fgcc, "extern void crosscall2(void (*fn)(void *, int, __SIZE_TYPE__), void *, int, __SIZE_TYPE__);\n") fmt.Fprintf(fgcc, "extern __SIZE_TYPE__ _cgo_wait_runtime_init_done();\n") fmt.Fprintf(fgcc, "extern void _cgo_release_context(__SIZE_TYPE__);\n\n") @@ -1340,6 +1344,10 @@ __cgo_size_assert(double, 8) extern char* _cgo_topofstack(void); +/* We use packed structs, but they are always aligned. */ +#pragma GCC diagnostic ignored "-Wpragmas" +#pragma GCC diagnostic ignored "-Waddress-of-packed-member" + #include #include ` -- cgit v1.2.3-54-g00ecf From 4ae9e7f1667d0a7df157b47d9d2d1f4a3c67849d Mon Sep 17 00:00:00 2001 From: Yuval Pavel Zholkover Date: Fri, 1 Feb 2019 13:51:31 +0200 Subject: [release-branch.go1.11] cmd/cgo: ignore unrecognized GCC warning group pragmas CL 159859 causes build failure with old clang versions (3.4.1) on FreeBSD 10.3/10.4. Update #29962 Reviewed-on: https://go-review.googlesource.com/c/160777 Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor (cherry picked from commit 6f4dc1ccf9735013fdb7cd044bda29d19bebb906) Change-Id: Ie78d552ea6494fe3c4059847b26c2a6e206f9515 Reviewed-on: https://go-review.googlesource.com/c/160780 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- src/cmd/cgo/out.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/cmd/cgo/out.go b/src/cmd/cgo/out.go index bbed5d598f..1c36b80088 100644 --- a/src/cmd/cgo/out.go +++ b/src/cmd/cgo/out.go @@ -741,6 +741,8 @@ func (p *Package) writeExports(fgo2, fm, fgcc, fgcch io.Writer) { fmt.Fprintf(fgcc, "#include \"_cgo_export.h\"\n\n") // We use packed structs, but they are always aligned. + // The pragmas and address-of-packed-member are not recognized as warning groups in clang 3.4.1, so ignore unknown pragmas first. + fmt.Fprintf(fgcc, "#pragma GCC diagnostic ignored \"-Wunknown-pragmas\"\n") fmt.Fprintf(fgcc, "#pragma GCC diagnostic ignored \"-Wpragmas\"\n") fmt.Fprintf(fgcc, "#pragma GCC diagnostic ignored \"-Waddress-of-packed-member\"\n") @@ -1345,6 +1347,9 @@ __cgo_size_assert(double, 8) extern char* _cgo_topofstack(void); /* We use packed structs, but they are always aligned. */ +/* The pragmas and address-of-packed-member are not recognized as warning groups in clang 3.4.1, so ignore unknown pragmas first. */ + +#pragma GCC diagnostic ignored "-Wunknown-pragmas" #pragma GCC diagnostic ignored "-Wpragmas" #pragma GCC diagnostic ignored "-Waddress-of-packed-member" -- cgit v1.2.3-54-g00ecf From 688dc859ea9cd09851c5e9157cfeba5e84c87a55 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Mon, 6 Aug 2018 15:41:34 -0400 Subject: [release-branch.go1.11] crypto/x509: fix root CA extraction on macOS (cgo path) The cgo path was not taking policies into account, using the last security setting in the array whatever it was. Also, it was not aware of the defaults for empty security settings, and for security settings without a result type. Finally, certificates restricted to a hostname were considered roots. The API docs for this code are partial and not very clear, so this is a best effort, really. Updates #24652 Updates #26039 Change-Id: I8fa2fe4706f44f3d963b32e0615d149e997b537d Reviewed-on: https://go-review.googlesource.com/c/128056 Run-TryBot: Filippo Valsorda TryBot-Result: Gobot Gobot Reviewed-by: Adam Langley Reviewed-by: Adam Langley (cherry picked from commit f6be1cf109a2be59b96d1fa913adfa1fbc628579) Reviewed-on: https://go-review.googlesource.com/c/162860 Reviewed-by: Andrew Bonventre --- src/crypto/x509/root_cgo_darwin.go | 258 ++++++++++++++++++++++++++----------- src/crypto/x509/root_darwin.go | 18 +-- 2 files changed, 192 insertions(+), 84 deletions(-) diff --git a/src/crypto/x509/root_cgo_darwin.go b/src/crypto/x509/root_cgo_darwin.go index a02ac3cfe8..a168135a33 100644 --- a/src/crypto/x509/root_cgo_darwin.go +++ b/src/crypto/x509/root_cgo_darwin.go @@ -16,7 +16,135 @@ package x509 #include #include -// FetchPEMRoots fetches the system's list of trusted X.509 root certificates. +static bool isSSLPolicy(SecPolicyRef policyRef) { + if (!policyRef) { + return false; + } + CFDictionaryRef properties = SecPolicyCopyProperties(policyRef); + if (properties == NULL) { + return false; + } + CFTypeRef value = NULL; + if (CFDictionaryGetValueIfPresent(properties, kSecPolicyOid, (const void **)&value)) { + CFRelease(properties); + return CFEqual(value, kSecPolicyAppleSSL); + } + CFRelease(properties); + return false; +} + +// sslTrustSettingsResult obtains the final kSecTrustSettingsResult value +// for a certificate in the user or admin domain, combining usage constraints +// for the SSL SecTrustSettingsPolicy, ignoring SecTrustSettingsKeyUsage and +// kSecTrustSettingsAllowedError. +// https://developer.apple.com/documentation/security/1400261-sectrustsettingscopytrustsetting +static SInt32 sslTrustSettingsResult(SecCertificateRef cert) { + CFArrayRef trustSettings = NULL; + OSStatus err = SecTrustSettingsCopyTrustSettings(cert, kSecTrustSettingsDomainUser, &trustSettings); + + // According to Apple's SecTrustServer.c, "user trust settings overrule admin trust settings", + // but the rules of the override are unclear. Let's assume admin trust settings are applicable + // if and only if user trust settings fail to load or are NULL. + if (err != errSecSuccess || trustSettings == NULL) { + if (trustSettings != NULL) CFRelease(trustSettings); + err = SecTrustSettingsCopyTrustSettings(cert, kSecTrustSettingsDomainAdmin, &trustSettings); + } + + // > no trust settings [...] means "this certificate must be verified to a known trusted certificate” + if (err != errSecSuccess || trustSettings == NULL) { + if (trustSettings != NULL) CFRelease(trustSettings); + return kSecTrustSettingsResultUnspecified; + } + + // > An empty trust settings array means "always trust this certificate” with an + // > overall trust setting for the certificate of kSecTrustSettingsResultTrustRoot. + if (CFArrayGetCount(trustSettings) == 0) { + CFRelease(trustSettings); + return kSecTrustSettingsResultTrustRoot; + } + + // kSecTrustSettingsResult is defined as CFSTR("kSecTrustSettingsResult"), + // but the Go linker's internal linking mode can't handle CFSTR relocations. + // Create our own dynamic string instead and release it below. + CFStringRef _kSecTrustSettingsResult = CFStringCreateWithCString( + NULL, "kSecTrustSettingsResult", kCFStringEncodingUTF8); + CFStringRef _kSecTrustSettingsPolicy = CFStringCreateWithCString( + NULL, "kSecTrustSettingsPolicy", kCFStringEncodingUTF8); + CFStringRef _kSecTrustSettingsPolicyString = CFStringCreateWithCString( + NULL, "kSecTrustSettingsPolicyString", kCFStringEncodingUTF8); + + CFIndex m; SInt32 result = 0; + for (m = 0; m < CFArrayGetCount(trustSettings); m++) { + CFDictionaryRef tSetting = (CFDictionaryRef)CFArrayGetValueAtIndex(trustSettings, m); + + // First, check if this trust setting applies to our policy. We assume + // only one will. The docs suggest that there might be multiple applying + // but don't explain how to combine them. + SecPolicyRef policyRef; + if (CFDictionaryGetValueIfPresent(tSetting, _kSecTrustSettingsPolicy, (const void**)&policyRef)) { + if (!isSSLPolicy(policyRef)) { + continue; + } + } else { + continue; + } + + if (CFDictionaryContainsKey(tSetting, _kSecTrustSettingsPolicyString)) { + // Restricted to a hostname, not a root. + continue; + } + + CFNumberRef cfNum; + if (CFDictionaryGetValueIfPresent(tSetting, _kSecTrustSettingsResult, (const void**)&cfNum)) { + CFNumberGetValue(cfNum, kCFNumberSInt32Type, &result); + } else { + // > If the value of the kSecTrustSettingsResult component is not + // > kSecTrustSettingsResultUnspecified for a usage constraints dictionary that has + // > no constraints, the default value kSecTrustSettingsResultTrustRoot is assumed. + result = kSecTrustSettingsResultTrustRoot; + } + + break; + } + + // If trust settings are present, but none of them match the policy... + // the docs don't tell us what to do. + // + // "Trust settings for a given use apply if any of the dictionaries in the + // certificate’s trust settings array satisfies the specified use." suggests + // that it's as if there were no trust settings at all, so we should probably + // fallback to the admin trust settings. TODO. + if (result == 0) { + result = kSecTrustSettingsResultUnspecified; + } + + CFRelease(_kSecTrustSettingsPolicy); + CFRelease(_kSecTrustSettingsPolicyString); + CFRelease(_kSecTrustSettingsResult); + CFRelease(trustSettings); + + return result; +} + +// isRootCertificate reports whether Subject and Issuer match. +static Boolean isRootCertificate(SecCertificateRef cert, CFErrorRef *errRef) { + CFDataRef subjectName = SecCertificateCopyNormalizedSubjectContent(cert, errRef); + if (*errRef != NULL) { + return false; + } + CFDataRef issuerName = SecCertificateCopyNormalizedIssuerContent(cert, errRef); + if (*errRef != NULL) { + CFRelease(subjectName); + return false; + } + Boolean equal = CFEqual(subjectName, issuerName); + CFRelease(subjectName); + CFRelease(issuerName); + return equal; +} + +// FetchPEMRoots fetches the system's list of trusted X.509 root certificates +// for the kSecTrustSettingsPolicy SSL. // // On success it returns 0 and fills pemRoots with a CFDataRef that contains the extracted root // certificates of the system. On failure, the function returns -1. @@ -24,26 +152,28 @@ package x509 // // Note: The CFDataRef returned in pemRoots and untrustedPemRoots must // be released (using CFRelease) after we've consumed its content. -int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots) { +int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots, bool debugDarwinRoots) { int i; + if (debugDarwinRoots) { + printf("crypto/x509: kSecTrustSettingsResultInvalid = %d\n", kSecTrustSettingsResultInvalid); + printf("crypto/x509: kSecTrustSettingsResultTrustRoot = %d\n", kSecTrustSettingsResultTrustRoot); + printf("crypto/x509: kSecTrustSettingsResultTrustAsRoot = %d\n", kSecTrustSettingsResultTrustAsRoot); + printf("crypto/x509: kSecTrustSettingsResultDeny = %d\n", kSecTrustSettingsResultDeny); + printf("crypto/x509: kSecTrustSettingsResultUnspecified = %d\n", kSecTrustSettingsResultUnspecified); + } + // Get certificates from all domains, not just System, this lets // the user add CAs to their "login" keychain, and Admins to add // to the "System" keychain SecTrustSettingsDomain domains[] = { kSecTrustSettingsDomainSystem, - kSecTrustSettingsDomainAdmin, - kSecTrustSettingsDomainUser }; + kSecTrustSettingsDomainAdmin, kSecTrustSettingsDomainUser }; int numDomains = sizeof(domains)/sizeof(SecTrustSettingsDomain); if (pemRoots == NULL) { return -1; } - // kSecTrustSettingsResult is defined as CFSTR("kSecTrustSettingsResult"), - // but the Go linker's internal linking mode can't handle CFSTR relocations. - // Create our own dynamic string instead and release it below. - CFStringRef policy = CFStringCreateWithCString(NULL, "kSecTrustSettingsResult", kCFStringEncodingUTF8); - CFMutableDataRef combinedData = CFDataCreateMutable(kCFAllocatorDefault, 0); CFMutableDataRef combinedUntrustedData = CFDataCreateMutable(kCFAllocatorDefault, 0); for (i = 0; i < numDomains; i++) { @@ -57,102 +187,81 @@ int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots) { CFIndex numCerts = CFArrayGetCount(certs); for (j = 0; j < numCerts; j++) { CFDataRef data = NULL; - CFErrorRef errRef = NULL; CFArrayRef trustSettings = NULL; SecCertificateRef cert = (SecCertificateRef)CFArrayGetValueAtIndex(certs, j); if (cert == NULL) { continue; } - // We only want trusted certs. - int untrusted = 0; - int trustAsRoot = 0; - int trustRoot = 0; - if (i == 0) { - trustAsRoot = 1; - } else { - int k; - CFIndex m; + SInt32 result; + if (domains[i] == kSecTrustSettingsDomainSystem) { // Certs found in the system domain are always trusted. If the user // configures "Never Trust" on such a cert, it will also be found in the // admin or user domain, causing it to be added to untrustedPemRoots. The // Go code will then clean this up. - - // Trust may be stored in any of the domains. According to Apple's - // SecTrustServer.c, "user trust settings overrule admin trust settings", - // so take the last trust settings array we find. - // Skip the system domain since it is always trusted. - for (k = i; k < numDomains; k++) { - CFArrayRef domainTrustSettings = NULL; - err = SecTrustSettingsCopyTrustSettings(cert, domains[k], &domainTrustSettings); - if (err == errSecSuccess && domainTrustSettings != NULL) { - if (trustSettings) { - CFRelease(trustSettings); - } - trustSettings = domainTrustSettings; + result = kSecTrustSettingsResultTrustRoot; + } else { + result = sslTrustSettingsResult(cert); + if (debugDarwinRoots) { + CFErrorRef errRef = NULL; + CFStringRef summary = SecCertificateCopyShortDescription(NULL, cert, &errRef); + if (errRef != NULL) { + printf("crypto/x509: SecCertificateCopyShortDescription failed\n"); + CFRelease(errRef); + continue; } - } - if (trustSettings == NULL) { - // "this certificate must be verified to a known trusted certificate"; aka not a root. - continue; - } - for (m = 0; m < CFArrayGetCount(trustSettings); m++) { - CFNumberRef cfNum; - CFDictionaryRef tSetting = (CFDictionaryRef)CFArrayGetValueAtIndex(trustSettings, m); - if (CFDictionaryGetValueIfPresent(tSetting, policy, (const void**)&cfNum)){ - SInt32 result = 0; - CFNumberGetValue(cfNum, kCFNumberSInt32Type, &result); - // TODO: The rest of the dictionary specifies conditions for evaluation. - if (result == kSecTrustSettingsResultDeny) { - untrusted = 1; - } else if (result == kSecTrustSettingsResultTrustAsRoot) { - trustAsRoot = 1; - } else if (result == kSecTrustSettingsResultTrustRoot) { - trustRoot = 1; - } + + CFIndex length = CFStringGetLength(summary); + CFIndex maxSize = CFStringGetMaximumSizeForEncoding(length, kCFStringEncodingUTF8) + 1; + char *buffer = malloc(maxSize); + if (CFStringGetCString(summary, buffer, maxSize, kCFStringEncodingUTF8)) { + printf("crypto/x509: %s returned %d\n", buffer, result); } + free(buffer); + CFRelease(summary); } - CFRelease(trustSettings); } - if (trustRoot) { - // We only want to add Root CAs, so make sure Subject and Issuer Name match - CFDataRef subjectName = SecCertificateCopyNormalizedSubjectContent(cert, &errRef); - if (errRef != NULL) { - CFRelease(errRef); - continue; - } - CFDataRef issuerName = SecCertificateCopyNormalizedIssuerContent(cert, &errRef); - if (errRef != NULL) { - CFRelease(subjectName); - CFRelease(errRef); + CFMutableDataRef appendTo; + // > Note the distinction between the results kSecTrustSettingsResultTrustRoot + // > and kSecTrustSettingsResultTrustAsRoot: The former can only be applied to + // > root (self-signed) certificates; the latter can only be applied to + // > non-root certificates. + if (result == kSecTrustSettingsResultTrustRoot) { + CFErrorRef errRef = NULL; + if (!isRootCertificate(cert, &errRef) || errRef != NULL) { + if (errRef != NULL) CFRelease(errRef); continue; } - Boolean equal = CFEqual(subjectName, issuerName); - CFRelease(subjectName); - CFRelease(issuerName); - if (!equal) { + + appendTo = combinedData; + } else if (result == kSecTrustSettingsResultTrustAsRoot) { + CFErrorRef errRef = NULL; + if (isRootCertificate(cert, &errRef) || errRef != NULL) { + if (errRef != NULL) CFRelease(errRef); continue; } + + appendTo = combinedData; + } else if (result == kSecTrustSettingsResultDeny) { + appendTo = combinedUntrustedData; + } else if (result == kSecTrustSettingsResultUnspecified) { + continue; + } else { + continue; } err = SecItemExport(cert, kSecFormatX509Cert, kSecItemPemArmour, NULL, &data); if (err != noErr) { continue; } - if (data != NULL) { - if (!trustRoot && !trustAsRoot) { - untrusted = 1; - } - CFMutableDataRef appendTo = untrusted ? combinedUntrustedData : combinedData; CFDataAppendBytes(appendTo, CFDataGetBytePtr(data), CFDataGetLength(data)); CFRelease(data); } } CFRelease(certs); } - CFRelease(policy); *pemRoots = combinedData; *untrustedPemRoots = combinedUntrustedData; return 0; @@ -169,9 +278,8 @@ func loadSystemRoots() (*CertPool, error) { var data C.CFDataRef = 0 var untrustedData C.CFDataRef = 0 - err := C.FetchPEMRoots(&data, &untrustedData) + err := C.FetchPEMRoots(&data, &untrustedData, C.bool(debugDarwinRoots)) if err == -1 { - // TODO: better error message return nil, errors.New("crypto/x509: failed to load darwin system roots with cgo") } diff --git a/src/crypto/x509/root_darwin.go b/src/crypto/x509/root_darwin.go index 9d7b3a6ffb..ae396d80a9 100644 --- a/src/crypto/x509/root_darwin.go +++ b/src/crypto/x509/root_darwin.go @@ -22,7 +22,7 @@ import ( "sync" ) -var debugExecDarwinRoots = strings.Contains(os.Getenv("GODEBUG"), "x509roots=1") +var debugDarwinRoots = strings.Contains(os.Getenv("GODEBUG"), "x509roots=1") func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate, err error) { return nil, nil @@ -58,7 +58,7 @@ func execSecurityRoots() (*CertPool, error) { if err != nil { return nil, err } - if debugExecDarwinRoots { + if debugDarwinRoots { println(fmt.Sprintf("crypto/x509: %d certs have a trust policy", len(hasPolicy))) } @@ -69,8 +69,8 @@ func execSecurityRoots() (*CertPool, error) { u, err := user.Current() if err != nil { - if debugExecDarwinRoots { - println(fmt.Sprintf("crypto/x509: get current user: %v", err)) + if debugDarwinRoots { + println(fmt.Sprintf("crypto/x509: can't get user home directory: %v", err)) } } else { args = append(args, @@ -148,7 +148,7 @@ func execSecurityRoots() (*CertPool, error) { close(blockCh) wg.Wait() - if debugExecDarwinRoots { + if debugDarwinRoots { mu.Lock() defer mu.Unlock() println(fmt.Sprintf("crypto/x509: ran security verify-cert %d times", numVerified)) @@ -176,16 +176,16 @@ func verifyCertWithSystem(block *pem.Block, cert *Certificate) bool { } cmd := exec.Command("/usr/bin/security", "verify-cert", "-c", f.Name(), "-l", "-L") var stderr bytes.Buffer - if debugExecDarwinRoots { + if debugDarwinRoots { cmd.Stderr = &stderr } if err := cmd.Run(); err != nil { - if debugExecDarwinRoots { + if debugDarwinRoots { println(fmt.Sprintf("crypto/x509: verify-cert rejected %s: %q", cert.Subject, bytes.TrimSpace(stderr.Bytes()))) } return false } - if debugExecDarwinRoots { + if debugDarwinRoots { println(fmt.Sprintf("crypto/x509: verify-cert approved %s", cert.Subject)) } return true @@ -218,7 +218,7 @@ func getCertsWithTrustPolicy() (map[string]bool, error) { // Rather than match on English substrings that are probably // localized on macOS, just interpret any failure to mean that // there are no trust settings. - if debugExecDarwinRoots { + if debugDarwinRoots { println(fmt.Sprintf("crypto/x509: exec %q: %v, %s", cmd.Args, err, stderr.Bytes())) } return nil -- cgit v1.2.3-54-g00ecf From 3705d34af14eb7aea26a2e0ce97bf6e3b09b0c28 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Mon, 6 Aug 2018 18:38:18 -0400 Subject: [release-branch.go1.11] crypto/x509: fix root CA extraction on macOS (no-cgo path) Certificates without any trust settings might still be in the keychain (for example if they used to have some, or if they are intermediates for offline verification), but they are not to be trusted. The only ones we can trust unconditionally are the ones in the system roots store. Moreover, the verify-cert invocation was not specifying the ssl policy, defaulting instead to the basic one. We have no way of communicating different usages in a CertPool, so stick to the WebPKI use-case as the primary one for crypto/x509. Updates #24652 Fixes #26039 Change-Id: Ife8b3d2f4026daa1223aa81fac44aeeb4f96528a Reviewed-on: https://go-review.googlesource.com/c/128116 Reviewed-by: Adam Langley Reviewed-by: Adam Langley (cherry picked from commit aa2415807781ba84bf917c62cb983dc1a44f2ad1) Reviewed-on: https://go-review.googlesource.com/c/162861 Run-TryBot: Filippo Valsorda TryBot-Result: Gobot Gobot Reviewed-by: Andrew Bonventre --- src/crypto/x509/root_darwin.go | 111 +++++++++++++++++++++++++---------------- 1 file changed, 67 insertions(+), 44 deletions(-) diff --git a/src/crypto/x509/root_darwin.go b/src/crypto/x509/root_darwin.go index ae396d80a9..b0460527b1 100644 --- a/src/crypto/x509/root_darwin.go +++ b/src/crypto/x509/root_darwin.go @@ -40,8 +40,8 @@ func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate // // 1. Run "security trust-settings-export" and "security // trust-settings-export -d" to discover the set of certs with some -// user-tweaked trust policy. We're too lazy to parse the XML (at -// least at this stage of Go 1.8) to understand what the trust +// user-tweaked trust policy. We're too lazy to parse the XML +// (Issue 26830) to understand what the trust // policy actually is. We just learn that there is _some_ policy. // // 2. Run "security find-certificate" to dump the list of system root @@ -59,21 +59,18 @@ func execSecurityRoots() (*CertPool, error) { return nil, err } if debugDarwinRoots { - println(fmt.Sprintf("crypto/x509: %d certs have a trust policy", len(hasPolicy))) + fmt.Printf("crypto/x509: %d certs have a trust policy\n", len(hasPolicy)) } - args := []string{"find-certificate", "-a", "-p", - "/System/Library/Keychains/SystemRootCertificates.keychain", - "/Library/Keychains/System.keychain", - } + keychains := []string{"/Library/Keychains/System.keychain"} u, err := user.Current() if err != nil { if debugDarwinRoots { - println(fmt.Sprintf("crypto/x509: can't get user home directory: %v", err)) + fmt.Printf("crypto/x509: can't get user home directory: %v\n", err) } } else { - args = append(args, + keychains = append(keychains, filepath.Join(u.HomeDir, "/Library/Keychains/login.keychain"), // Fresh installs of Sierra use a slightly different path for the login keychain @@ -81,21 +78,19 @@ func execSecurityRoots() (*CertPool, error) { ) } - cmd := exec.Command("/usr/bin/security", args...) - data, err := cmd.Output() - if err != nil { - return nil, err + type rootCandidate struct { + c *Certificate + system bool } var ( mu sync.Mutex roots = NewCertPool() numVerified int // number of execs of 'security verify-cert', for debug stats + wg sync.WaitGroup + verifyCh = make(chan rootCandidate) ) - blockCh := make(chan *pem.Block) - var wg sync.WaitGroup - // Using 4 goroutines to pipe into verify-cert seems to be // about the best we can do. The verify-cert binary seems to // just RPC to another server with coarse locking anyway, so @@ -109,31 +104,62 @@ func execSecurityRoots() (*CertPool, error) { wg.Add(1) go func() { defer wg.Done() - for block := range blockCh { - cert, err := ParseCertificate(block.Bytes) - if err != nil { - continue - } - sha1CapHex := fmt.Sprintf("%X", sha1.Sum(block.Bytes)) + for cert := range verifyCh { + sha1CapHex := fmt.Sprintf("%X", sha1.Sum(cert.c.Raw)) - valid := true + var valid bool verifyChecks := 0 if hasPolicy[sha1CapHex] { verifyChecks++ - if !verifyCertWithSystem(block, cert) { - valid = false - } + valid = verifyCertWithSystem(cert.c) + } else { + // Certificates not in SystemRootCertificates without user + // or admin trust settings are not trusted. + valid = cert.system } mu.Lock() numVerified += verifyChecks if valid { - roots.AddCert(cert) + roots.AddCert(cert.c) } mu.Unlock() } }() } + err = forEachCertInKeychains(keychains, func(cert *Certificate) { + verifyCh <- rootCandidate{c: cert, system: false} + }) + if err != nil { + close(verifyCh) + return nil, err + } + err = forEachCertInKeychains([]string{ + "/System/Library/Keychains/SystemRootCertificates.keychain", + }, func(cert *Certificate) { + verifyCh <- rootCandidate{c: cert, system: true} + }) + if err != nil { + close(verifyCh) + return nil, err + } + close(verifyCh) + wg.Wait() + + if debugDarwinRoots { + fmt.Printf("crypto/x509: ran security verify-cert %d times\n", numVerified) + } + + return roots, nil +} + +func forEachCertInKeychains(paths []string, f func(*Certificate)) error { + args := append([]string{"find-certificate", "-a", "-p"}, paths...) + cmd := exec.Command("/usr/bin/security", args...) + data, err := cmd.Output() + if err != nil { + return err + } for len(data) > 0 { var block *pem.Block block, data = pem.Decode(data) @@ -143,22 +169,19 @@ func execSecurityRoots() (*CertPool, error) { if block.Type != "CERTIFICATE" || len(block.Headers) != 0 { continue } - blockCh <- block - } - close(blockCh) - wg.Wait() - - if debugDarwinRoots { - mu.Lock() - defer mu.Unlock() - println(fmt.Sprintf("crypto/x509: ran security verify-cert %d times", numVerified)) + cert, err := ParseCertificate(block.Bytes) + if err != nil { + continue + } + f(cert) } - - return roots, nil + return nil } -func verifyCertWithSystem(block *pem.Block, cert *Certificate) bool { - data := pem.EncodeToMemory(block) +func verifyCertWithSystem(cert *Certificate) bool { + data := pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", Bytes: cert.Raw, + }) f, err := ioutil.TempFile("", "cert") if err != nil { @@ -174,19 +197,19 @@ func verifyCertWithSystem(block *pem.Block, cert *Certificate) bool { fmt.Fprintf(os.Stderr, "can't write temporary file for cert: %v", err) return false } - cmd := exec.Command("/usr/bin/security", "verify-cert", "-c", f.Name(), "-l", "-L") + cmd := exec.Command("/usr/bin/security", "verify-cert", "-p", "ssl", "-c", f.Name(), "-l", "-L") var stderr bytes.Buffer if debugDarwinRoots { cmd.Stderr = &stderr } if err := cmd.Run(); err != nil { if debugDarwinRoots { - println(fmt.Sprintf("crypto/x509: verify-cert rejected %s: %q", cert.Subject, bytes.TrimSpace(stderr.Bytes()))) + fmt.Printf("crypto/x509: verify-cert rejected %s: %q\n", cert.Subject, bytes.TrimSpace(stderr.Bytes())) } return false } if debugDarwinRoots { - println(fmt.Sprintf("crypto/x509: verify-cert approved %s", cert.Subject)) + fmt.Printf("crypto/x509: verify-cert approved %s\n", cert.Subject) } return true } @@ -219,7 +242,7 @@ func getCertsWithTrustPolicy() (map[string]bool, error) { // localized on macOS, just interpret any failure to mean that // there are no trust settings. if debugDarwinRoots { - println(fmt.Sprintf("crypto/x509: exec %q: %v, %s", cmd.Args, err, stderr.Bytes())) + fmt.Printf("crypto/x509: exec %q: %v, %s\n", cmd.Args, err, stderr.Bytes()) } return nil } -- cgit v1.2.3-54-g00ecf From c8c897a67066ef9e6011933ea5fead3bfa6b2333 Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Wed, 2 Jan 2019 12:27:55 -0500 Subject: [release-branch.go1.11] cmd/compile: fix deriving from x+d >= w on overflow in prove pass In the case of x+d >= w, where d and w are constants, we are deriving x is within the bound of min=w-d and max=maxInt-d. When there is an overflow (min >= max), we know only one of x >= min or x <= max is true, and we derive this by excluding the other. When excluding x >= min, we did not consider the equal case, so we could incorrectly derive x <= max when x == min. Updates #29502. Fixes #29503. Change-Id: Ia9f7d814264b1a3ddf78f52e2ce23377450e6e8a Reviewed-on: https://go-review.googlesource.com/c/156019 Reviewed-by: David Chase (cherry picked from commit 2e217fa726a624093eea5b099d1531c79e27a423) Reviewed-on: https://go-review.googlesource.com/c/163724 Run-TryBot: Cherry Zhang Reviewed-by: Brad Fitzpatrick TryBot-Result: Gobot Gobot --- src/cmd/compile/internal/ssa/prove.go | 19 +++++++++++++++---- test/prove.go | 14 ++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/cmd/compile/internal/ssa/prove.go b/src/cmd/compile/internal/ssa/prove.go index af2b9ef0ed..bddc1abe43 100644 --- a/src/cmd/compile/internal/ssa/prove.go +++ b/src/cmd/compile/internal/ssa/prove.go @@ -197,6 +197,9 @@ func newFactsTable(f *Func) *factsTable { // update updates the set of relations between v and w in domain d // restricting it to r. func (ft *factsTable) update(parent *Block, v, w *Value, d domain, r relation) { + if parent.Func.pass.debug > 2 { + parent.Func.Warnl(parent.Pos, "parent=%s, update %s %s %s", parent, v, w, r) + } // No need to do anything else if we already found unsat. if ft.unsat { return @@ -234,6 +237,9 @@ func (ft *factsTable) update(parent *Block, v, w *Value, d domain, r relation) { panic("unknown relation") } if !ok { + if parent.Func.pass.debug > 2 { + parent.Func.Warnl(parent.Pos, "unsat %s %s %s", v, w, r) + } ft.unsat = true return } @@ -260,6 +266,9 @@ func (ft *factsTable) update(parent *Block, v, w *Value, d domain, r relation) { ft.facts[p] = oldR & r // If this relation is not satisfiable, mark it and exit right away if oldR&r == 0 { + if parent.Func.pass.debug > 2 { + parent.Func.Warnl(parent.Pos, "unsat %s %s %s", v, w, r) + } ft.unsat = true return } @@ -361,7 +370,7 @@ func (ft *factsTable) update(parent *Block, v, w *Value, d domain, r relation) { lim = old.intersect(lim) ft.limits[v.ID] = lim if v.Block.Func.pass.debug > 2 { - v.Block.Func.Warnl(parent.Pos, "parent=%s, new limits %s %s %s", parent, v, w, lim.String()) + v.Block.Func.Warnl(parent.Pos, "parent=%s, new limits %s %s %s %s", parent, v, w, r, lim.String()) } if lim.min > lim.max || lim.umin > lim.umax { ft.unsat = true @@ -442,7 +451,7 @@ func (ft *factsTable) update(parent *Block, v, w *Value, d domain, r relation) { if r == gt || r == gt|eq { if x, delta := isConstDelta(v); x != nil && d == signed { if parent.Func.pass.debug > 1 { - parent.Func.Warnl(parent.Pos, "x+d >= w; x:%v %v delta:%v w:%v d:%v", x, parent.String(), delta, w.AuxInt, d) + parent.Func.Warnl(parent.Pos, "x+d %s w; x:%v %v delta:%v w:%v d:%v", r, x, parent.String(), delta, w.AuxInt, d) } if !w.isGenericIntConst() { // If we know that x+delta > w but w is not constant, we can derive: @@ -503,8 +512,10 @@ func (ft *factsTable) update(parent *Block, v, w *Value, d domain, r relation) { // the other must be true if l, has := ft.limits[x.ID]; has { if l.max <= min { - // x>min is impossible, so it must be x<=max - ft.update(parent, vmax, x, d, r|eq) + if r&eq == 0 || l.max < min { + // x>min (x>=min) is impossible, so it must be x<=max + ft.update(parent, vmax, x, d, r|eq) + } } else if l.min > max { // x<=max is impossible, so it must be x>min ft.update(parent, x, vmin, d, r) diff --git a/test/prove.go b/test/prove.go index 0de6bd63b4..ce6c02ffc3 100644 --- a/test/prove.go +++ b/test/prove.go @@ -488,6 +488,20 @@ func f18(b []int, x int, y uint) { } } +func f19() (e int64, err error) { + // Issue 29502: slice[:0] is incorrectly disproved. + var stack []int64 + stack = append(stack, 123) + if len(stack) > 1 { + panic("too many elements") + } + last := len(stack) - 1 + e = stack[last] + // Buggy compiler prints "Disproved Geq64" for the next line. + stack = stack[:last] // ERROR "Proved IsSliceInBounds" + return e, nil +} + func sm1(b []int, x int) { // Test constant argument to slicemask. useSlice(b[2:8]) // ERROR "Proved slicemask not needed$" -- cgit v1.2.3-54-g00ecf From aa95a1eb5a3423d96873946d47c663bdc8f3565e Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Mon, 4 Feb 2019 18:08:43 -0500 Subject: [release-branch.go1.11] crypto/x509: consider parents by Subject if AKID has no match If a certificate somehow has an AKID, it should still chain successfully to a parent without a SKID, even if the latter is invalid according to RFC 5280, because only the Subject is authoritative. This reverts to the behavior before #29233 was fixed in 770130659. Roots with the right subject will still be shadowed by roots with the right SKID and the wrong subject, but that's been the case for a long time, and is left for a more complete fix in Go 1.13. Updates #30079 Fixes #30081 Change-Id: If8ab0179aca86cb74caa926d1ef93fb5e416b4bb Reviewed-on: https://go-review.googlesource.com/c/161097 Reviewed-by: Adam Langley (cherry picked from commit 95e5b07cf5fdf3352f04f5557df38f22c55ce8e8) Reviewed-on: https://go-review.googlesource.com/c/163739 Run-TryBot: Filippo Valsorda TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- src/crypto/x509/cert_pool.go | 9 +++- src/crypto/x509/verify_test.go | 116 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) diff --git a/src/crypto/x509/cert_pool.go b/src/crypto/x509/cert_pool.go index 61107f86b8..1229fb43d6 100644 --- a/src/crypto/x509/cert_pool.go +++ b/src/crypto/x509/cert_pool.go @@ -71,10 +71,15 @@ func (s *CertPool) findPotentialParents(cert *Certificate) []int { if s == nil { return nil } + + var candidates []int if len(cert.AuthorityKeyId) > 0 { - return s.bySubjectKeyId[string(cert.AuthorityKeyId)] + candidates = s.bySubjectKeyId[string(cert.AuthorityKeyId)] + } + if len(candidates) == 0 { + candidates = s.byName[string(cert.RawIssuer)] } - return s.byName[string(cert.RawIssuer)] + return candidates } func (s *CertPool) contains(cert *Certificate) bool { diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go index 85f4703d4c..86fe76a57d 100644 --- a/src/crypto/x509/verify_test.go +++ b/src/crypto/x509/verify_test.go @@ -386,6 +386,19 @@ var verifyTests = []verifyTest{ errorCallback: expectHostnameError("not valid for any names"), }, + { + // A certificate with an AKID should still chain to a parent without SKID. + // See Issue 30079. + leaf: leafWithAKID, + roots: []string{rootWithoutSKID}, + currentTime: 1550000000, + dnsName: "example", + systemSkip: true, + + expectedChains: [][]string{ + {"Acme LLC", "Acme Co"}, + }, + }, } func expectHostnameError(msg string) func(*testing.T, int, error) bool { @@ -1679,6 +1692,109 @@ h7olHCpY9yMRiz0= -----END CERTIFICATE----- ` +const ( + rootWithoutSKID = ` +Certificate: + Data: + Version: 3 (0x2) + Serial Number: + 78:29:2a:dc:2f:12:39:7f:c9:33:93:ea:61:39:7d:70 + Signature Algorithm: ecdsa-with-SHA256 + Issuer: O = Acme Co + Validity + Not Before: Feb 4 22:56:34 2019 GMT + Not After : Feb 1 22:56:34 2029 GMT + Subject: O = Acme Co + Subject Public Key Info: + Public Key Algorithm: id-ecPublicKey + Public-Key: (256 bit) + pub: + 04:84:a6:8c:69:53:af:87:4b:39:64:fe:04:24:e6: + d8:fc:d6:46:39:35:0e:92:dc:48:08:7e:02:5f:1e: + 07:53:5c:d9:e0:56:c5:82:07:f6:a3:e2:ad:f6:ad: + be:a0:4e:03:87:39:67:0c:9c:46:91:68:6b:0e:8e: + f8:49:97:9d:5b + ASN1 OID: prime256v1 + NIST CURVE: P-256 + X509v3 extensions: + X509v3 Key Usage: critical + Digital Signature, Key Encipherment, Certificate Sign + X509v3 Extended Key Usage: + TLS Web Server Authentication + X509v3 Basic Constraints: critical + CA:TRUE + X509v3 Subject Alternative Name: + DNS:example + Signature Algorithm: ecdsa-with-SHA256 + 30:46:02:21:00:c6:81:61:61:42:8d:37:e7:d0:c3:72:43:44: + 17:bd:84:ff:88:81:68:9a:99:08:ab:3c:3a:c0:1e:ea:8c:ba: + c0:02:21:00:de:c9:fa:e5:5e:c6:e2:db:23:64:43:a9:37:42: + 72:92:7f:6e:89:38:ea:9e:2a:a7:fd:2f:ea:9a:ff:20:21:e7 +-----BEGIN CERTIFICATE----- +MIIBbzCCARSgAwIBAgIQeCkq3C8SOX/JM5PqYTl9cDAKBggqhkjOPQQDAjASMRAw +DgYDVQQKEwdBY21lIENvMB4XDTE5MDIwNDIyNTYzNFoXDTI5MDIwMTIyNTYzNFow +EjEQMA4GA1UEChMHQWNtZSBDbzBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABISm +jGlTr4dLOWT+BCTm2PzWRjk1DpLcSAh+Al8eB1Nc2eBWxYIH9qPirfatvqBOA4c5 +ZwycRpFoaw6O+EmXnVujTDBKMA4GA1UdDwEB/wQEAwICpDATBgNVHSUEDDAKBggr +BgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MBIGA1UdEQQLMAmCB2V4YW1wbGUwCgYI +KoZIzj0EAwIDSQAwRgIhAMaBYWFCjTfn0MNyQ0QXvYT/iIFompkIqzw6wB7qjLrA +AiEA3sn65V7G4tsjZEOpN0Jykn9uiTjqniqn/S/qmv8gIec= +-----END CERTIFICATE----- +` + leafWithAKID = ` + Certificate: + Data: + Version: 3 (0x2) + Serial Number: + f0:8a:62:f0:03:84:a2:cf:69:63:ad:71:3b:b6:5d:8c + Signature Algorithm: ecdsa-with-SHA256 + Issuer: O = Acme Co + Validity + Not Before: Feb 4 23:06:52 2019 GMT + Not After : Feb 1 23:06:52 2029 GMT + Subject: O = Acme LLC + Subject Public Key Info: + Public Key Algorithm: id-ecPublicKey + Public-Key: (256 bit) + pub: + 04:5a:4e:4d:fb:ff:17:f7:b6:13:e8:29:45:34:81: + 39:ff:8c:9c:d9:8c:0a:9f:dd:b5:97:4c:2b:20:91: + 1c:4f:6b:be:53:27:66:ec:4a:ad:08:93:6d:66:36: + 0c:02:70:5d:01:ca:7f:c3:29:e9:4f:00:ba:b4:14: + ec:c5:c3:34:b3 + ASN1 OID: prime256v1 + NIST CURVE: P-256 + X509v3 extensions: + X509v3 Key Usage: critical + Digital Signature, Key Encipherment + X509v3 Extended Key Usage: + TLS Web Server Authentication + X509v3 Basic Constraints: critical + CA:FALSE + X509v3 Authority Key Identifier: + keyid:C2:2B:5F:91:78:34:26:09:42:8D:6F:51:B2:C5:AF:4C:0B:DE:6A:42 + + X509v3 Subject Alternative Name: + DNS:example + Signature Algorithm: ecdsa-with-SHA256 + 30:44:02:20:64:e0:ba:56:89:63:ce:22:5e:4f:22:15:fd:3c: + 35:64:9a:3a:6b:7b:9a:32:a0:7f:f7:69:8c:06:f0:00:58:b8: + 02:20:09:e4:9f:6d:8b:9e:38:e1:b6:01:d5:ee:32:a4:94:65: + 93:2a:78:94:bb:26:57:4b:c7:dd:6c:3d:40:2b:63:90 +-----BEGIN CERTIFICATE----- +MIIBjTCCATSgAwIBAgIRAPCKYvADhKLPaWOtcTu2XYwwCgYIKoZIzj0EAwIwEjEQ +MA4GA1UEChMHQWNtZSBDbzAeFw0xOTAyMDQyMzA2NTJaFw0yOTAyMDEyMzA2NTJa +MBMxETAPBgNVBAoTCEFjbWUgTExDMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE +Wk5N+/8X97YT6ClFNIE5/4yc2YwKn921l0wrIJEcT2u+Uydm7EqtCJNtZjYMAnBd +Acp/wynpTwC6tBTsxcM0s6NqMGgwDgYDVR0PAQH/BAQDAgWgMBMGA1UdJQQMMAoG +CCsGAQUFBwMBMAwGA1UdEwEB/wQCMAAwHwYDVR0jBBgwFoAUwitfkXg0JglCjW9R +ssWvTAveakIwEgYDVR0RBAswCYIHZXhhbXBsZTAKBggqhkjOPQQDAgNHADBEAiBk +4LpWiWPOIl5PIhX9PDVkmjpre5oyoH/3aYwG8ABYuAIgCeSfbYueOOG2AdXuMqSU +ZZMqeJS7JldLx91sPUArY5A= +-----END CERTIFICATE----- +` +) + var unknownAuthorityErrorTests = []struct { cert string expected string -- cgit v1.2.3-54-g00ecf From 8390781ca35ac5874eb5b136cfc29bb47adee94b Mon Sep 17 00:00:00 2001 From: Tobias Klauser Date: Thu, 6 Dec 2018 20:58:26 +0100 Subject: [release-branch.go1.11] crypto/x509: explicitly cast printf format argument After CL 128056 the build fails on darwin/386 with src/crypto/x509/root_cgo_darwin.go:218:55: warning: values of type 'SInt32' should not be used as format arguments; add an explicit cast to 'int' instead [-Wformat] go build crypto/x509: C compiler warning promoted to error on Go builders Fix the warning by explicitly casting the argument to an int as suggested by the warning. Fixes #30444 Change-Id: Icb6bd622a543e9bc5f669fd3d7abd418b4a8e579 Reviewed-on: https://go-review.googlesource.com/c/152958 Run-TryBot: Tobias Klauser TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor (cherry picked from commit ec0077c54d6261ba5cbab2c5dc2e80345068233f) Reviewed-on: https://go-review.googlesource.com/c/164240 Run-TryBot: Filippo Valsorda Reviewed-by: Brad Fitzpatrick --- src/crypto/x509/root_cgo_darwin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crypto/x509/root_cgo_darwin.go b/src/crypto/x509/root_cgo_darwin.go index a168135a33..e6332072d6 100644 --- a/src/crypto/x509/root_cgo_darwin.go +++ b/src/crypto/x509/root_cgo_darwin.go @@ -215,7 +215,7 @@ int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots, bool debugD CFIndex maxSize = CFStringGetMaximumSizeForEncoding(length, kCFStringEncodingUTF8) + 1; char *buffer = malloc(maxSize); if (CFStringGetCString(summary, buffer, maxSize, kCFStringEncodingUTF8)) { - printf("crypto/x509: %s returned %d\n", buffer, result); + printf("crypto/x509: %s returned %d\n", buffer, (int)result); } free(buffer); CFRelease(summary); -- cgit v1.2.3-54-g00ecf From 6a7a8ba07fb22b006fb1af4c9413d20f0257a32e Mon Sep 17 00:00:00 2001 From: Andrew Bonventre Date: Thu, 14 Mar 2019 13:58:47 -0400 Subject: [release-branch.go1.11] doc: document Go 1.11.6 Change-Id: I99832fa4f2c3ec28e2dad46cf7607f3766948031 Reviewed-on: https://go-review.googlesource.com/c/go/+/167698 Reviewed-by: Brad Fitzpatrick (cherry picked from commit d3bb45d9046bb7d12c4fc9cdaf122f36d001fd31) Reviewed-on: https://go-review.googlesource.com/c/go/+/167700 --- doc/devel/release.html | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/devel/release.html b/doc/devel/release.html index 226148e93b..3b6a635a68 100644 --- a/doc/devel/release.html +++ b/doc/devel/release.html @@ -73,6 +73,14 @@ See the Go +1.11.6 milestone on our issue tracker for details. +

+

go1.10 (released 2018/02/16)

-- cgit v1.2.3-54-g00ecf From 6e6462315b628bc364bbdf68da64322763f17b81 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Thu, 20 Dec 2018 20:21:45 +0000 Subject: [release-branch.go1.11] runtime: skip TestLockOSThreadAvoidsStatePropagation if one can't unshare This change splits a testprog out of TestLockOSThreadExit and makes it its own test. Then, this change makes the testprog exit prematurely with a special message if unshare fails with EPERM because not all of the builders allow the user to call the unshare syscall. Also, do some minor cleanup on the TestLockOSThread* tests. Fixes #29366. Change-Id: Id8a9f6c4b16e26af92ed2916b90b0249ba226dbe Reviewed-on: https://go-review.googlesource.com/c/155437 Run-TryBot: Michael Knyszek TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick (cherry picked from commit 429bae715876c69853bb63db1733f580e293c916) Reviewed-on: https://go-review.googlesource.com/c/go/+/167707 Run-TryBot: Andrew Bonventre Reviewed-by: Michael Knyszek --- src/runtime/proc_test.go | 21 +++++---- src/runtime/testdata/testprog/lockosthread.go | 4 ++ src/runtime/testdata/testprog/syscalls.go | 47 +------------------- src/runtime/testdata/testprog/syscalls_linux.go | 59 +++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 53 deletions(-) create mode 100644 src/runtime/testdata/testprog/syscalls_linux.go diff --git a/src/runtime/proc_test.go b/src/runtime/proc_test.go index e6947d5849..1715324aa0 100644 --- a/src/runtime/proc_test.go +++ b/src/runtime/proc_test.go @@ -885,23 +885,28 @@ func TestLockOSThreadNesting(t *testing.T) { func TestLockOSThreadExit(t *testing.T) { testLockOSThreadExit(t, "testprog") - - want := "OK\n" - output := runTestProg(t, "testprog", "LockOSThreadAvoidsStatePropagation", "GOMAXPROCS=1") - if output != want { - t.Errorf("want %s, got %s\n", want, output) - } } func testLockOSThreadExit(t *testing.T, prog string) { output := runTestProg(t, prog, "LockOSThreadMain", "GOMAXPROCS=1") want := "OK\n" if output != want { - t.Errorf("want %s, got %s\n", want, output) + t.Errorf("want %q, got %q", want, output) } output = runTestProg(t, prog, "LockOSThreadAlt") if output != want { - t.Errorf("want %s, got %s\n", want, output) + t.Errorf("want %q, got %q", want, output) + } +} + +func TestLockOSThreadAvoidsStatePropagation(t *testing.T) { + want := "OK\n" + skip := "unshare not permitted\n" + output := runTestProg(t, "testprog", "LockOSThreadAvoidsStatePropagation", "GOMAXPROCS=1") + if output == skip { + t.Skip("unshare syscall not permitted on this system") + } else if output != want { + t.Errorf("want %q, got %q", want, output) } } diff --git a/src/runtime/testdata/testprog/lockosthread.go b/src/runtime/testdata/testprog/lockosthread.go index 5119cf8131..fd3123e647 100644 --- a/src/runtime/testdata/testprog/lockosthread.go +++ b/src/runtime/testdata/testprog/lockosthread.go @@ -144,6 +144,10 @@ func LockOSThreadAvoidsStatePropagation() { // the rest of the process on this thread. // On systems other than Linux, this is a no-op. if err := unshareFs(); err != nil { + if err == errNotPermitted { + println("unshare not permitted") + os.Exit(0) + } println("failed to unshare fs:", err.Error()) os.Exit(1) } diff --git a/src/runtime/testdata/testprog/syscalls.go b/src/runtime/testdata/testprog/syscalls.go index 08284fc561..098d5cadf8 100644 --- a/src/runtime/testdata/testprog/syscalls.go +++ b/src/runtime/testdata/testprog/syscalls.go @@ -2,53 +2,10 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// +build linux - package main import ( - "bytes" - "fmt" - "io/ioutil" - "os" - "syscall" + "errors" ) -func gettid() int { - return syscall.Gettid() -} - -func tidExists(tid int) (exists, supported bool) { - stat, err := ioutil.ReadFile(fmt.Sprintf("/proc/self/task/%d/stat", tid)) - if os.IsNotExist(err) { - return false, true - } - // Check if it's a zombie thread. - state := bytes.Fields(stat)[2] - return !(len(state) == 1 && state[0] == 'Z'), true -} - -func getcwd() (string, error) { - if !syscall.ImplementsGetwd { - return "", nil - } - // Use the syscall to get the current working directory. - // This is imperative for checking for OS thread state - // after an unshare since os.Getwd might just check the - // environment, or use some other mechanism. - var buf [4096]byte - n, err := syscall.Getcwd(buf[:]) - if err != nil { - return "", err - } - // Subtract one for null terminator. - return string(buf[:n-1]), nil -} - -func unshareFs() error { - return syscall.Unshare(syscall.CLONE_FS) -} - -func chdir(path string) error { - return syscall.Chdir(path) -} +var errNotPermitted = errors.New("operation not permitted") diff --git a/src/runtime/testdata/testprog/syscalls_linux.go b/src/runtime/testdata/testprog/syscalls_linux.go new file mode 100644 index 0000000000..b8ac087626 --- /dev/null +++ b/src/runtime/testdata/testprog/syscalls_linux.go @@ -0,0 +1,59 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "bytes" + "fmt" + "io/ioutil" + "os" + "syscall" +) + +func gettid() int { + return syscall.Gettid() +} + +func tidExists(tid int) (exists, supported bool) { + stat, err := ioutil.ReadFile(fmt.Sprintf("/proc/self/task/%d/stat", tid)) + if os.IsNotExist(err) { + return false, true + } + // Check if it's a zombie thread. + state := bytes.Fields(stat)[2] + return !(len(state) == 1 && state[0] == 'Z'), true +} + +func getcwd() (string, error) { + if !syscall.ImplementsGetwd { + return "", nil + } + // Use the syscall to get the current working directory. + // This is imperative for checking for OS thread state + // after an unshare since os.Getwd might just check the + // environment, or use some other mechanism. + var buf [4096]byte + n, err := syscall.Getcwd(buf[:]) + if err != nil { + return "", err + } + // Subtract one for null terminator. + return string(buf[:n-1]), nil +} + +func unshareFs() error { + err := syscall.Unshare(syscall.CLONE_FS) + if err != nil { + errno, ok := err.(syscall.Errno) + if ok && errno == syscall.EPERM { + return errNotPermitted + } + } + return err +} + +func chdir(path string) error { + return syscall.Chdir(path) +} -- cgit v1.2.3-54-g00ecf From e18f2ca380f52bbf8cac039ccfdf445e9047c810 Mon Sep 17 00:00:00 2001 From: Andrew Bonventre Date: Thu, 14 Mar 2019 15:33:37 -0400 Subject: [release-branch.go1.11] go1.11.6 Change-Id: I944d7cb825b8791486446d78feae9eed0a5479c4 Reviewed-on: https://go-review.googlesource.com/c/go/+/167705 Run-TryBot: Andrew Bonventre TryBot-Result: Gobot Gobot Reviewed-by: Andrew Bonventre --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 04364e1646..291f4083bb 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.11.5 \ No newline at end of file +go1.11.6 \ No newline at end of file -- cgit v1.2.3-54-g00ecf