From 88be85f18bf0244a2470fdf6719e1b5ca5a5e50a Mon Sep 17 00:00:00 2001 From: Cherry Mui Date: Thu, 3 Mar 2022 13:35:44 -0500 Subject: [release-branch.go1.17] runtime: count spill slot for frame size at finalizer call The finalizer is called using reflectcall. When register ABI is used, the finalizer's argument is passed in register(s). But the frame size calculation does not include the spill slot. When the argument actually spills, it may clobber the caller's stack frame. This CL fixes it. Updates #51457. Fixes #51458. Change-Id: Ibcc7507c518ba65c1c5a7759e5cab0ae3fc7efce Reviewed-on: https://go-review.googlesource.com/c/go/+/389574 Trust: Cherry Mui Run-TryBot: Cherry Mui TryBot-Result: Gopher Robot Reviewed-by: Michael Knyszek (cherry picked from commit 58804ea67a28c1d8e37ed548b685bc0c09638886) Reviewed-on: https://go-review.googlesource.com/c/go/+/389794 --- src/runtime/mfinal.go | 24 +++++++++--------------- src/runtime/mfinal_test.go | 9 +++++++++ 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/runtime/mfinal.go b/src/runtime/mfinal.go index c134a0f22d..a6653032d7 100644 --- a/src/runtime/mfinal.go +++ b/src/runtime/mfinal.go @@ -187,21 +187,15 @@ func runfinq() { f := &fb.fin[i-1] var regs abi.RegArgs - var framesz uintptr - if argRegs > 0 { - // The args can always be passed in registers if they're - // available, because platforms we support always have no - // argument registers available, or more than 2. - // - // But unfortunately because we can have an arbitrary - // amount of returns and it would be complex to try and - // figure out how many of those can get passed in registers, - // just conservatively assume none of them do. - framesz = f.nret - } else { - // Need to pass arguments on the stack too. - framesz = unsafe.Sizeof((interface{})(nil)) + f.nret - } + // The args may be passed in registers or on stack. Even for + // the register case, we still need the spill slots. + // TODO: revisit if we remove spill slots. + // + // Unfortunately because we can have an arbitrary + // amount of returns and it would be complex to try and + // figure out how many of those can get passed in registers, + // just conservatively assume none of them do. + framesz := unsafe.Sizeof((interface{})(nil)) + f.nret if framecap < framesz { // The frame does not contain pointers interesting for GC, // all not yet finalized objects are stored in finq. diff --git a/src/runtime/mfinal_test.go b/src/runtime/mfinal_test.go index 3ca8d31c60..8827d55af1 100644 --- a/src/runtime/mfinal_test.go +++ b/src/runtime/mfinal_test.go @@ -42,6 +42,15 @@ func TestFinalizerType(t *testing.T) { {func(x *int) interface{} { return Tintptr(x) }, func(v *int) { finalize(v) }}, {func(x *int) interface{} { return (*Tint)(x) }, func(v *Tint) { finalize((*int)(v)) }}, {func(x *int) interface{} { return (*Tint)(x) }, func(v Tinter) { finalize((*int)(v.(*Tint))) }}, + // Test case for argument spill slot. + // If the spill slot was not counted for the frame size, it will (incorrectly) choose + // call32 as the result has (exactly) 32 bytes. When the argument actually spills, + // it clobbers the caller's frame (likely the return PC). + {func(x *int) interface{} { return x }, func(v interface{}) [4]int64 { + print() // force spill + finalize(v.(*int)) + return [4]int64{} + }}, } for i, tt := range finalizerTests { -- cgit v1.2.3-54-g00ecf From efed283e795c7f89fa4432b4addcd68aee0340fa Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Tue, 15 Mar 2022 21:05:11 -0700 Subject: [release-branch.go1.17] runtime: call testenv.MustHaveCGO in a couple of tests For #51695 Fixes #51696 Change-Id: Icfe9d26ecc28a7db9040d50d4661cf9e8245471e Reviewed-on: https://go-review.googlesource.com/c/go/+/392916 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Gopher Robot Reviewed-by: Bryan Mills (cherry picked from commit 2d32594396b231b39d09ec21d34b22b0270268b5) Reviewed-on: https://go-review.googlesource.com/c/go/+/393698 Reviewed-by: Emmanuel Odeke --- src/runtime/signal_windows_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/runtime/signal_windows_test.go b/src/runtime/signal_windows_test.go index 1b7cb9d4c4..14b55fe01b 100644 --- a/src/runtime/signal_windows_test.go +++ b/src/runtime/signal_windows_test.go @@ -25,6 +25,7 @@ func TestVectoredHandlerDontCrashOnLibrary(t *testing.T) { t.Skip("this test can only run on windows/amd64") } testenv.MustHaveGoBuild(t) + testenv.MustHaveCGO(t) testenv.MustHaveExecPath(t, "gcc") testprog.Lock() defer testprog.Unlock() @@ -149,6 +150,7 @@ func TestLibraryCtrlHandler(t *testing.T) { t.Skip("this test can only run on windows/amd64") } testenv.MustHaveGoBuild(t) + testenv.MustHaveCGO(t) testenv.MustHaveExecPath(t, "gcc") testprog.Lock() defer testprog.Unlock() -- cgit v1.2.3-54-g00ecf From 4e69fddc640c727865490706633833870408e6ff Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 12 Jan 2022 17:22:09 -0500 Subject: [release-branch.go1.17] runtime: fix net poll races The netpoll code was written long ago, when the only multiprocessors that Go ran on were x86. It assumed that an atomic store would trigger a full memory barrier and then used that barrier to order otherwise racy access to a handful of fields, including pollDesc.closing. On ARM64, this code has finally failed, because the atomic store is on a value completely unrelated to any of the racily-accessed fields, and the ARMv8 hardware, unlike x86, is clever enough not to do a full memory barrier for a simple atomic store. We are seeing a constant background rate of trybot failures where the net/http tests deadlock - a netpollblock has clearly happened after the pollDesc has begun to close. The code that does the racy reads is netpollcheckerr, which needs to be able to run without acquiring a lock. This CL fixes the race, without introducing unnecessary inefficiency or deadlock, by arranging for every updater of the relevant fields to publish a summary as a single atomic uint32, and then having netpollcheckerr use a single atomic load to fetch the relevant bits and then proceed as before. For #45211 Fixes #50611 Change-Id: Ib6788c8da4d00b7bda84d55ca3fdffb5a64c1a0a Reviewed-on: https://go-review.googlesource.com/c/go/+/378234 Trust: Russ Cox Run-TryBot: Russ Cox Trust: Bryan Mills Reviewed-by: Ian Lance Taylor (cherry picked from commit 17b2fb1b656a275906b5071c562439d50a27f167) Reviewed-on: https://go-review.googlesource.com/c/go/+/392714 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Gopher Robot Reviewed-by: Emmanuel Odeke --- src/runtime/netpoll.go | 129 +++++++++++++++++++++++++++++++---------- src/runtime/netpoll_aix.go | 5 +- src/runtime/netpoll_epoll.go | 5 +- src/runtime/netpoll_kqueue.go | 5 +- src/runtime/netpoll_solaris.go | 2 +- src/runtime/runtime2.go | 2 + 6 files changed, 105 insertions(+), 43 deletions(-) diff --git a/src/runtime/netpoll.go b/src/runtime/netpoll.go index 4f8d24427c..72e47bfd29 100644 --- a/src/runtime/netpoll.go +++ b/src/runtime/netpoll.go @@ -72,31 +72,99 @@ const pollBlockSize = 4 * 1024 //go:notinheap type pollDesc struct { link *pollDesc // in pollcache, protected by pollcache.lock + fd uintptr // constant for pollDesc usage lifetime + + // atomicInfo holds bits from closing, rd, and wd, + // which are only ever written while holding the lock, + // summarized for use by netpollcheckerr, + // which cannot acquire the lock. + // After writing these fields under lock in a way that + // might change the summary, code must call publishInfo + // before releasing the lock. + // Code that changes fields and then calls netpollunblock + // (while still holding the lock) must call publishInfo + // before calling netpollunblock, because publishInfo is what + // stops netpollblock from blocking anew + // (by changing the result of netpollcheckerr). + // atomicInfo also holds the eventErr bit, + // recording whether a poll event on the fd got an error; + // atomicInfo is the only source of truth for that bit. + atomicInfo uint32 // atomic pollInfo + + // rg, wg are accessed atomically and hold g pointers. + // (Using atomic.Uintptr here is similar to using guintptr elsewhere.) + rg uintptr // pdReady, pdWait, G waiting for read or nil + wg uintptr // pdReady, pdWait, G waiting for write or nil - // The lock protects pollOpen, pollSetDeadline, pollUnblock and deadlineimpl operations. - // This fully covers seq, rt and wt variables. fd is constant throughout the PollDesc lifetime. - // pollReset, pollWait, pollWaitCanceled and runtime·netpollready (IO readiness notification) - // proceed w/o taking the lock. So closing, everr, rg, rd, wg and wd are manipulated - // in a lock-free way by all operations. - // TODO(golang.org/issue/49008): audit these lock-free fields for continued correctness. - // NOTE(dvyukov): the following code uses uintptr to store *g (rg/wg), - // that will blow up when GC starts moving objects. lock mutex // protects the following fields - fd uintptr closing bool - everr bool // marks event scanning error happened user uint32 // user settable cookie rseq uintptr // protects from stale read timers - rg uintptr // pdReady, pdWait, G waiting for read or nil. Accessed atomically. rt timer // read deadline timer (set if rt.f != nil) - rd int64 // read deadline + rd int64 // read deadline (a nanotime in the future, -1 when expired) wseq uintptr // protects from stale write timers - wg uintptr // pdReady, pdWait, G waiting for write or nil. Accessed atomically. wt timer // write deadline timer - wd int64 // write deadline + wd int64 // write deadline (a nanotime in the future, -1 when expired) self *pollDesc // storage for indirect interface. See (*pollDesc).makeArg. } +// pollInfo is the bits needed by netpollcheckerr, stored atomically, +// mostly duplicating state that is manipulated under lock in pollDesc. +// The one exception is the pollEventErr bit, which is maintained only +// in the pollInfo. +type pollInfo uint32 + +const ( + pollClosing = 1 << iota + pollEventErr + pollExpiredReadDeadline + pollExpiredWriteDeadline +) + +func (i pollInfo) closing() bool { return i&pollClosing != 0 } +func (i pollInfo) eventErr() bool { return i&pollEventErr != 0 } +func (i pollInfo) expiredReadDeadline() bool { return i&pollExpiredReadDeadline != 0 } +func (i pollInfo) expiredWriteDeadline() bool { return i&pollExpiredWriteDeadline != 0 } + +// info returns the pollInfo corresponding to pd. +func (pd *pollDesc) info() pollInfo { + return pollInfo(atomic.Load(&pd.atomicInfo)) +} + +// publishInfo updates pd.atomicInfo (returned by pd.info) +// using the other values in pd. +// It must be called while holding pd.lock, +// and it must be called after changing anything +// that might affect the info bits. +// In practice this means after changing closing +// or changing rd or wd from < 0 to >= 0. +func (pd *pollDesc) publishInfo() { + var info uint32 + if pd.closing { + info |= pollClosing + } + if pd.rd < 0 { + info |= pollExpiredReadDeadline + } + if pd.wd < 0 { + info |= pollExpiredWriteDeadline + } + + // Set all of x except the pollEventErr bit. + x := atomic.Load(&pd.atomicInfo) + for !atomic.Cas(&pd.atomicInfo, x, (x&pollEventErr)|info) { + x = atomic.Load(&pd.atomicInfo) + } +} + +// setEventErr sets the result of pd.info().eventErr() to b. +func (pd *pollDesc) setEventErr(b bool) { + x := atomic.Load(&pd.atomicInfo) + for (x&pollEventErr != 0) != b && !atomic.Cas(&pd.atomicInfo, x, x^pollEventErr) { + x = atomic.Load(&pd.atomicInfo) + } +} + type pollCache struct { lock mutex first *pollDesc @@ -158,7 +226,7 @@ func poll_runtime_pollOpen(fd uintptr) (*pollDesc, int) { } pd.fd = fd pd.closing = false - pd.everr = false + pd.setEventErr(false) pd.rseq++ atomic.Storeuintptr(&pd.rg, 0) pd.rd = 0 @@ -166,6 +234,7 @@ func poll_runtime_pollOpen(fd uintptr) (*pollDesc, int) { atomic.Storeuintptr(&pd.wg, 0) pd.wd = 0 pd.self = pd + pd.publishInfo() unlock(&pd.lock) errno := netpollopen(fd, pd) @@ -274,6 +343,7 @@ func poll_runtime_pollSetDeadline(pd *pollDesc, d int64, mode int) { if mode == 'w' || mode == 'r'+'w' { pd.wd = d } + pd.publishInfo() combo := pd.rd > 0 && pd.rd == pd.wd rtf := netpollReadDeadline if combo { @@ -315,15 +385,13 @@ func poll_runtime_pollSetDeadline(pd *pollDesc, d int64, mode int) { } } // If we set the new deadline in the past, unblock currently pending IO if any. + // Note that pd.publishInfo has already been called, above, immediately after modifying rd and wd. var rg, wg *g - if pd.rd < 0 || pd.wd < 0 { - atomic.StorepNoWB(noescape(unsafe.Pointer(&wg)), nil) // full memory barrier between stores to rd/wd and load of rg/wg in netpollunblock - if pd.rd < 0 { - rg = netpollunblock(pd, 'r', false) - } - if pd.wd < 0 { - wg = netpollunblock(pd, 'w', false) - } + if pd.rd < 0 { + rg = netpollunblock(pd, 'r', false) + } + if pd.wd < 0 { + wg = netpollunblock(pd, 'w', false) } unlock(&pd.lock) if rg != nil { @@ -344,7 +412,7 @@ func poll_runtime_pollUnblock(pd *pollDesc) { pd.rseq++ pd.wseq++ var rg, wg *g - atomic.StorepNoWB(noescape(unsafe.Pointer(&rg)), nil) // full memory barrier between store to closing and read of rg/wg in netpollunblock + pd.publishInfo() rg = netpollunblock(pd, 'r', false) wg = netpollunblock(pd, 'w', false) if pd.rt.f != nil { @@ -389,16 +457,17 @@ func netpollready(toRun *gList, pd *pollDesc, mode int32) { } func netpollcheckerr(pd *pollDesc, mode int32) int { - if pd.closing { + info := pd.info() + if info.closing() { return pollErrClosing } - if (mode == 'r' && pd.rd < 0) || (mode == 'w' && pd.wd < 0) { + if (mode == 'r' && info.expiredReadDeadline()) || (mode == 'w' && info.expiredWriteDeadline()) { return pollErrTimeout } // Report an event scanning error only on a read event. // An error on a write event will be captured in a subsequent // write call that is able to report a more specific error. - if mode == 'r' && pd.everr { + if mode == 'r' && info.eventErr() { return pollErrNotPollable } return pollNoError @@ -449,7 +518,7 @@ func netpollblock(pd *pollDesc, mode int32, waitio bool) bool { // need to recheck error states after setting gpp to pdWait // this is necessary because runtime_pollUnblock/runtime_pollSetDeadline/deadlineimpl - // do the opposite: store to closing/rd/wd, membarrier, load of rg/wg + // do the opposite: store to closing/rd/wd, publishInfo, load of rg/wg if waitio || netpollcheckerr(pd, mode) == 0 { gopark(netpollblockcommit, unsafe.Pointer(gpp), waitReasonIOWait, traceEvGoBlockNet, 5) } @@ -509,7 +578,7 @@ func netpolldeadlineimpl(pd *pollDesc, seq uintptr, read, write bool) { throw("runtime: inconsistent read deadline") } pd.rd = -1 - atomic.StorepNoWB(unsafe.Pointer(&pd.rt.f), nil) // full memory barrier between store to rd and load of rg in netpollunblock + pd.publishInfo() rg = netpollunblock(pd, 'r', false) } var wg *g @@ -518,7 +587,7 @@ func netpolldeadlineimpl(pd *pollDesc, seq uintptr, read, write bool) { throw("runtime: inconsistent write deadline") } pd.wd = -1 - atomic.StorepNoWB(unsafe.Pointer(&pd.wt.f), nil) // full memory barrier between store to wd and load of wg in netpollunblock + pd.publishInfo() wg = netpollunblock(pd, 'w', false) } unlock(&pd.lock) diff --git a/src/runtime/netpoll_aix.go b/src/runtime/netpoll_aix.go index 4590ed81a6..90950af444 100644 --- a/src/runtime/netpoll_aix.go +++ b/src/runtime/netpoll_aix.go @@ -212,10 +212,7 @@ retry: pfd.events &= ^_POLLOUT } if mode != 0 { - pds[i].everr = false - if pfd.revents == _POLLERR { - pds[i].everr = true - } + pds[i].setEventErr(pfd.revents == _POLLERR) netpollready(&toRun, pds[i], mode) n-- } diff --git a/src/runtime/netpoll_epoll.go b/src/runtime/netpoll_epoll.go index 371ac59f8e..31b8decbef 100644 --- a/src/runtime/netpoll_epoll.go +++ b/src/runtime/netpoll_epoll.go @@ -169,10 +169,7 @@ retry: } if mode != 0 { pd := *(**pollDesc)(unsafe.Pointer(&ev.data)) - pd.everr = false - if ev.events == _EPOLLERR { - pd.everr = true - } + pd.setEventErr(ev.events == _EPOLLERR) netpollready(&toRun, pd, mode) } } diff --git a/src/runtime/netpoll_kqueue.go b/src/runtime/netpoll_kqueue.go index 80d1b0cf18..8ddabb15fc 100644 --- a/src/runtime/netpoll_kqueue.go +++ b/src/runtime/netpoll_kqueue.go @@ -180,10 +180,7 @@ retry: } if mode != 0 { pd := (*pollDesc)(unsafe.Pointer(ev.udata)) - pd.everr = false - if ev.flags == _EV_ERROR { - pd.everr = true - } + pd.setEventErr(ev.flags == _EV_ERROR) netpollready(&toRun, pd, mode) } } diff --git a/src/runtime/netpoll_solaris.go b/src/runtime/netpoll_solaris.go index d217d5b160..6e545b3d31 100644 --- a/src/runtime/netpoll_solaris.go +++ b/src/runtime/netpoll_solaris.go @@ -158,7 +158,7 @@ func netpollclose(fd uintptr) int32 { // this call, port_getn will return one and only one event for that // particular descriptor, so this function needs to be called again. func netpollupdate(pd *pollDesc, set, clear uint32) { - if pd.closing { + if pd.info().closing() { return } diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go index 5051ec4d3e..e675ef98d5 100644 --- a/src/runtime/runtime2.go +++ b/src/runtime/runtime2.go @@ -255,6 +255,8 @@ func efaceOf(ep *interface{}) *eface { // so I can't see them ever moving. If we did want to start moving data // in the GC, we'd need to allocate the goroutine structs from an // alternate arena. Using guintptr doesn't make that problem any worse. +// Note that pollDesc.rg, pollDesc.wg also store g in uintptr form, +// so they would need to be updated too if g's start moving. type guintptr uintptr //go:nosplit -- cgit v1.2.3-54-g00ecf From eb75219438e3c3d8947373c1f27c3ac4abf7ee8b Mon Sep 17 00:00:00 2001 From: Cherry Mui Date: Wed, 16 Mar 2022 13:07:57 -0400 Subject: [release-branch.go1.17] cmd/link: mark unexported methods for plugins When plugin is used, we already mark all exported methods reachable. However, when the plugin and the host program share a common package, an unexported method could also be reachable from both the plugin and the host via interfaces. We need to mark them as well. Fixes #51736. Updates #51621. Change-Id: I1a70d3f96b66b803f2d0ab14d00ed0df276ea500 Reviewed-on: https://go-review.googlesource.com/c/go/+/393365 Trust: Cherry Mui Run-TryBot: Cherry Mui TryBot-Result: Gopher Robot Reviewed-by: Than McIntosh (cherry picked from commit 91631bc7e0131367eb051b581cf34573399ac592) Reviewed-on: https://go-review.googlesource.com/c/go/+/397484 --- misc/cgo/testplugin/plugin_test.go | 6 +++++ misc/cgo/testplugin/testdata/method3/main.go | 32 ++++++++++++++++++++++++++ misc/cgo/testplugin/testdata/method3/p/p.go | 17 ++++++++++++++ misc/cgo/testplugin/testdata/method3/plugin.go | 11 +++++++++ src/cmd/link/internal/ld/deadcode.go | 2 +- 5 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 misc/cgo/testplugin/testdata/method3/main.go create mode 100644 misc/cgo/testplugin/testdata/method3/p/p.go create mode 100644 misc/cgo/testplugin/testdata/method3/plugin.go diff --git a/misc/cgo/testplugin/plugin_test.go b/misc/cgo/testplugin/plugin_test.go index c8ded42b69..3ccaa72446 100644 --- a/misc/cgo/testplugin/plugin_test.go +++ b/misc/cgo/testplugin/plugin_test.go @@ -287,6 +287,12 @@ func TestMethod2(t *testing.T) { run(t, "./method2.exe") } +func TestMethod3(t *testing.T) { + goCmd(t, "build", "-buildmode=plugin", "-o", "method3.so", "./method3/plugin.go") + goCmd(t, "build", "-o", "method3.exe", "./method3/main.go") + run(t, "./method3.exe") +} + func TestIssue44956(t *testing.T) { goCmd(t, "build", "-buildmode=plugin", "-o", "issue44956p1.so", "./issue44956/plugin1.go") goCmd(t, "build", "-buildmode=plugin", "-o", "issue44956p2.so", "./issue44956/plugin2.go") diff --git a/misc/cgo/testplugin/testdata/method3/main.go b/misc/cgo/testplugin/testdata/method3/main.go new file mode 100644 index 0000000000..a3a51711cd --- /dev/null +++ b/misc/cgo/testplugin/testdata/method3/main.go @@ -0,0 +1,32 @@ +// Copyright 2022 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. + +// An unexported method can be reachable from the plugin via interface +// when a package is shared. So it need to be live. + +package main + +import ( + "plugin" + + "testplugin/method3/p" +) + +var i p.I + +func main() { + pl, err := plugin.Open("method3.so") + if err != nil { + panic(err) + } + + f, err := pl.Lookup("F") + if err != nil { + panic(err) + } + + f.(func())() + + i = p.T(123) +} diff --git a/misc/cgo/testplugin/testdata/method3/p/p.go b/misc/cgo/testplugin/testdata/method3/p/p.go new file mode 100644 index 0000000000..3846bc07f5 --- /dev/null +++ b/misc/cgo/testplugin/testdata/method3/p/p.go @@ -0,0 +1,17 @@ +// Copyright 2022 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 p + +type T int + +func (T) m() { println("m") } + +type I interface { m() } + +func F() { + i.m() +} + +var i I = T(123) diff --git a/misc/cgo/testplugin/testdata/method3/plugin.go b/misc/cgo/testplugin/testdata/method3/plugin.go new file mode 100644 index 0000000000..bd25b31857 --- /dev/null +++ b/misc/cgo/testplugin/testdata/method3/plugin.go @@ -0,0 +1,11 @@ +// Copyright 2022 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 "testplugin/method3/p" + +func main() {} + +func F() { p.F() } diff --git a/src/cmd/link/internal/ld/deadcode.go b/src/cmd/link/internal/ld/deadcode.go index e4fa75f8e1..21a9703791 100644 --- a/src/cmd/link/internal/ld/deadcode.go +++ b/src/cmd/link/internal/ld/deadcode.go @@ -350,7 +350,7 @@ func deadcode(ctxt *Link) { // in the last pass. rem := d.markableMethods[:0] for _, m := range d.markableMethods { - if (d.reflectSeen && m.isExported()) || d.ifaceMethod[m.m] { + if (d.reflectSeen && (m.isExported() || d.dynlink)) || d.ifaceMethod[m.m] { d.markMethod(m) } else { rem = append(rem, m) -- cgit v1.2.3-54-g00ecf From 7139e8b024604ab168b51b99c6e8168257a5bf58 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Thu, 31 Mar 2022 12:31:58 -0400 Subject: [release-branch.go1.17] crypto/elliptic: tolerate zero-padded scalars in generic P-256 Updates #52075 Fixes #52076 Fixes CVE-2022-28327 Change-Id: I595a7514c9a0aa1b9c76aedfc2307e1124271f27 Reviewed-on: https://go-review.googlesource.com/c/go/+/397136 Trust: Filippo Valsorda Reviewed-by: Julie Qiu --- src/crypto/elliptic/p256.go | 2 +- src/crypto/elliptic/p256_test.go | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/crypto/elliptic/p256.go b/src/crypto/elliptic/p256.go index b2b12c8f13..da5283735c 100644 --- a/src/crypto/elliptic/p256.go +++ b/src/crypto/elliptic/p256.go @@ -52,7 +52,7 @@ func p256GetScalar(out *[32]byte, in []byte) { n := new(big.Int).SetBytes(in) var scalarBytes []byte - if n.Cmp(p256Params.N) >= 0 { + if n.Cmp(p256Params.N) >= 0 || len(in) > len(out) { n.Mod(n, p256Params.N) scalarBytes = n.Bytes() } else { diff --git a/src/crypto/elliptic/p256_test.go b/src/crypto/elliptic/p256_test.go index 1435f5e1a5..694186df81 100644 --- a/src/crypto/elliptic/p256_test.go +++ b/src/crypto/elliptic/p256_test.go @@ -153,3 +153,17 @@ func TestP256CombinedMult(t *testing.T) { t.Errorf("1×G + (-1)×G = (%d, %d), should be ∞", x, y) } } + +func TestIssue52075(t *testing.T) { + Gx, Gy := P256().Params().Gx, P256().Params().Gy + scalar := make([]byte, 33) + scalar[32] = 1 + x, y := P256().ScalarBaseMult(scalar) + if x.Cmp(Gx) != 0 || y.Cmp(Gy) != 0 { + t.Errorf("unexpected output (%v,%v)", x, y) + } + x, y = P256().ScalarMult(Gx, Gy, scalar) + if x.Cmp(Gx) != 0 || y.Cmp(Gy) != 0 { + t.Errorf("unexpected output (%v,%v)", x, y) + } +} -- cgit v1.2.3-54-g00ecf From 2116d60993e90d3f9b963c979f4bf1d116af03ff Mon Sep 17 00:00:00 2001 From: Julie Qiu Date: Tue, 1 Mar 2022 10:19:38 -0600 Subject: [release-branch.go1.17] encoding/pem: fix stack overflow in Decode Previously, Decode called decodeError, a recursive function that was prone to stack overflows when given a large PEM file containing errors. Credit to Juho Nurminen of Mattermost who reported the error. Fixes CVE-2022-24675 Updates #51853 Fixes #52036 Change-Id: Iffe768be53c8ddc0036fea0671d290f8f797692c Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1391157 Reviewed-by: Damien Neil Reviewed-by: Filippo Valsorda (cherry picked from commit 794ea5e828010e8b68493b2fc6d2963263195a02) Reviewed-on: https://go-review.googlesource.com/c/go/+/399816 Run-TryBot: Dmitri Shuralyov Reviewed-by: Dmitri Shuralyov Reviewed-by: Cherry Mui TryBot-Result: Gopher Robot --- src/encoding/pem/pem.go | 174 ++++++++++++++++++------------------------- src/encoding/pem/pem_test.go | 28 ++++++- 2 files changed, 101 insertions(+), 101 deletions(-) diff --git a/src/encoding/pem/pem.go b/src/encoding/pem/pem.go index a7272da5ad..1bee1c12d2 100644 --- a/src/encoding/pem/pem.go +++ b/src/encoding/pem/pem.go @@ -87,123 +87,97 @@ func Decode(data []byte) (p *Block, rest []byte) { // pemStart begins with a newline. However, at the very beginning of // the byte array, we'll accept the start string without it. rest = data - if bytes.HasPrefix(data, pemStart[1:]) { - rest = rest[len(pemStart)-1 : len(data)] - } else if i := bytes.Index(data, pemStart); i >= 0 { - rest = rest[i+len(pemStart) : len(data)] - } else { - return nil, data - } - - typeLine, rest := getLine(rest) - if !bytes.HasSuffix(typeLine, pemEndOfLine) { - return decodeError(data, rest) - } - typeLine = typeLine[0 : len(typeLine)-len(pemEndOfLine)] - - p = &Block{ - Headers: make(map[string]string), - Type: string(typeLine), - } - for { - // This loop terminates because getLine's second result is - // always smaller than its argument. - if len(rest) == 0 { + if bytes.HasPrefix(rest, pemStart[1:]) { + rest = rest[len(pemStart)-1:] + } else if i := bytes.Index(rest, pemStart); i >= 0 { + rest = rest[i+len(pemStart) : len(rest)] + } else { return nil, data } - line, next := getLine(rest) - i := bytes.IndexByte(line, ':') - if i == -1 { - break + var typeLine []byte + typeLine, rest = getLine(rest) + if !bytes.HasSuffix(typeLine, pemEndOfLine) { + continue } + typeLine = typeLine[0 : len(typeLine)-len(pemEndOfLine)] - // TODO(agl): need to cope with values that spread across lines. - key, val := line[:i], line[i+1:] - key = bytes.TrimSpace(key) - val = bytes.TrimSpace(val) - p.Headers[string(key)] = string(val) - rest = next - } + p = &Block{ + Headers: make(map[string]string), + Type: string(typeLine), + } - var endIndex, endTrailerIndex int + for { + // This loop terminates because getLine's second result is + // always smaller than its argument. + if len(rest) == 0 { + return nil, data + } + line, next := getLine(rest) - // If there were no headers, the END line might occur - // immediately, without a leading newline. - if len(p.Headers) == 0 && bytes.HasPrefix(rest, pemEnd[1:]) { - endIndex = 0 - endTrailerIndex = len(pemEnd) - 1 - } else { - endIndex = bytes.Index(rest, pemEnd) - endTrailerIndex = endIndex + len(pemEnd) - } + i := bytes.IndexByte(line, ':') + if i == -1 { + break + } - if endIndex < 0 { - return decodeError(data, rest) - } + // TODO(agl): need to cope with values that spread across lines. + key, val := line[:i], line[i+1:] + key = bytes.TrimSpace(key) + val = bytes.TrimSpace(val) + p.Headers[string(key)] = string(val) + rest = next + } - // After the "-----" of the ending line, there should be the same type - // and then a final five dashes. - endTrailer := rest[endTrailerIndex:] - endTrailerLen := len(typeLine) + len(pemEndOfLine) - if len(endTrailer) < endTrailerLen { - return decodeError(data, rest) - } + var endIndex, endTrailerIndex int - restOfEndLine := endTrailer[endTrailerLen:] - endTrailer = endTrailer[:endTrailerLen] - if !bytes.HasPrefix(endTrailer, typeLine) || - !bytes.HasSuffix(endTrailer, pemEndOfLine) { - return decodeError(data, rest) - } + // If there were no headers, the END line might occur + // immediately, without a leading newline. + if len(p.Headers) == 0 && bytes.HasPrefix(rest, pemEnd[1:]) { + endIndex = 0 + endTrailerIndex = len(pemEnd) - 1 + } else { + endIndex = bytes.Index(rest, pemEnd) + endTrailerIndex = endIndex + len(pemEnd) + } - // The line must end with only whitespace. - if s, _ := getLine(restOfEndLine); len(s) != 0 { - return decodeError(data, rest) - } + if endIndex < 0 { + continue + } - base64Data := removeSpacesAndTabs(rest[:endIndex]) - p.Bytes = make([]byte, base64.StdEncoding.DecodedLen(len(base64Data))) - n, err := base64.StdEncoding.Decode(p.Bytes, base64Data) - if err != nil { - return decodeError(data, rest) - } - p.Bytes = p.Bytes[:n] + // After the "-----" of the ending line, there should be the same type + // and then a final five dashes. + endTrailer := rest[endTrailerIndex:] + endTrailerLen := len(typeLine) + len(pemEndOfLine) + if len(endTrailer) < endTrailerLen { + continue + } + + restOfEndLine := endTrailer[endTrailerLen:] + endTrailer = endTrailer[:endTrailerLen] + if !bytes.HasPrefix(endTrailer, typeLine) || + !bytes.HasSuffix(endTrailer, pemEndOfLine) { + continue + } - // the -1 is because we might have only matched pemEnd without the - // leading newline if the PEM block was empty. - _, rest = getLine(rest[endIndex+len(pemEnd)-1:]) + // The line must end with only whitespace. + if s, _ := getLine(restOfEndLine); len(s) != 0 { + continue + } - return -} + base64Data := removeSpacesAndTabs(rest[:endIndex]) + p.Bytes = make([]byte, base64.StdEncoding.DecodedLen(len(base64Data))) + n, err := base64.StdEncoding.Decode(p.Bytes, base64Data) + if err != nil { + continue + } + p.Bytes = p.Bytes[:n] -func decodeError(data, rest []byte) (*Block, []byte) { - // If we get here then we have rejected a likely looking, but - // ultimately invalid PEM block. We need to start over from a new - // position. We have consumed the preamble line and will have consumed - // any lines which could be header lines. However, a valid preamble - // line is not a valid header line, therefore we cannot have consumed - // the preamble line for the any subsequent block. Thus, we will always - // find any valid block, no matter what bytes precede it. - // - // For example, if the input is - // - // -----BEGIN MALFORMED BLOCK----- - // junk that may look like header lines - // or data lines, but no END line - // - // -----BEGIN ACTUAL BLOCK----- - // realdata - // -----END ACTUAL BLOCK----- - // - // we've failed to parse using the first BEGIN line - // and now will try again, using the second BEGIN line. - p, rest := Decode(rest) - if p == nil { - rest = data + // the -1 is because we might have only matched pemEnd without the + // leading newline if the PEM block was empty. + _, rest = getLine(rest[endIndex+len(pemEnd)-1:]) + return p, rest } - return p, rest } const pemLineLength = 64 diff --git a/src/encoding/pem/pem_test.go b/src/encoding/pem/pem_test.go index b2b6b15e73..c94b5ca53b 100644 --- a/src/encoding/pem/pem_test.go +++ b/src/encoding/pem/pem_test.go @@ -107,6 +107,12 @@ const pemMissingEndingSpace = ` dGVzdA== -----ENDBAR-----` +const pemMissingEndLine = ` +-----BEGIN FOO----- +Header: 1` + +var pemRepeatingBegin = strings.Repeat("-----BEGIN \n", 10) + var badPEMTests = []struct { name string input string @@ -131,14 +137,34 @@ var badPEMTests = []struct { "missing ending space", pemMissingEndingSpace, }, + { + "repeating begin", + pemRepeatingBegin, + }, + { + "missing end line", + pemMissingEndLine, + }, } func TestBadDecode(t *testing.T) { for _, test := range badPEMTests { - result, _ := Decode([]byte(test.input)) + result, rest := Decode([]byte(test.input)) if result != nil { t.Errorf("unexpected success while parsing %q", test.name) } + if string(rest) != test.input { + t.Errorf("unexpected rest: %q; want = %q", rest, test.input) + } + } +} + +func TestCVE202224675(t *testing.T) { + // Prior to CVE-2022-24675, this input would cause a stack overflow. + input := []byte(strings.Repeat("-----BEGIN \n", 10000000)) + result, rest := Decode(input) + if result != nil || !reflect.DeepEqual(rest, input) { + t.Errorf("Encode of %#v decoded as %#v", input, rest) } } -- cgit v1.2.3-54-g00ecf From 346b18ee9d15410ab08dd583787c64dbed0666d2 Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Tue, 12 Apr 2022 10:55:38 -0400 Subject: [release-branch.go1.17] go1.17.9 Change-Id: Id2437003673e7eb1c514f15e6266b93308b7d5e8 Reviewed-on: https://go-review.googlesource.com/c/go/+/399819 Run-TryBot: Dmitri Shuralyov Reviewed-by: Cherry Mui TryBot-Result: Gopher Robot Reviewed-by: Dmitri Shuralyov --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index cea581fa74..699d953a7f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.17.8 \ No newline at end of file +go1.17.9 \ No newline at end of file -- cgit v1.2.3-54-g00ecf