From 312f7c1bd300d374f7078c4449c5ad142e0c3a5e Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Fri, 6 Dec 2024 15:37:34 -0500 Subject: [PATCH 01/22] runtime: add note that Callers never returns an entry PC The presence of a pc > entry check in CallersFrame implies we might actually see pc == entry, when in reality Callers will never return such a PC. This check is actually just a safety check for avoid reporting completely nonsensical from bad input. all.bash reports two violations to this invariant: TestCallersFromWrapper, which explicitly constructs a CallersFrame input with an entry PC. runtime/pprof.printStackRecord, which passes pprof stacks to CallersFrame (technically not a valid use of CallersFrames!). runtime/pprof.(*Profile).Add can add the entry PC of runtime/pprof.lostProfileEvent to samples. (CPU profiles do lostProfileEvent + 1. I will send a second CL to fix Add.) Change-Id: Iac2a2f0c15117d4a383bd84cddf0413b2d7dd3ef Reviewed-on: https://go-review.googlesource.com/c/go/+/634315 Auto-Submit: Michael Pratt LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Knyszek --- src/runtime/symtab.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/runtime/symtab.go b/src/runtime/symtab.go index ea048832c7..c78b044264 100644 --- a/src/runtime/symtab.go +++ b/src/runtime/symtab.go @@ -118,11 +118,16 @@ func (ci *Frames) Next() (frame Frame, more bool) { } f := funcInfo._Func() entry := f.Entry() + // We store the pc of the start of the instruction following + // the instruction in question (the call or the inline mark). + // This is done for historical reasons, and to make FuncForPC + // work correctly for entries in the result of runtime.Callers. + // Decrement to get back to the instruction we care about. + // + // It is not possible to get pc == entry from runtime.Callers, + // but if the caller does provide one, provide best-effort + // results by avoiding backing out of the function entirely. if pc > entry { - // We store the pc of the start of the instruction following - // the instruction in question (the call or the inline mark). - // This is done for historical reasons, and to make FuncForPC - // work correctly for entries in the result of runtime.Callers. pc-- } // It's important that interpret pc non-strictly as cgoTraceback may From 04cdaa9984682e36e7603dd7e40309fc916edb56 Mon Sep 17 00:00:00 2001 From: Cherry Mui Date: Thu, 5 Dec 2024 13:01:32 -0500 Subject: [PATCH 02/22] cmd/go: document c-shared buildmode for building WASI library/reactor For #65199. Change-Id: Icd3ec7cf25c2d381401686333c8aeed8013b3fbd Reviewed-on: https://go-review.googlesource.com/c/go/+/633418 Reviewed-by: Michael Matloob LUCI-TryBot-Result: Go LUCI --- src/cmd/go/alldocs.go | 5 ++++- src/cmd/go/internal/help/helpdoc.go | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go index ced43b6d5b..f227d93de7 100644 --- a/src/cmd/go/alldocs.go +++ b/src/cmd/go/alldocs.go @@ -2210,7 +2210,10 @@ // Build the listed main package, plus all packages it imports, // into a C shared library. The only callable symbols will // be those functions exported using a cgo //export comment. -// Requires exactly one main package to be listed. +// On wasip1, this mode builds it to a WASI reactor/library, +// of which the callable symbols are those functions exported +// using a //go:wasmexport directive. Requires exactly one +// main package to be listed. // // -buildmode=default // Listed main packages are built into executables and listed diff --git a/src/cmd/go/internal/help/helpdoc.go b/src/cmd/go/internal/help/helpdoc.go index a64f980e5e..d373c675f6 100644 --- a/src/cmd/go/internal/help/helpdoc.go +++ b/src/cmd/go/internal/help/helpdoc.go @@ -769,7 +769,10 @@ are: Build the listed main package, plus all packages it imports, into a C shared library. The only callable symbols will be those functions exported using a cgo //export comment. - Requires exactly one main package to be listed. + On wasip1, this mode builds it to a WASI reactor/library, + of which the callable symbols are those functions exported + using a //go:wasmexport directive. Requires exactly one + main package to be listed. -buildmode=default Listed main packages are built into executables and listed From 8c3e391573403cf1cf85b3256e99d0c0b7d79b3a Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Fri, 6 Dec 2024 20:43:16 -0500 Subject: [PATCH 03/22] runtime: improve AddCleanup documentation Steer people from SetFinalizer to AddCleanup. Address some of the *non*-constraints on AddCleanup. Add some of the subtlety from the SetFinalizer documentation to the AddCleanup documentation. Updates #67535. Updates #70425. Change-Id: I8d13b756ca866051b8a5c19327fd5a76f5e0f3d7 Reviewed-on: https://go-review.googlesource.com/c/go/+/634318 Reviewed-by: Michael Knyszek LUCI-TryBot-Result: Go LUCI Auto-Submit: Austin Clements --- src/runtime/mcleanup.go | 41 ++++++++++++++++++++++++++++++++--------- src/runtime/mfinal.go | 3 +++ 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/runtime/mcleanup.go b/src/runtime/mcleanup.go index 04d8ff59aa..22d40a5e84 100644 --- a/src/runtime/mcleanup.go +++ b/src/runtime/mcleanup.go @@ -12,8 +12,29 @@ import ( // AddCleanup attaches a cleanup function to ptr. Some time after ptr is no longer // reachable, the runtime will call cleanup(arg) in a separate goroutine. // +// A typical use is that ptr is an object wrapping an underlying resource (e.g., +// a File object wrapping an OS file descriptor), arg is the underlying resource +// (e.g., the OS file descriptor), and the cleanup function releases the underlying +// resource (e.g., by calling the close system call). +// +// There are few constraints on ptr. In particular, multiple cleanups may be +// attached to the same pointer, or to different pointers within the same +// allocation. +// // If ptr is reachable from cleanup or arg, ptr will never be collected -// and the cleanup will never run. AddCleanup panics if arg is equal to ptr. +// and the cleanup will never run. As a protection against simple cases of this, +// AddCleanup panics if arg is equal to ptr. +// +// There is no specified order in which cleanups will run. +// In particular, if several objects point to each other and all become +// unreachable at the same time, their cleanups all become eligible to run +// and can run in any order. This is true even if the objects form a cycle. +// +// A single goroutine runs all cleanup calls for a program, sequentially. If a +// cleanup function must run for a long time, it should create a new goroutine. +// +// If ptr has both a cleanup and a finalizer, the cleanup will only run once +// it has been finalized and becomes unreachable without an associated finalizer. // // The cleanup(arg) call is not always guaranteed to run; in particular it is not // guaranteed to run before program exit. @@ -22,14 +43,6 @@ import ( // it may share same address with other zero-size objects in memory. See // https://go.dev/ref/spec#Size_and_alignment_guarantees. // -// There is no specified order in which cleanups will run. -// -// A single goroutine runs all cleanup calls for a program, sequentially. If a -// cleanup function must run for a long time, it should create a new goroutine. -// -// If ptr has both a cleanup and a finalizer, the cleanup will only run once -// it has been finalized and becomes unreachable without an associated finalizer. -// // It is not guaranteed that a cleanup will run for objects allocated // in initializers for package-level variables. Such objects may be // linker-allocated, not heap-allocated. @@ -41,6 +54,16 @@ import ( // allocation may never run if it always exists in the same batch as a // referenced object. Typically, this batching only happens for tiny // (on the order of 16 bytes or less) and pointer-free objects. +// +// A cleanup may run as soon as an object becomes unreachable. +// In order to use cleanups correctly, the program must ensure that +// the object is reachable until it is safe to run its cleanup. +// Objects stored in global variables, or that can be found by tracing +// pointers from a global variable, are reachable. A function argument or +// receiver may become unreachable at the last point where the function +// mentions it. To ensure a cleanup does not get called prematurely, +// pass the object to the [KeepAlive] function after the last point +// where the object must remain reachable. func AddCleanup[T, S any](ptr *T, cleanup func(S), arg S) Cleanup { // Explicitly force ptr to escape to the heap. ptr = abi.Escape(ptr) diff --git a/src/runtime/mfinal.go b/src/runtime/mfinal.go index 89a9c84170..4962a63a41 100644 --- a/src/runtime/mfinal.go +++ b/src/runtime/mfinal.go @@ -350,6 +350,9 @@ func blockUntilEmptyFinalizerQueue(timeout int64) bool { // // SetFinalizer(obj, nil) clears any finalizer associated with obj. // +// New Go code should consider using [AddCleanup] instead, which is much +// less error-prone than SetFinalizer. +// // The argument obj must be a pointer to an object allocated by calling // new, by taking the address of a composite literal, or by taking the // address of a local variable. From c8fb6ae617d65b42089202040d8fbd309d1a0fe4 Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Sat, 7 Dec 2024 17:48:27 -0500 Subject: [PATCH 04/22] lib/wasm: provide fs.constants.O_DIRECTORY definition CL 606658 added a constants.Get("O_DIRECTORY").Int() call at init time, which panics in browsers because O_DIRECTORY is undefined. It needs to be a JavaScript number to avoid that. Fixes #70723. Change-Id: I727240bd25b47401d14a5e1a364d460708803f1f Reviewed-on: https://go-review.googlesource.com/c/go/+/634455 TryBot-Bypass: Dmitri Shuralyov Auto-Submit: Dmitri Shuralyov Reviewed-by: Ian Lance Taylor Reviewed-by: Dmitri Shuralyov Reviewed-by: Zxilly Chou --- lib/wasm/wasm_exec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/wasm/wasm_exec.js b/lib/wasm/wasm_exec.js index ec96d42db5..d71af9e97e 100644 --- a/lib/wasm/wasm_exec.js +++ b/lib/wasm/wasm_exec.js @@ -14,7 +14,7 @@ if (!globalThis.fs) { let outputBuf = ""; globalThis.fs = { - constants: { O_WRONLY: -1, O_RDWR: -1, O_CREAT: -1, O_TRUNC: -1, O_APPEND: -1, O_EXCL: -1 }, // unused + constants: { O_WRONLY: -1, O_RDWR: -1, O_CREAT: -1, O_TRUNC: -1, O_APPEND: -1, O_EXCL: -1, O_DIRECTORY: -1 }, // unused writeSync(fd, buf) { outputBuf += decoder.decode(buf); const nl = outputBuf.lastIndexOf("\n"); From e79b2e1e3acbce03b04f4ae95a8884183006bd1e Mon Sep 17 00:00:00 2001 From: Sean Liao Date: Mon, 2 Dec 2024 18:32:36 +0000 Subject: [PATCH 05/22] cmd/go: document the build cache as safe for concurrent use Fixes #26677 Change-Id: I2ca0408503000ccaddb0bd1fd359381ddd4fb699 Reviewed-on: https://go-review.googlesource.com/c/go/+/632895 Reviewed-by: Michael Matloob Auto-Submit: Sam Thanawalla LUCI-TryBot-Result: Go LUCI Reviewed-by: Sam Thanawalla --- src/cmd/go/alldocs.go | 1 + src/cmd/go/internal/help/helpdoc.go | 1 + 2 files changed, 2 insertions(+) diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go index f227d93de7..3a4473902c 100644 --- a/src/cmd/go/alldocs.go +++ b/src/cmd/go/alldocs.go @@ -2264,6 +2264,7 @@ // The go command caches build outputs for reuse in future builds. // The default location for cache data is a subdirectory named go-build // in the standard user cache directory for the current operating system. +// The cache is safe for concurrent invocations of the go command. // Setting the GOCACHE environment variable overrides this default, // and running 'go env GOCACHE' prints the current cache directory. // diff --git a/src/cmd/go/internal/help/helpdoc.go b/src/cmd/go/internal/help/helpdoc.go index d373c675f6..e1240de710 100644 --- a/src/cmd/go/internal/help/helpdoc.go +++ b/src/cmd/go/internal/help/helpdoc.go @@ -809,6 +809,7 @@ var HelpCache = &base.Command{ The go command caches build outputs for reuse in future builds. The default location for cache data is a subdirectory named go-build in the standard user cache directory for the current operating system. +The cache is safe for concurrent invocations of the go command. Setting the GOCACHE environment variable overrides this default, and running 'go env GOCACHE' prints the current cache directory. From e3e1d735287a3bab5b060415513bd64785c4e209 Mon Sep 17 00:00:00 2001 From: Petr Osetrov Date: Fri, 6 Dec 2024 21:51:36 +0000 Subject: [PATCH 06/22] bufio: make the description of Peek's behavior better Previously, based on the description, it was not obvious that Peek could change the buffer. It may have been mistakenly assumed that Peek would always return an error if n is greater than b.Buffered(). Change-Id: I095006dd2ba1c2138bb193396cb24e2dda42d771 GitHub-Last-Rev: 9d48f8ac81f46d5b8f4a1885af28cbccd1747c3b GitHub-Pull-Request: golang/go#70712 Reviewed-on: https://go-review.googlesource.com/c/go/+/634175 Reviewed-by: Carlos Amedee LUCI-TryBot-Result: Go LUCI Auto-Submit: Ian Lance Taylor Reviewed-by: Ian Lance Taylor Reviewed-by: Jorropo --- src/bufio/bufio.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/bufio/bufio.go b/src/bufio/bufio.go index 160bd8adb3..d589701e19 100644 --- a/src/bufio/bufio.go +++ b/src/bufio/bufio.go @@ -133,9 +133,10 @@ func (b *Reader) readErr() error { } // Peek returns the next n bytes without advancing the reader. The bytes stop -// being valid at the next read call. If Peek returns fewer than n bytes, it -// also returns an error explaining why the read is short. The error is -// [ErrBufferFull] if n is larger than b's buffer size. +// being valid at the next read call. If necessary, Peek will read more bytes +// into the buffer in order to make n bytes available. If Peek returns fewer +// than n bytes, it also returns an error explaining why the read is short. +// The error is [ErrBufferFull] if n is larger than b's buffer size. // // Calling Peek prevents a [Reader.UnreadByte] or [Reader.UnreadRune] call from succeeding // until the next read operation. From 07398d2e57ac5df6f95b0344252f1560376328f3 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Sat, 7 Dec 2024 02:58:00 +0000 Subject: [PATCH 07/22] weak: align weak.Pointer documentation with runtime.AddCleanup In hindsight, I think the "advice" I wrote is a bit heavy-handed and better suited for something like the GC guide. Listing the use-cases seems good, and all the possible things that go wrong seems to do the trick in terms of deterrence, like it does with finalizers. Also, include some points I missed, like the tiny allocator warning and the fact that weak pointers are not guaranteed to ever return nil. Also, a lot of this actually shouldn't have been in the package docs. Many of the warnings only apply to weak pointers, but not other data structures that may live in this package in the future, like weak-keyed maps. Change-Id: Id245661540ffd93de4b727cd272284491d085c1e Reviewed-on: https://go-review.googlesource.com/c/go/+/634376 Reviewed-by: Carlos Amedee LUCI-TryBot-Result: Go LUCI Auto-Submit: Michael Knyszek --- src/weak/doc.go | 27 ++------------------------- src/weak/pointer.go | 42 +++++++++++++++++++++++++++++++++++------- 2 files changed, 37 insertions(+), 32 deletions(-) diff --git a/src/weak/doc.go b/src/weak/doc.go index e66d5ab5ac..1af8e4c69b 100644 --- a/src/weak/doc.go +++ b/src/weak/doc.go @@ -3,30 +3,7 @@ // license that can be found in the LICENSE file. /* -Package weak provides weak pointers with the goal of memory efficiency. -The primary use-cases for weak pointers are for implementing caches, -canonicalization maps (like the unique package), and for tying together -the lifetimes of separate values (for example, through a map with weak -keys). - -# Advice - -This package is intended to target niche use-cases like the unique -package, and the structures inside are not intended to be general -replacements for regular Go pointers, maps, etc. -Misuse of the structures in this package may generate unexpected and -hard-to-reproduce bugs. -Using the facilities in this package to try and resolve out-of-memory -issues requires careful consideration, and even so, will likely be the -wrong answer if the solution does not fall into one of the listed -use-cases above. - -The structures in this package are intended to be an implementation -detail of the package they are used by (again, see the unique package). -If you're writing a package intended to be used by others, as a rule of -thumb, avoid exposing the behavior of any weak structures in your package's -API. -Doing so will almost certainly make your package more difficult to use -correctly. +Package weak provides ways to safely reference memory weakly, +that is, without preventing its reclamation. */ package weak diff --git a/src/weak/pointer.go b/src/weak/pointer.go index f6d20530ab..fb10bc2d69 100644 --- a/src/weak/pointer.go +++ b/src/weak/pointer.go @@ -12,11 +12,21 @@ import ( // Pointer is a weak pointer to a value of type T. // -// Two Pointer values compare equal if the pointers -// that they were created from compare equal. This property is retained even -// after the object referenced by the pointer used to create a weak reference -// is reclaimed. +// Just like regular pointers, Pointer may reference any part of an +// object, such as the field of a struct or an element of an array. +// Objects that are only pointed to by weak pointers are not considered +// reachable and once the object becomes unreachable [Pointer.Value] +// may return nil. // +// The primary use-cases for weak pointers are for implementing caches, +// canonicalization maps (like the unique package), and for tying together +// the lifetimes of separate values (for example, through a map with weak +// keys). +// +// Two Pointer values always compare equal if the pointers that they were +// created from compare equal. This property is retained even after the +// object referenced by the pointer used to create a weak reference is +// reclaimed. // If multiple weak pointers are made to different offsets within same object // (for example, pointers to different fields of the same struct), those pointers // will not compare equal. @@ -24,14 +34,32 @@ import ( // then resurrected due to a finalizer, that weak pointer will not compare equal // with weak pointers created after resurrection. // -// Calling Make with a nil pointer returns a weak pointer whose Value method +// Calling [Make] with a nil pointer returns a weak pointer whose [Pointer.Value] // always returns nil. The zero value of a Pointer behaves as if it was created -// by passing nil to Make and compares equal with such pointers. +// by passing nil to [Make] and compares equal with such pointers. +// +// [Pointer.Value] is not guaranteed to eventually return nil. +// [Pointer.Value] may return nil as soon as the object becomes +// unreachable. +// Values stored in global variables, or that can be found by tracing +// pointers from a global variable, are reachable. A function argument or +// receiver may become unreachable at the last point where the function +// mentions it. To ensure [Pointer.Value] does not return nil, +// pass a pointer to the object to the [runtime.KeepAlive] function after +// the last point where the object must remain reachable. +// +// Note that because [Pointer.Value] is not guaranteed to eventually return +// nil, even after an object is no longer referenced, the runtime is allowed to +// perform a space-saving optimization that batches objects together in a single +// allocation slot. The weak pointer for an unreferenced object in such an +// allocation may never be called if it always exists in the same batch as a +// referenced object. Typically, this batching only happens for tiny +// (on the order of 16 bytes or less) and pointer-free objects. type Pointer[T any] struct { u unsafe.Pointer } -// Make creates a weak pointer from a strong pointer to some value of type T. +// Make creates a weak pointer from a pointer to some value of type T. func Make[T any](ptr *T) Pointer[T] { // Explicitly force ptr to escape to the heap. ptr = abi.Escape(ptr) From 6705ac688528b5a9ef7ec94ba04ab1f65f048a75 Mon Sep 17 00:00:00 2001 From: Paul Cacheux Date: Sun, 8 Dec 2024 21:45:48 +0100 Subject: [PATCH 08/22] runtime: remove datadog-agent from prof labels hall of shame github.com/DataDog/datadog-agent has stopped using runtime_setProfLabel and runtime_getProfLabel, remove them from the hall of shame. Updates #67401 Change-Id: I4a66c5e70397d43d7f064aeae5bad064e168316f Reviewed-on: https://go-review.googlesource.com/c/go/+/634476 Auto-Submit: Ian Lance Taylor Reviewed-by: Cherry Mui LUCI-TryBot-Result: Go LUCI Reviewed-by: Ian Lance Taylor --- src/runtime/proflabel.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/runtime/proflabel.go b/src/runtime/proflabel.go index 1a5e7e5e2f..f9b9dd16a5 100644 --- a/src/runtime/proflabel.go +++ b/src/runtime/proflabel.go @@ -12,7 +12,6 @@ var labelSync uintptr // but widely used packages access it using linkname. // Notable members of the hall of shame include: // - github.com/cloudwego/localsession -// - github.com/DataDog/datadog-agent // // Do not remove or change the type signature. // See go.dev/issue/67401. @@ -47,7 +46,6 @@ func runtime_setProfLabel(labels unsafe.Pointer) { // but widely used packages access it using linkname. // Notable members of the hall of shame include: // - github.com/cloudwego/localsession -// - github.com/DataDog/datadog-agent // // Do not remove or change the type signature. // See go.dev/issue/67401. From d87878c62b2db318a12e5bd2126a82c117961156 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Mon, 9 Dec 2024 19:21:48 +0000 Subject: [PATCH 09/22] runtime: make special offset a uintptr Currently specials try to save on space by only encoding the offset from the base of the span in a uint16. This worked fine up until Go 1.24. - Most specials have an offset of 0 (mem profile, finalizers, etc.) - Cleanups do not care about the offset at all, so even if it's wrong, it's OK. - Weak pointers *do* care, but the unique package always makes a new allocation, so the weak pointer handle offset it makes is always zero. With Go 1.24 and general weak pointers now available, nothing is stopping someone from just creating a weak pointer that is >64 KiB offset from the start of an object, and this weak pointer must be distinct from others. Fix this problem by just increasing the size of a special and making the offset a uintptr, to capture all possible offsets. Since we're in the freeze, this is the safest thing to do. Specials aren't so common that I expect a substantial memory increase from this change. In a future release (or if there is a problem) we can almost certainly pack the special's kind and offset together. There was already a bunch of wasted space due to padding, so this would bring us back to the same memory footprint before this change. Also, add tests for equality of basic weak interior pointers. This works, but we really should've had tests for it. Fixes #70739. Change-Id: Ib49a7f8f0f1ec3db4571a7afb0f4d94c8a93aa40 Reviewed-on: https://go-review.googlesource.com/c/go/+/634598 LUCI-TryBot-Result: Go LUCI Reviewed-by: Carlos Amedee Auto-Submit: Michael Knyszek Commit-Queue: Michael Knyszek --- src/runtime/mheap.go | 4 ++-- src/runtime/pinner.go | 2 +- src/weak/pointer_test.go | 17 +++++++++++++++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/runtime/mheap.go b/src/runtime/mheap.go index 4fcfbeca84..e058dd8489 100644 --- a/src/runtime/mheap.go +++ b/src/runtime/mheap.go @@ -1839,7 +1839,7 @@ const ( type special struct { _ sys.NotInHeap next *special // linked list in span - offset uint16 // span offset of object + offset uintptr // span offset of object kind byte // kind of special } @@ -1886,7 +1886,7 @@ func addspecial(p unsafe.Pointer, s *special, force bool) bool { iter, exists := span.specialFindSplicePoint(offset, kind) if !exists || force { // Splice in record, fill in offset. - s.offset = uint16(offset) + s.offset = offset s.next = *iter *iter = s spanHasSpecials(span) diff --git a/src/runtime/pinner.go b/src/runtime/pinner.go index 7a9c381580..543bfdb7a4 100644 --- a/src/runtime/pinner.go +++ b/src/runtime/pinner.go @@ -331,7 +331,7 @@ func (span *mspan) incPinCounter(offset uintptr) { rec = (*specialPinCounter)(mheap_.specialPinCounterAlloc.alloc()) unlock(&mheap_.speciallock) // splice in record, fill in offset. - rec.special.offset = uint16(offset) + rec.special.offset = offset rec.special.kind = _KindSpecialPinCounter rec.special.next = *ref *ref = (*special)(unsafe.Pointer(rec)) diff --git a/src/weak/pointer_test.go b/src/weak/pointer_test.go index 213dde8c40..002b4130f0 100644 --- a/src/weak/pointer_test.go +++ b/src/weak/pointer_test.go @@ -43,9 +43,11 @@ func TestPointer(t *testing.T) { func TestPointerEquality(t *testing.T) { bt := make([]*T, 10) wt := make([]weak.Pointer[T], 10) + wo := make([]weak.Pointer[int], 10) for i := range bt { bt[i] = new(T) wt[i] = weak.Make(bt[i]) + wo[i] = weak.Make(&bt[i].a) } for i := range bt { st := wt[i].Value() @@ -55,6 +57,9 @@ func TestPointerEquality(t *testing.T) { if wp := weak.Make(st); wp != wt[i] { t.Fatalf("new weak pointer not equal to existing weak pointer: %v vs. %v", wp, wt[i]) } + if wp := weak.Make(&st.a); wp != wo[i] { + t.Fatalf("new weak pointer not equal to existing weak pointer: %v vs. %v", wp, wo[i]) + } if i == 0 { continue } @@ -72,6 +77,9 @@ func TestPointerEquality(t *testing.T) { if wp := weak.Make(st); wp != wt[i] { t.Fatalf("new weak pointer not equal to existing weak pointer: %v vs. %v", wp, wt[i]) } + if wp := weak.Make(&st.a); wp != wo[i] { + t.Fatalf("new weak pointer not equal to existing weak pointer: %v vs. %v", wp, wo[i]) + } if i == 0 { continue } @@ -210,3 +218,12 @@ func TestIssue69210(t *testing.T) { } wg.Wait() } + +func TestIssue70739(t *testing.T) { + x := make([]*int, 4<<16) + wx1 := weak.Make(&x[1<<16]) + wx2 := weak.Make(&x[1<<16]) + if wx1 != wx2 { + t.Fatal("failed to look up special and made duplicate weak handle; see issue #70739") + } +} From fce17b0c774593da0dfb39b829464d1da8ceca77 Mon Sep 17 00:00:00 2001 From: Tom Thorogood Date: Tue, 10 Dec 2024 23:22:49 +1030 Subject: [PATCH 10/22] crypto/internal/fips140/ecdsa: fix reseed_counter check for HMAC_DRBG_Generate_algorithm SP 800-90A Rev. 1 10.1.2.5 step 7 requires reseed_counter = reseed_counter + 1 as the final step before returning SUCCESS. This increment of reseedCounter was missing, meaning the reseed interval check at the start of Generate wasn't actually functional. Given how it's used, and that it has a reseed interval of 2^48, this condition will never actually occur but the check is still required by the standard. For #69536 Change-Id: I314a7eee5852e6d0fa1a0a04842003553cd803e7 Reviewed-on: https://go-review.googlesource.com/c/go/+/634775 Reviewed-by: Carlos Amedee LUCI-TryBot-Result: Go LUCI Auto-Submit: Filippo Valsorda Reviewed-by: Filippo Valsorda Reviewed-by: Michael Knyszek --- src/crypto/internal/fips140/ecdsa/hmacdrbg.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/crypto/internal/fips140/ecdsa/hmacdrbg.go b/src/crypto/internal/fips140/ecdsa/hmacdrbg.go index 6fd7ac6974..4f085e2801 100644 --- a/src/crypto/internal/fips140/ecdsa/hmacdrbg.go +++ b/src/crypto/internal/fips140/ecdsa/hmacdrbg.go @@ -160,4 +160,6 @@ func (d *hmacDRBG) Generate(out []byte) { d.hK = d.newHMAC(K) d.hK.Write(d.V) d.V = d.hK.Sum(d.V[:0]) + + d.reseedCounter++ } From e6de1b2debe2bc7211f6f9cac4b64d7cd90f7c4e Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Mon, 9 Dec 2024 11:53:32 -0800 Subject: [PATCH 11/22] html/template: escape script tags in JS errors case insensitively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks to Juho Forsén of Mattermost for reporting this issue. Fixes #70740 Change-Id: I1a49b199dee91cd2bb4df5b174aaa958dc040c18 Reviewed-on: https://go-review.googlesource.com/c/go/+/634696 LUCI-TryBot-Result: Go LUCI Reviewed-by: Damien Neil --- src/html/template/js.go | 11 +++++++---- src/html/template/js_test.go | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/html/template/js.go b/src/html/template/js.go index d1463dee14..b3bf94801b 100644 --- a/src/html/template/js.go +++ b/src/html/template/js.go @@ -9,6 +9,7 @@ import ( "encoding/json" "fmt" "reflect" + "regexp" "strings" "unicode/utf8" ) @@ -144,6 +145,8 @@ func indirectToJSONMarshaler(a any) any { return v.Interface() } +var scriptTagRe = regexp.MustCompile("(?i)<(/?)script") + // jsValEscaper escapes its inputs to a JS Expression (section 11.14) that has // neither side-effects nor free variables outside (NaN, Infinity). func jsValEscaper(args ...any) string { @@ -181,9 +184,9 @@ func jsValEscaper(args ...any) string { // In particular we: // * replace "*/" comment end tokens with "* /", which does not // terminate the comment - // * replace " Date: Tue, 10 Dec 2024 09:49:45 -0800 Subject: [PATCH 12/22] runtime: avoid panic in expired synctest timer chan read When reading from time.Timer.C for an expired timer using a fake clock (in a synctest bubble), the timer will not be in a heap. Avoid a spurious panic claiming the timer moved between synctest bubbles. Drop the panic when a bubbled goroutine reads from a non-bubbled timer channel: We allow bubbled goroutines to access non-bubbled channels in general. Fixes #70741 Change-Id: I27005e46f4d0067cc6846d234d22766d2e05d163 Reviewed-on: https://go-review.googlesource.com/c/go/+/634955 Auto-Submit: Damien Neil Reviewed-by: Michael Pratt LUCI-TryBot-Result: Go LUCI --- src/internal/synctest/synctest_test.go | 41 ++++++++++++++++++++++++-- src/runtime/time.go | 10 +++++-- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/internal/synctest/synctest_test.go b/src/internal/synctest/synctest_test.go index 2c4ac0ff64..7d1e04e2ba 100644 --- a/src/internal/synctest/synctest_test.go +++ b/src/internal/synctest/synctest_test.go @@ -105,7 +105,7 @@ func TestMallocs(t *testing.T) { } } -func TestTimer(t *testing.T) { +func TestTimerReadBeforeDeadline(t *testing.T) { synctest.Run(func() { start := time.Now() tm := time.NewTimer(5 * time.Second) @@ -116,6 +116,41 @@ func TestTimer(t *testing.T) { }) } +func TestTimerReadAfterDeadline(t *testing.T) { + synctest.Run(func() { + delay := 1 * time.Second + want := time.Now().Add(delay) + tm := time.NewTimer(delay) + time.Sleep(2 * delay) + got := <-tm.C + if got != want { + t.Errorf("<-tm.C = %v, want %v", got, want) + } + }) +} + +func TestTimerReset(t *testing.T) { + synctest.Run(func() { + start := time.Now() + tm := time.NewTimer(1 * time.Second) + if got, want := <-tm.C, start.Add(1*time.Second); got != want { + t.Errorf("first sleep: <-tm.C = %v, want %v", got, want) + } + + tm.Reset(2 * time.Second) + if got, want := <-tm.C, start.Add((1+2)*time.Second); got != want { + t.Errorf("second sleep: <-tm.C = %v, want %v", got, want) + } + + tm.Reset(3 * time.Second) + time.Sleep(1 * time.Second) + tm.Reset(3 * time.Second) + if got, want := <-tm.C, start.Add((1+2+4)*time.Second); got != want { + t.Errorf("third sleep: <-tm.C = %v, want %v", got, want) + } + }) +} + func TestTimeAfter(t *testing.T) { synctest.Run(func() { i := 0 @@ -138,9 +173,11 @@ func TestTimeAfter(t *testing.T) { func TestTimerFromOutsideBubble(t *testing.T) { tm := time.NewTimer(10 * time.Millisecond) synctest.Run(func() { - defer wantPanic(t, "timer moved between synctest groups") <-tm.C }) + if tm.Stop() { + t.Errorf("synctest.Run unexpectedly returned before timer fired") + } } func TestChannelFromOutsideBubble(t *testing.T) { diff --git a/src/runtime/time.go b/src/runtime/time.go index 7c6d798872..c22d39c089 100644 --- a/src/runtime/time.go +++ b/src/runtime/time.go @@ -1370,15 +1370,19 @@ func badTimer() { // to send a value to its associated channel. If so, it does. // The timer must not be locked. func (t *timer) maybeRunChan() { - if sg := getg().syncGroup; sg != nil || t.isFake { + if t.isFake { t.lock() var timerGroup *synctestGroup if t.ts != nil { timerGroup = t.ts.syncGroup } t.unlock() - if sg == nil || !t.isFake || sg != timerGroup { - panic(plainError("timer moved between synctest groups")) + sg := getg().syncGroup + if sg == nil { + panic(plainError("synctest timer accessed from outside bubble")) + } + if timerGroup != nil && sg != timerGroup { + panic(plainError("timer moved between synctest bubbles")) } // No need to do anything here. // synctest.Run will run the timer when it advances its fake clock. From a9922d096f0de877fba68739b35367d4c25f6ecb Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Tue, 10 Dec 2024 16:10:27 -0800 Subject: [PATCH 13/22] reflect: consistently document when value must be settable Fixes #70760 Change-Id: Ia00723698b7e502fa2c63f8f1dbe1143af22e0a5 Reviewed-on: https://go-review.googlesource.com/c/go/+/634799 Reviewed-by: Ian Lance Taylor LUCI-TryBot-Result: Go LUCI Reviewed-by: Keith Randall Commit-Queue: Ian Lance Taylor Reviewed-by: Keith Randall Auto-Submit: Ian Lance Taylor Auto-Submit: Ian Lance Taylor --- src/reflect/map_noswiss.go | 2 ++ src/reflect/map_swiss.go | 2 ++ src/reflect/value.go | 36 +++++++++++++++++++++++------------- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/reflect/map_noswiss.go b/src/reflect/map_noswiss.go index 99609829f0..eb0a52a390 100644 --- a/src/reflect/map_noswiss.go +++ b/src/reflect/map_noswiss.go @@ -289,6 +289,7 @@ func (iter *MapIter) Key() Value { // It is equivalent to v.Set(iter.Key()), but it avoids allocating a new Value. // As in Go, the key must be assignable to v's type and // must not be derived from an unexported field. +// It panics if [Value.CanSet] returns false. func (v Value) SetIterKey(iter *MapIter) { if !iter.hiter.initialized() { panic("reflect: Value.SetIterKey called before Next") @@ -332,6 +333,7 @@ func (iter *MapIter) Value() Value { // It is equivalent to v.Set(iter.Value()), but it avoids allocating a new Value. // As in Go, the value must be assignable to v's type and // must not be derived from an unexported field. +// It panics if [Value.CanSet] returns false. func (v Value) SetIterValue(iter *MapIter) { if !iter.hiter.initialized() { panic("reflect: Value.SetIterValue called before Next") diff --git a/src/reflect/map_swiss.go b/src/reflect/map_swiss.go index 7098e21291..75dcb117df 100644 --- a/src/reflect/map_swiss.go +++ b/src/reflect/map_swiss.go @@ -240,6 +240,7 @@ func (iter *MapIter) Key() Value { // It is equivalent to v.Set(iter.Key()), but it avoids allocating a new Value. // As in Go, the key must be assignable to v's type and // must not be derived from an unexported field. +// It panics if [Value.CanSet] returns false. func (v Value) SetIterKey(iter *MapIter) { if !iter.hiter.Initialized() { panic("reflect: Value.SetIterKey called before Next") @@ -283,6 +284,7 @@ func (iter *MapIter) Value() Value { // It is equivalent to v.Set(iter.Value()), but it avoids allocating a new Value. // As in Go, the value must be assignable to v's type and // must not be derived from an unexported field. +// It panics if [Value.CanSet] returns false. func (v Value) SetIterValue(iter *MapIter) { if !iter.hiter.Initialized() { panic("reflect: Value.SetIterValue called before Next") diff --git a/src/reflect/value.go b/src/reflect/value.go index e02002ff33..4ed94addf9 100644 --- a/src/reflect/value.go +++ b/src/reflect/value.go @@ -2072,7 +2072,8 @@ func (v Value) SetBool(x bool) { } // SetBytes sets v's underlying value. -// It panics if v's underlying value is not a slice of bytes. +// It panics if v's underlying value is not a slice of bytes +// or if [Value.CanSet] returns false. func (v Value) SetBytes(x []byte) { v.mustBeAssignable() v.mustBe(Slice) @@ -2083,7 +2084,8 @@ func (v Value) SetBytes(x []byte) { } // setRunes sets v's underlying value. -// It panics if v's underlying value is not a slice of runes (int32s). +// It panics if v's underlying value is not a slice of runes (int32s) +// or if [Value.CanSet] returns false. func (v Value) setRunes(x []rune) { v.mustBeAssignable() v.mustBe(Slice) @@ -2094,7 +2096,8 @@ func (v Value) setRunes(x []rune) { } // SetComplex sets v's underlying value to x. -// It panics if v's Kind is not [Complex64] or [Complex128], or if [Value.CanSet] returns false. +// It panics if v's Kind is not [Complex64] or [Complex128], +// or if [Value.CanSet] returns false. func (v Value) SetComplex(x complex128) { v.mustBeAssignable() switch k := v.kind(); k { @@ -2108,7 +2111,8 @@ func (v Value) SetComplex(x complex128) { } // SetFloat sets v's underlying value to x. -// It panics if v's Kind is not [Float32] or [Float64], or if [Value.CanSet] returns false. +// It panics if v's Kind is not [Float32] or [Float64], +// or if [Value.CanSet] returns false. func (v Value) SetFloat(x float64) { v.mustBeAssignable() switch k := v.kind(); k { @@ -2122,7 +2126,8 @@ func (v Value) SetFloat(x float64) { } // SetInt sets v's underlying value to x. -// It panics if v's Kind is not [Int], [Int8], [Int16], [Int32], or [Int64], or if [Value.CanSet] returns false. +// It panics if v's Kind is not [Int], [Int8], [Int16], [Int32], or [Int64], +// or if [Value.CanSet] returns false. func (v Value) SetInt(x int64) { v.mustBeAssignable() switch k := v.kind(); k { @@ -2142,8 +2147,9 @@ func (v Value) SetInt(x int64) { } // SetLen sets v's length to n. -// It panics if v's Kind is not [Slice] or if n is negative or -// greater than the capacity of the slice. +// It panics if v's Kind is not [Slice], or if n is negative or +// greater than the capacity of the slice, +// or if [Value.CanSet] returns false. func (v Value) SetLen(n int) { v.mustBeAssignable() v.mustBe(Slice) @@ -2155,8 +2161,9 @@ func (v Value) SetLen(n int) { } // SetCap sets v's capacity to n. -// It panics if v's Kind is not [Slice] or if n is smaller than the length or -// greater than the capacity of the slice. +// It panics if v's Kind is not [Slice], or if n is smaller than the length or +// greater than the capacity of the slice, +// or if [Value.CanSet] returns false. func (v Value) SetCap(n int) { v.mustBeAssignable() v.mustBe(Slice) @@ -2168,7 +2175,8 @@ func (v Value) SetCap(n int) { } // SetUint sets v's underlying value to x. -// It panics if v's Kind is not [Uint], [Uintptr], [Uint8], [Uint16], [Uint32], or [Uint64], or if [Value.CanSet] returns false. +// It panics if v's Kind is not [Uint], [Uintptr], [Uint8], [Uint16], [Uint32], or [Uint64], +// or if [Value.CanSet] returns false. func (v Value) SetUint(x uint64) { v.mustBeAssignable() switch k := v.kind(); k { @@ -2190,7 +2198,8 @@ func (v Value) SetUint(x uint64) { } // SetPointer sets the [unsafe.Pointer] value v to x. -// It panics if v's Kind is not [UnsafePointer]. +// It panics if v's Kind is not [UnsafePointer] +// or if [Value.CanSet] returns false. func (v Value) SetPointer(x unsafe.Pointer) { v.mustBeAssignable() v.mustBe(UnsafePointer) @@ -2558,8 +2567,8 @@ func arrayAt(p unsafe.Pointer, i int, eltSize uintptr, whySafe string) unsafe.Po // another n elements. After Grow(n), at least n elements can be appended // to the slice without another allocation. // -// It panics if v's Kind is not a [Slice] or if n is negative or too large to -// allocate the memory. +// It panics if v's Kind is not a [Slice], or if n is negative or too large to +// allocate the memory, or if [Value.CanSet] returns false. func (v Value) Grow(n int) { v.mustBeAssignable() v.mustBe(Slice) @@ -2647,6 +2656,7 @@ func AppendSlice(s, t Value) Value { // It returns the number of elements copied. // Dst and src each must have kind [Slice] or [Array], and // dst and src must have the same element type. +// It dst is an [Array], it panics if [Value.CanSet] returns false. // // As a special case, src can have kind [String] if the element type of dst is kind [Uint8]. func Copy(dst, src Value) int { From e0c76d95abfc1621259864adb3d101cf6f1f90fc Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 10 Dec 2024 12:10:13 -0800 Subject: [PATCH 14/22] syscall: remove a wrong comment in Clearenv The comment being removed was added by commit ff3173849e (which predates Gerrit and Rietveld, so no CL link), and at the time it made sense. Since CL 148370043 (and up to the current implementation of Clearenv) the env map, which is populated by copyenv, is actually used, so the comment is no longer valid. It is also misleading, so it's best to remove it. Change-Id: I8bd2e8bca6262759538e5bcbd396f0c71cca6a4c Reviewed-on: https://go-review.googlesource.com/c/go/+/635078 Reviewed-by: Carlos Amedee Reviewed-by: Ian Lance Taylor LUCI-TryBot-Result: Go LUCI Auto-Submit: Ian Lance Taylor --- src/syscall/env_unix.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/syscall/env_unix.go b/src/syscall/env_unix.go index 8e87e018e8..1144ed1416 100644 --- a/src/syscall/env_unix.go +++ b/src/syscall/env_unix.go @@ -124,7 +124,7 @@ func Setenv(key, value string) error { } func Clearenv() { - envOnce.Do(copyenv) // prevent copyenv in Getenv/Setenv + envOnce.Do(copyenv) envLock.Lock() defer envLock.Unlock() From 6c25cf1c5fc063cc9ea27aa850ef0c4345f3a5b4 Mon Sep 17 00:00:00 2001 From: Cherry Mui Date: Tue, 10 Dec 2024 12:00:10 -0500 Subject: [PATCH 15/22] cmd/internal/objfile: break out dissassemblers to another package Currently, cmd/internal/objfile provides dissassembly routines for various architectures, which depend on dissassemblers from x/arch. cmd/internal/objfile is imported in tools that need dissassembly (objdump, pprof) and tools that don't need dissassembly (nm, addr2line). Adding/improving disassembly support for more architectures can cause binary size increase, and for some tools (nm, addr2line) it is not necessary. This CL breaks out dissassembly routines to a different package, which is only imported in tools that need dissassembly. Other tools can depend on cmd/internal/objfile without the disassembly code from x/arch. This reduces binary sizes for those tools. On darwin/arm64, old new cmd/addr2line 4554418 3648882 -20% cmd/addr2line (-ldflags=-w) 3464626 2641650 -24% cmd/nm 4503874 3616722 -20% cmd/nm (-ldflags=-w) 3430594 2609490 -24% For #70699. Change-Id: Ie45d5d5c5500c5f3882e8b3c4e6eb81f0d815292 Reviewed-on: https://go-review.googlesource.com/c/go/+/634916 LUCI-TryBot-Result: Go LUCI Reviewed-by: David Chase --- .../internal/{objfile => disasm}/disasm.go | 28 ++++++++++--------- src/cmd/internal/objfile/objfile.go | 10 ++++--- src/cmd/objdump/main.go | 3 +- src/cmd/pprof/pprof.go | 9 +++--- 4 files changed, 28 insertions(+), 22 deletions(-) rename src/cmd/internal/{objfile => disasm}/disasm.go (94%) diff --git a/src/cmd/internal/objfile/disasm.go b/src/cmd/internal/disasm/disasm.go similarity index 94% rename from src/cmd/internal/objfile/disasm.go rename to src/cmd/internal/disasm/disasm.go index 99f54143fa..c317effa90 100644 --- a/src/cmd/internal/objfile/disasm.go +++ b/src/cmd/internal/disasm/disasm.go @@ -2,13 +2,16 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -package objfile +// Package disasm provides disassembly routines. +// +// It is broken out from cmd/internal/objfile so tools that don't need +// disassembling don't need to depend on x/arch disassembler code. +package disasm import ( "bufio" "bytes" "container/list" - "debug/gosym" "encoding/binary" "fmt" "io" @@ -19,6 +22,7 @@ import ( "strings" "text/tabwriter" + "cmd/internal/objfile" "cmd/internal/src" "golang.org/x/arch/arm/armasm" @@ -32,8 +36,8 @@ import ( // Disasm is a disassembler for a given File. type Disasm struct { - syms []Sym //symbols in file, sorted by address - pcln Liner // pcln table + syms []objfile.Sym // symbols in file, sorted by address + pcln objfile.Liner // pcln table text []byte // bytes of text segment (actual instructions) textStart uint64 // start PC of text textEnd uint64 // end PC of text @@ -42,8 +46,12 @@ type Disasm struct { byteOrder binary.ByteOrder // byte order for goarch } -// Disasm returns a disassembler for the file f. -func (e *Entry) Disasm() (*Disasm, error) { +// DisasmForFile returns a disassembler for the file f. +func DisasmForFile(f *objfile.File) (*Disasm, error) { + return disasmForEntry(f.Entries()[0]) +} + +func disasmForEntry(e *objfile.Entry) (*Disasm, error) { syms, err := e.Symbols() if err != nil { return nil, err @@ -269,7 +277,7 @@ func (d *Disasm) Print(w io.Writer, filter *regexp.Regexp, start, end uint64, pr } // Decode disassembles the text segment range [start, end), calling f for each instruction. -func (d *Disasm) Decode(start, end uint64, relocs []Reloc, gnuAsm bool, f func(pc, size uint64, file string, line int, text string)) { +func (d *Disasm) Decode(start, end uint64, relocs []objfile.Reloc, gnuAsm bool, f func(pc, size uint64, file string, line int, text string)) { if start < d.textStart { start = d.textStart } @@ -452,9 +460,3 @@ var byteOrders = map[string]binary.ByteOrder{ "riscv64": binary.LittleEndian, "s390x": binary.BigEndian, } - -type Liner interface { - // Given a pc, returns the corresponding file, line, and function data. - // If unknown, returns "",0,nil. - PCToLine(uint64) (string, int, *gosym.Func) -} diff --git a/src/cmd/internal/objfile/objfile.go b/src/cmd/internal/objfile/objfile.go index 2f2d771813..ed9aae280e 100644 --- a/src/cmd/internal/objfile/objfile.go +++ b/src/cmd/internal/objfile/objfile.go @@ -119,10 +119,6 @@ func (f *File) DWARF() (*dwarf.Data, error) { return f.entries[0].DWARF() } -func (f *File) Disasm() (*Disasm, error) { - return f.entries[0].Disasm() -} - func (e *Entry) Name() string { return e.name } @@ -181,3 +177,9 @@ func (e *Entry) LoadAddress() (uint64, error) { func (e *Entry) DWARF() (*dwarf.Data, error) { return e.raw.dwarf() } + +type Liner interface { + // Given a pc, returns the corresponding file, line, and function data. + // If unknown, returns "",0,nil. + PCToLine(uint64) (string, int, *gosym.Func) +} diff --git a/src/cmd/objdump/main.go b/src/cmd/objdump/main.go index b5b0d7f517..c98551e6b8 100644 --- a/src/cmd/objdump/main.go +++ b/src/cmd/objdump/main.go @@ -40,6 +40,7 @@ import ( "strconv" "strings" + "cmd/internal/disasm" "cmd/internal/objfile" "cmd/internal/telemetry/counter" ) @@ -82,7 +83,7 @@ func main() { } defer f.Close() - dis, err := f.Disasm() + dis, err := disasm.DisasmForFile(f) if err != nil { log.Fatalf("disassemble %s: %v", flag.Arg(0), err) } diff --git a/src/cmd/pprof/pprof.go b/src/cmd/pprof/pprof.go index a1c2cd210f..bfc2911b69 100644 --- a/src/cmd/pprof/pprof.go +++ b/src/cmd/pprof/pprof.go @@ -24,6 +24,7 @@ import ( "sync" "time" + "cmd/internal/disasm" "cmd/internal/objfile" "cmd/internal/telemetry/counter" @@ -162,7 +163,7 @@ func adjustURL(source string, duration, timeout time.Duration) (string, time.Dur // (instead of invoking GNU binutils). type objTool struct { mu sync.Mutex - disasmCache map[string]*objfile.Disasm + disasmCache map[string]*disasm.Disasm } func (*objTool) Open(name string, start, limit, offset uint64, relocationSymbol string) (driver.ObjFile, error) { @@ -202,11 +203,11 @@ func (t *objTool) Disasm(file string, start, end uint64, intelSyntax bool) ([]dr return asm, nil } -func (t *objTool) cachedDisasm(file string) (*objfile.Disasm, error) { +func (t *objTool) cachedDisasm(file string) (*disasm.Disasm, error) { t.mu.Lock() defer t.mu.Unlock() if t.disasmCache == nil { - t.disasmCache = make(map[string]*objfile.Disasm) + t.disasmCache = make(map[string]*disasm.Disasm) } d := t.disasmCache[file] if d != nil { @@ -216,7 +217,7 @@ func (t *objTool) cachedDisasm(file string) (*objfile.Disasm, error) { if err != nil { return nil, err } - d, err = f.Disasm() + d, err = disasm.DisasmForFile(f) f.Close() if err != nil { return nil, err From e424d78c3da1732b72f9170e7c01f400926143ce Mon Sep 17 00:00:00 2001 From: zfdx123 <2915441170@qq.com> Date: Wed, 11 Dec 2024 04:17:11 +0000 Subject: [PATCH 16/22] internal/goos: fix bug in gengoos.go CL 601357 mistakenly added an extra period. Change-Id: I54db621663797f094059a4eb86bf5d9626fa59d6 GitHub-Last-Rev: c756e0a82427c44b00bd88547dc40bf88c85fc1f GitHub-Pull-Request: golang/go#70733 Reviewed-on: https://go-review.googlesource.com/c/go/+/634517 LUCI-TryBot-Result: Go LUCI Auto-Submit: Ian Lance Taylor Reviewed-by: Michael Pratt Reviewed-by: Ian Lance Taylor --- src/internal/goos/gengoos.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal/goos/gengoos.go b/src/internal/goos/gengoos.go index aba0d3c335..e0d4d38e89 100644 --- a/src/internal/goos/gengoos.go +++ b/src/internal/goos/gengoos.go @@ -17,7 +17,7 @@ import ( var gooses []string func main() { - data, err := os.ReadFile("../../internal/syslist/syslist..go") + data, err := os.ReadFile("../../internal/syslist/syslist.go") if err != nil { log.Fatal(err) } From 979c1cfbe8880e302d5a73df47f4efc3d34ee416 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Wed, 11 Dec 2024 15:03:59 +0000 Subject: [PATCH 17/22] net: avoid unnecessary interface lookup fetching all interface addresses InterfaceAddrs returns a list of the system's unicast interface addresses. In order to do so, the function reuses the existing helpers and list first all addresses with the netlink call RTM_GETADDR, then all interfaces with RTM_GETLINK, and later it merge both lists (each address references an interface). However, the list of interfaces and addresses are obtained at different times and there can be inconsistencies and, if an address references an interface that is not present in the list of interfaces, the function fails with an error. Since the function InterfaceAddress is only about the system addresses, there is no need to list all the interfaces, and we can obtain the list of addresses directly from the netlink call RTM_GETADDR. There is no need to correlate this list with the list of interfaces, as the OS is the source of truth and should be the one providing the consistency between addresses and interfaces. Fixes #51934 Change-Id: I3b816e8146b1c07fdfe1bf6af338f001ef75734f Reviewed-on: https://go-review.googlesource.com/c/go/+/635196 Reviewed-by: Ian Lance Taylor Commit-Queue: Ian Lance Taylor LUCI-TryBot-Result: Go LUCI Auto-Submit: Ian Lance Taylor --- src/net/interface_linux.go | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/src/net/interface_linux.go b/src/net/interface_linux.go index 9112ecc854..7856dae8fc 100644 --- a/src/net/interface_linux.go +++ b/src/net/interface_linux.go @@ -129,22 +129,14 @@ func interfaceAddrTable(ifi *Interface) ([]Addr, error) { if err != nil { return nil, os.NewSyscallError("parsenetlinkmessage", err) } - var ift []Interface - if ifi == nil { - var err error - ift, err = interfaceTable(0) - if err != nil { - return nil, err - } - } - ifat, err := addrTable(ift, ifi, msgs) + ifat, err := addrTable(ifi, msgs) if err != nil { return nil, err } return ifat, nil } -func addrTable(ift []Interface, ifi *Interface, msgs []syscall.NetlinkMessage) ([]Addr, error) { +func addrTable(ifi *Interface, msgs []syscall.NetlinkMessage) ([]Addr, error) { var ifat []Addr loop: for _, m := range msgs { @@ -153,14 +145,7 @@ loop: break loop case syscall.RTM_NEWADDR: ifam := (*syscall.IfAddrmsg)(unsafe.Pointer(&m.Data[0])) - if len(ift) != 0 || ifi.Index == int(ifam.Index) { - if len(ift) != 0 { - var err error - ifi, err = interfaceByIndex(ift, int(ifam.Index)) - if err != nil { - return nil, err - } - } + if ifi == nil || ifi.Index == int(ifam.Index) { attrs, err := syscall.ParseNetlinkRouteAttr(&m) if err != nil { return nil, os.NewSyscallError("parsenetlinkrouteattr", err) From a7c4cadce0799a74e48d394ff662ed5128667621 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Wed, 11 Dec 2024 08:59:04 -0800 Subject: [PATCH 18/22] cmd/compile: update broken link Fixes #70778 Change-Id: Ie5ed53aa39446beb0316eb134cc705ea06b37435 Reviewed-on: https://go-review.googlesource.com/c/go/+/635295 Auto-Submit: Ian Lance Taylor Reviewed-by: Keith Randall Reviewed-by: Keith Randall Reviewed-by: Ian Lance Taylor LUCI-TryBot-Result: Go LUCI --- src/cmd/compile/internal/ssa/_gen/rulegen.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cmd/compile/internal/ssa/_gen/rulegen.go b/src/cmd/compile/internal/ssa/_gen/rulegen.go index b635631501..4374d3e153 100644 --- a/src/cmd/compile/internal/ssa/_gen/rulegen.go +++ b/src/cmd/compile/internal/ssa/_gen/rulegen.go @@ -5,7 +5,8 @@ // This program generates Go code that applies rewrite rules to a Value. // The generated code implements a function of type func (v *Value) bool // which reports whether if did something. -// Ideas stolen from Swift: http://www.hpl.hp.com/techreports/Compaq-DEC/WRL-2000-2.html +// Ideas stolen from the Swift Java compiler: +// https://bitsavers.org/pdf/dec/tech_reports/WRL-2000-2.pdf package main From d5c1333eb452e37f80af797c6c26a93b00697f7f Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 11 Dec 2024 09:49:36 -0800 Subject: [PATCH 19/22] net/http: document zero value of Protocols For #67814 Change-Id: I182e9c7e720493adb9d2384336e757dace818525 Reviewed-on: https://go-review.googlesource.com/c/go/+/635335 LUCI-TryBot-Result: Go LUCI Auto-Submit: Damien Neil Reviewed-by: Austin Clements --- src/net/http/http.go | 1 + 1 file changed, 1 insertion(+) diff --git a/src/net/http/http.go b/src/net/http/http.go index 4da77889b1..32ff7e2008 100644 --- a/src/net/http/http.go +++ b/src/net/http/http.go @@ -17,6 +17,7 @@ import ( ) // Protocols is a set of HTTP protocols. +// The zero value is an empty set of protocols. // // The supported protocols are: // From 5424f2e200e022e5ddf95088118fb0914343492a Mon Sep 17 00:00:00 2001 From: Sam Thanawalla Date: Wed, 11 Dec 2024 15:41:05 +0000 Subject: [PATCH 20/22] cmd/go: add more tests for GOAUTH's user provided authenticator For #26232 Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest Change-Id: I4b6eb63d4c1d71983e1ae764a6a38744a5f01317 Reviewed-on: https://go-review.googlesource.com/c/go/+/635255 Auto-Submit: Sam Thanawalla LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Matloob --- .../go/testdata/script/goauth_userauth.txt | 94 +++++++++++++++---- 1 file changed, 75 insertions(+), 19 deletions(-) diff --git a/src/cmd/go/testdata/script/goauth_userauth.txt b/src/cmd/go/testdata/script/goauth_userauth.txt index 8403c37125..036573e07a 100644 --- a/src/cmd/go/testdata/script/goauth_userauth.txt +++ b/src/cmd/go/testdata/script/goauth_userauth.txt @@ -3,13 +3,8 @@ env GOPROXY=direct env GOSUMDB=off - -# Use a custom authenticator to provide custom credentials mkdir $WORK/bin env PATH=$WORK/bin${:}$PATH -cd auth -go build -o $WORK/bin/my-auth$GOEXE . -cd .. # Without credentials, downloading a module from a path that requires HTTPS # basic auth should fail. @@ -21,8 +16,10 @@ stderr '^\tserver response: ACCESS DENIED, buddy$' ! go mod tidy stderr '^\tserver response: ACCESS DENIED, buddy$' -# With credentials from the my-auth binary, it should succeed. -env GOAUTH='my-auth'$GOEXE' --arg1 "value with spaces"' +# Initial invocation of authenticator is successful. +go build -o $WORK/bin/basic$GOEXE scripts/basic.go +# With credentials from the binary, it should succeed. +env GOAUTH='basic'$GOEXE cp go.mod.orig go.mod go get vcs-test.golang.org/auth/or401 # go imports should resolve correctly as well. @@ -30,7 +27,54 @@ go mod tidy go list all stdout vcs-test.golang.org/auth/or401 --- auth/main.go -- +# Second invocation of authenticator is successful. +go build -o $WORK/bin/reinvocation$GOEXE scripts/reinvocation.go +# With credentials from the binary, it should succeed. +env GOAUTH='reinvocation'$GOEXE +cp go.mod.orig go.mod +go get vcs-test.golang.org/auth/or401 +# go imports should resolve correctly as well. +go mod tidy +go list all +stdout vcs-test.golang.org/auth/or401 + +# Authenticator can parse arguments correctly. +go build -o $WORK/bin/arguments$GOEXE scripts/arguments.go +# With credentials from the binary, it should succeed. +env GOAUTH='arguments'$GOEXE' --arg1 "value with spaces"' +cp go.mod.orig go.mod +go get vcs-test.golang.org/auth/or401 +# go imports should resolve correctly as well. +go mod tidy +go list all +stdout vcs-test.golang.org/auth/or401 + +# Authenticator provides bad credentials. +go build -o $WORK/bin/invalid$GOEXE scripts/invalid.go +# With credentials from the binary, it should fail. +env GOAUTH='invalid'$GOEXE +cp go.mod.orig go.mod +! go get vcs-test.golang.org/auth/or401 +stderr '^\tserver response: ACCESS DENIED, buddy$' +# go imports should fail as well. +! go mod tidy +stderr '^\tserver response: ACCESS DENIED, buddy$' + +-- go.mod.orig -- +module private.example.com +-- main.go -- +package useprivate + +import "vcs-test.golang.org/auth/or401" +-- scripts/basic.go -- +package main + +import "fmt" + +func main() { + fmt.Printf("https://vcs-test.golang.org\n\nAuthorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l\n\n") +} +-- scripts/reinvocation.go -- package main import( @@ -45,11 +89,7 @@ import( ) func main() { - arg1 := flag.String("arg1", "", "") flag.Parse() - if *arg1 != "value with spaces" { - log.Fatal("argument with spaces does not work") - } // wait for re-invocation if !strings.HasPrefix(flag.Arg(0), "https://vcs-test.golang.org") { return @@ -68,12 +108,28 @@ func main() { } fmt.Printf("https://vcs-test.golang.org\n\nAuthorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l\n\n") } +-- scripts/arguments.go -- +package main --- auth/go.mod -- -module my-auth --- go.mod.orig -- -module private.example.com --- main.go -- -package useprivate +import( + "flag" + "fmt" + "log" +) -import "vcs-test.golang.org/auth/or401" +func main() { + arg1 := flag.String("arg1", "", "") + flag.Parse() + if *arg1 != "value with spaces" { + log.Fatal("argument with spaces does not work") + } + fmt.Printf("https://vcs-test.golang.org\n\nAuthorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l\n\n") +} +-- scripts/invalid.go -- +package main + +import "fmt" + +func main() { + fmt.Printf("https://vcs-test.golang.org\n\nAuthorization: Basic invalid\n\n") +} \ No newline at end of file From 3104b6adbb36a43284f51ab0cb67c44f8ba75fac Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Wed, 11 Dec 2024 14:38:57 -0500 Subject: [PATCH 21/22] log/slog: make DiscardHandler example package-level Fixes #70782. Change-Id: I8e8b763040bd10147eb7d1a30ac0774e28f90911 Reviewed-on: https://go-review.googlesource.com/c/go/+/635217 LUCI-TryBot-Result: Go LUCI Reviewed-by: Ian Lance Taylor --- src/log/slog/example_discard_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/log/slog/example_discard_test.go b/src/log/slog/example_discard_test.go index c0cc2a65aa..3e3e37b189 100644 --- a/src/log/slog/example_discard_test.go +++ b/src/log/slog/example_discard_test.go @@ -10,7 +10,7 @@ import ( "os" ) -func ExampleDiscardHandler() { +func Example_discardHandler() { // A slog.TextHandler can output log messages. logger1 := slog.New(slog.NewTextHandler( os.Stdout, From c93477b5e563dd0ed7b45fd519762f24b7cfa7b0 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Wed, 11 Dec 2024 14:50:00 +0100 Subject: [PATCH 22/22] crypto: use provided random Reader in FIPS mode This removes the difference in behavior between FIPS mode on and off. Instead of the sentinel type we could have moved the Reader to the drbg package and checked for equality, but then we would have locked the crypto/rand.Reader implementation to the one in the FIPS module (which we might have to support for years). In internal/ed25519.GenerateKey we remove the random parameter entirely, since that function is not actually used by crypto/ed25519.GenerateKey, which instead commits to being deterministic. Fixes #70772 Change-Id: Ic1c7ca2c1cd59eb9cd090a8b235c0ce218921ac5 Reviewed-on: https://go-review.googlesource.com/c/go/+/635195 Reviewed-by: Roland Shoemaker Auto-Submit: Filippo Valsorda Reviewed-by: Russ Cox LUCI-TryBot-Result: Go LUCI --- src/crypto/ecdh/nist.go | 5 +++ src/crypto/ecdsa/ecdsa.go | 7 ++++ src/crypto/internal/fips140/drbg/rand.go | 37 +++++++++++++++++++ src/crypto/internal/fips140/ecdh/ecdh.go | 20 +++------- src/crypto/internal/fips140/ecdsa/cast.go | 3 +- src/crypto/internal/fips140/ecdsa/ecdsa.go | 23 ++---------- .../internal/fips140/ed25519/ed25519.go | 19 ++-------- src/crypto/internal/fips140/rsa/keygen.go | 14 ++----- src/crypto/internal/fips140/rsa/pkcs1v22.go | 21 ++--------- .../internal/fips140only/fips140only.go | 7 ++++ src/crypto/internal/fips140test/cast_test.go | 2 +- src/crypto/rand/rand.go | 4 +- src/crypto/rsa/fips.go | 9 +++++ src/crypto/rsa/rsa.go | 3 ++ src/crypto/rsa/rsa_test.go | 4 -- 15 files changed, 94 insertions(+), 84 deletions(-) diff --git a/src/crypto/ecdh/nist.go b/src/crypto/ecdh/nist.go index 0f4a65e5af..acef829894 100644 --- a/src/crypto/ecdh/nist.go +++ b/src/crypto/ecdh/nist.go @@ -8,6 +8,7 @@ import ( "bytes" "crypto/internal/boring" "crypto/internal/fips140/ecdh" + "crypto/internal/fips140only" "errors" "io" ) @@ -43,6 +44,10 @@ func (c *nistCurve) GenerateKey(rand io.Reader) (*PrivateKey, error) { return k, nil } + if fips140only.Enabled && !fips140only.ApprovedRandomReader(rand) { + return nil, errors.New("crypto/ecdh: only crypto/rand.Reader is allowed in FIPS 140-only mode") + } + privateKey, err := c.generate(rand) if err != nil { return nil, err diff --git a/src/crypto/ecdsa/ecdsa.go b/src/crypto/ecdsa/ecdsa.go index 0ad669795c..77727aaf96 100644 --- a/src/crypto/ecdsa/ecdsa.go +++ b/src/crypto/ecdsa/ecdsa.go @@ -21,6 +21,7 @@ import ( "crypto/internal/boring" "crypto/internal/boring/bbig" "crypto/internal/fips140/ecdsa" + "crypto/internal/fips140only" "crypto/internal/randutil" "crypto/sha512" "crypto/subtle" @@ -182,6 +183,9 @@ func GenerateKey(c elliptic.Curve, rand io.Reader) (*PrivateKey, error) { } func generateFIPS[P ecdsa.Point[P]](curve elliptic.Curve, c *ecdsa.Curve[P], rand io.Reader) (*PrivateKey, error) { + if fips140only.Enabled && fips140only.ApprovedRandomReader(rand) { + return nil, errors.New("crypto/ecdsa: only crypto/rand.Reader is allowed in FIPS 140-only mode") + } privateKey, err := ecdsa.GenerateKey(c, rand) if err != nil { return nil, err @@ -228,6 +232,9 @@ func SignASN1(rand io.Reader, priv *PrivateKey, hash []byte) ([]byte, error) { } func signFIPS[P ecdsa.Point[P]](c *ecdsa.Curve[P], priv *PrivateKey, rand io.Reader, hash []byte) ([]byte, error) { + if fips140only.Enabled && !fips140only.ApprovedRandomReader(rand) { + return nil, errors.New("crypto/ecdsa: only crypto/rand.Reader is allowed in FIPS 140-only mode") + } // privateKeyToFIPS is very slow in FIPS mode because it performs a // Sign+Verify cycle per FIPS 140-3 IG 10.3.A. We should find a way to cache // it or attach it to the PrivateKey. diff --git a/src/crypto/internal/fips140/drbg/rand.go b/src/crypto/internal/fips140/drbg/rand.go index 736a4b0cc0..967fb0673e 100644 --- a/src/crypto/internal/fips140/drbg/rand.go +++ b/src/crypto/internal/fips140/drbg/rand.go @@ -7,7 +7,9 @@ package drbg import ( "crypto/internal/entropy" "crypto/internal/fips140" + "crypto/internal/randutil" "crypto/internal/sysrand" + "io" "sync" ) @@ -56,3 +58,38 @@ func Read(b []byte) { b = b[size:] } } + +// DefaultReader is a sentinel type, embedded in the default +// [crypto/rand.Reader], used to recognize it when passed to +// APIs that accept a rand io.Reader. +type DefaultReader interface{ defaultReader() } + +// ReadWithReader uses Reader to fill b with cryptographically secure random +// bytes. It is intended for use in APIs that expose a rand io.Reader. +// +// If Reader is not the default Reader from crypto/rand, +// [randutil.MaybeReadByte] and [fips140.RecordNonApproved] are called. +func ReadWithReader(r io.Reader, b []byte) error { + if _, ok := r.(DefaultReader); ok { + Read(b) + return nil + } + + fips140.RecordNonApproved() + randutil.MaybeReadByte(r) + _, err := io.ReadFull(r, b) + return err +} + +// ReadWithReaderDeterministic is like ReadWithReader, but it doesn't call +// [randutil.MaybeReadByte] on non-default Readers. +func ReadWithReaderDeterministic(r io.Reader, b []byte) error { + if _, ok := r.(DefaultReader); ok { + Read(b) + return nil + } + + fips140.RecordNonApproved() + _, err := io.ReadFull(r, b) + return err +} diff --git a/src/crypto/internal/fips140/ecdh/ecdh.go b/src/crypto/internal/fips140/ecdh/ecdh.go index 19a45c00db..bf71c75a92 100644 --- a/src/crypto/internal/fips140/ecdh/ecdh.go +++ b/src/crypto/internal/fips140/ecdh/ecdh.go @@ -10,7 +10,6 @@ import ( "crypto/internal/fips140/drbg" "crypto/internal/fips140/nistec" "crypto/internal/fips140deps/byteorder" - "crypto/internal/randutil" "errors" "io" "math/bits" @@ -137,8 +136,6 @@ var p521Order = []byte{0x01, 0xff, } // GenerateKey generates a new ECDSA private key pair for the specified curve. -// -// In FIPS mode, rand is ignored. func GenerateKey[P Point[P]](c *Curve[P], rand io.Reader) (*PrivateKey, error) { fips140.RecordApproved() // This procedure is equivalent to Key Pair Generation by Testing @@ -146,18 +143,13 @@ func GenerateKey[P Point[P]](c *Curve[P], rand io.Reader) (*PrivateKey, error) { for { key := make([]byte, len(c.N)) - if fips140.Enabled { - drbg.Read(key) - } else { - randutil.MaybeReadByte(rand) - if _, err := io.ReadFull(rand, key); err != nil { - return nil, err - } - // In tests, rand will return all zeros and NewPrivateKey will reject - // the zero key as it generates the identity as a public key. This also - // makes this function consistent with crypto/elliptic.GenerateKey. - key[1] ^= 0x42 + if err := drbg.ReadWithReader(rand, key); err != nil { + return nil, err } + // In tests, rand will return all zeros and NewPrivateKey will reject + // the zero key as it generates the identity as a public key. This also + // makes this function consistent with crypto/elliptic.GenerateKey. + key[1] ^= 0x42 // Mask off any excess bits if the size of the underlying field is not a // whole number of bytes, which is only the case for P-521. diff --git a/src/crypto/internal/fips140/ecdsa/cast.go b/src/crypto/internal/fips140/ecdsa/cast.go index a324cf929d..219b7211e7 100644 --- a/src/crypto/internal/fips140/ecdsa/cast.go +++ b/src/crypto/internal/fips140/ecdsa/cast.go @@ -54,7 +54,8 @@ func testHash() []byte { func fipsPCT[P Point[P]](c *Curve[P], k *PrivateKey) error { return fips140.PCT("ECDSA PCT", func() error { hash := testHash() - sig, err := Sign(c, sha512.New, k, nil, hash) + drbg := newDRBG(sha512.New, k.d, bits2octets(P256(), hash), nil) + sig, err := sign(c, k, drbg, hash) if err != nil { return err } diff --git a/src/crypto/internal/fips140/ecdsa/ecdsa.go b/src/crypto/internal/fips140/ecdsa/ecdsa.go index 61b40122a0..9459b03de7 100644 --- a/src/crypto/internal/fips140/ecdsa/ecdsa.go +++ b/src/crypto/internal/fips140/ecdsa/ecdsa.go @@ -10,7 +10,6 @@ import ( "crypto/internal/fips140/bigmod" "crypto/internal/fips140/drbg" "crypto/internal/fips140/nistec" - "crypto/internal/randutil" "errors" "io" "sync" @@ -187,20 +186,11 @@ func NewPublicKey[P Point[P]](c *Curve[P], Q []byte) (*PublicKey, error) { } // GenerateKey generates a new ECDSA private key pair for the specified curve. -// -// In FIPS mode, rand is ignored. func GenerateKey[P Point[P]](c *Curve[P], rand io.Reader) (*PrivateKey, error) { fips140.RecordApproved() k, Q, err := randomPoint(c, func(b []byte) error { - if fips140.Enabled { - drbg.Read(b) - return nil - } else { - randutil.MaybeReadByte(rand) - _, err := io.ReadFull(rand, b) - return err - } + return drbg.ReadWithReader(rand, b) }) if err != nil { return nil, err @@ -281,8 +271,6 @@ type Signature struct { // the hash function H) using the private key, priv. If the hash is longer than // the bit-length of the private key's curve order, the hash will be truncated // to that length. -// -// The signature is randomized. If FIPS mode is enabled, rand is ignored. func Sign[P Point[P], H fips140.Hash](c *Curve[P], h func() H, priv *PrivateKey, rand io.Reader, hash []byte) (*Signature, error) { if priv.pub.curve != c.curve { return nil, errors.New("ecdsa: private key does not match curve") @@ -296,13 +284,8 @@ func Sign[P Point[P], H fips140.Hash](c *Curve[P], h func() H, priv *PrivateKey, // advantage of closely resembling Deterministic ECDSA. Z := make([]byte, len(priv.d)) - if fips140.Enabled { - drbg.Read(Z) - } else { - randutil.MaybeReadByte(rand) - if _, err := io.ReadFull(rand, Z); err != nil { - return nil, err - } + if err := drbg.ReadWithReader(rand, Z); err != nil { + return nil, err } // See https://github.com/cfrg/draft-irtf-cfrg-det-sigs-with-noise/issues/6 diff --git a/src/crypto/internal/fips140/ed25519/ed25519.go b/src/crypto/internal/fips140/ed25519/ed25519.go index 9824cbdf81..bbdc5b4a8b 100644 --- a/src/crypto/internal/fips140/ed25519/ed25519.go +++ b/src/crypto/internal/fips140/ed25519/ed25519.go @@ -11,7 +11,6 @@ import ( "crypto/internal/fips140/edwards25519" "crypto/internal/fips140/sha512" "errors" - "io" "strconv" ) @@ -61,24 +60,14 @@ func (pub *PublicKey) Bytes() []byte { } // GenerateKey generates a new Ed25519 private key pair. -// -// In FIPS mode, rand is ignored. Otherwise, the output of this function is -// deterministic, and equivalent to reading 32 bytes from rand, and passing them -// to [NewKeyFromSeed]. -func GenerateKey(rand io.Reader) (*PrivateKey, error) { +func GenerateKey() (*PrivateKey, error) { priv := &PrivateKey{} - return generateKey(priv, rand) + return generateKey(priv) } -func generateKey(priv *PrivateKey, rand io.Reader) (*PrivateKey, error) { +func generateKey(priv *PrivateKey) (*PrivateKey, error) { fips140.RecordApproved() - if fips140.Enabled { - drbg.Read(priv.seed[:]) - } else { - if _, err := io.ReadFull(rand, priv.seed[:]); err != nil { - return nil, err - } - } + drbg.Read(priv.seed[:]) precomputePrivateKey(priv) if err := fipsPCT(priv); err != nil { // This clearly can't happen, but FIPS 140-3 requires that we check. diff --git a/src/crypto/internal/fips140/rsa/keygen.go b/src/crypto/internal/fips140/rsa/keygen.go index a9e12eb1e8..df76772ef5 100644 --- a/src/crypto/internal/fips140/rsa/keygen.go +++ b/src/crypto/internal/fips140/rsa/keygen.go @@ -8,15 +8,12 @@ import ( "crypto/internal/fips140" "crypto/internal/fips140/bigmod" "crypto/internal/fips140/drbg" - "crypto/internal/randutil" "errors" "io" ) // GenerateKey generates a new RSA key pair of the given bit size. // bits must be at least 128. -// -// When operating in FIPS mode, rand is ignored. func GenerateKey(rand io.Reader, bits int) (*PrivateKey, error) { if bits < 128 { return nil, errors.New("rsa: key too small") @@ -94,7 +91,7 @@ func GenerateKey(rand io.Reader, bits int) (*PrivateKey, error) { } // randomPrime returns a random prime number of the given bit size following -// the process in FIPS 186-5, Appendix A.1.3. rand is ignored in FIPS mode. +// the process in FIPS 186-5, Appendix A.1.3. func randomPrime(rand io.Reader, bits int) ([]byte, error) { if bits < 64 { return nil, errors.New("rsa: prime size must be at least 32-bit") @@ -102,13 +99,8 @@ func randomPrime(rand io.Reader, bits int) ([]byte, error) { b := make([]byte, (bits+7)/8) for { - if fips140.Enabled { - drbg.Read(b) - } else { - randutil.MaybeReadByte(rand) - if _, err := io.ReadFull(rand, b); err != nil { - return nil, err - } + if err := drbg.ReadWithReader(rand, b); err != nil { + return nil, err } if excess := len(b)*8 - bits; excess != 0 { b[0] >>= excess diff --git a/src/crypto/internal/fips140/rsa/pkcs1v22.go b/src/crypto/internal/fips140/rsa/pkcs1v22.go index a62d7e485f..a5bc56dafc 100644 --- a/src/crypto/internal/fips140/rsa/pkcs1v22.go +++ b/src/crypto/internal/fips140/rsa/pkcs1v22.go @@ -264,8 +264,6 @@ func PSSMaxSaltLength(pub *PublicKey, hash fips140.Hash) (int, error) { } // SignPSS calculates the signature of hashed using RSASSA-PSS. -// -// In FIPS mode, rand is ignored and can be nil. func SignPSS(rand io.Reader, priv *PrivateKey, hash fips140.Hash, hashed []byte, saltLength int) ([]byte, error) { fipsSelfTest() fips140.RecordApproved() @@ -286,12 +284,8 @@ func SignPSS(rand io.Reader, priv *PrivateKey, hash fips140.Hash, hashed []byte, fips140.RecordNonApproved() } salt := make([]byte, saltLength) - if fips140.Enabled { - drbg.Read(salt) - } else { - if _, err := io.ReadFull(rand, salt); err != nil { - return nil, err - } + if err := drbg.ReadWithReaderDeterministic(rand, salt); err != nil { + return nil, err } emBits := priv.pub.N.BitLen() - 1 @@ -374,8 +368,6 @@ func checkApprovedHash(hash fips140.Hash) { } // EncryptOAEP encrypts the given message with RSAES-OAEP. -// -// In FIPS mode, random is ignored and can be nil. func EncryptOAEP(hash, mgfHash fips140.Hash, random io.Reader, pub *PublicKey, msg []byte, label []byte) ([]byte, error) { // Note that while we don't commit to deterministic execution with respect // to the random stream, we also don't apply MaybeReadByte, so per Hyrum's @@ -408,13 +400,8 @@ func EncryptOAEP(hash, mgfHash fips140.Hash, random io.Reader, pub *PublicKey, m db[len(db)-len(msg)-1] = 1 copy(db[len(db)-len(msg):], msg) - if fips140.Enabled { - drbg.Read(seed) - } else { - _, err := io.ReadFull(random, seed) - if err != nil { - return nil, err - } + if err := drbg.ReadWithReaderDeterministic(random, seed); err != nil { + return nil, err } mgf1XOR(db, mgfHash, seed) diff --git a/src/crypto/internal/fips140only/fips140only.go b/src/crypto/internal/fips140only/fips140only.go index 6ad97befbe..7126781af0 100644 --- a/src/crypto/internal/fips140only/fips140only.go +++ b/src/crypto/internal/fips140only/fips140only.go @@ -5,11 +5,13 @@ package fips140only import ( + "crypto/internal/fips140/drbg" "crypto/internal/fips140/sha256" "crypto/internal/fips140/sha3" "crypto/internal/fips140/sha512" "hash" "internal/godebug" + "io" ) // Enabled reports whether FIPS 140-only mode is enabled, in which non-approved @@ -24,3 +26,8 @@ func ApprovedHash(h hash.Hash) bool { return false } } + +func ApprovedRandomReader(r io.Reader) bool { + _, ok := r.(drbg.DefaultReader) + return ok +} diff --git a/src/crypto/internal/fips140test/cast_test.go b/src/crypto/internal/fips140test/cast_test.go index c6e3212f3f..b2aee15eab 100644 --- a/src/crypto/internal/fips140test/cast_test.go +++ b/src/crypto/internal/fips140test/cast_test.go @@ -85,7 +85,7 @@ func TestConditionals(t *testing.T) { t.Fatal(err) } ecdsa.SignDeterministic(ecdsa.P256(), sha256.New, kDSA, make([]byte, 32)) - k25519, err := ed25519.GenerateKey(rand.Reader) + k25519, err := ed25519.GenerateKey() if err != nil { t.Fatal(err) } diff --git a/src/crypto/rand/rand.go b/src/crypto/rand/rand.go index 5dd875e6e7..1ca16caa95 100644 --- a/src/crypto/rand/rand.go +++ b/src/crypto/rand/rand.go @@ -38,7 +38,9 @@ func init() { Reader = &reader{} } -type reader struct{} +type reader struct { + drbg.DefaultReader +} func (r *reader) Read(b []byte) (n int, err error) { boring.Unreachable() diff --git a/src/crypto/rsa/fips.go b/src/crypto/rsa/fips.go index bc23d59709..24dfb38cf6 100644 --- a/src/crypto/rsa/fips.go +++ b/src/crypto/rsa/fips.go @@ -17,6 +17,9 @@ import ( const ( // PSSSaltLengthAuto causes the salt in a PSS signature to be as large // as possible when signing, and to be auto-detected when verifying. + // + // When signing in FIPS 140-3 mode, the salt length is capped at the length + // of the hash function used in the signature. PSSSaltLengthAuto = 0 // PSSSaltLengthEqualsHash causes the salt length to equal the length // of the hash used in the signature. @@ -67,6 +70,9 @@ func SignPSS(rand io.Reader, priv *PrivateKey, hash crypto.Hash, digest []byte, if fips140only.Enabled && !fips140only.ApprovedHash(hash.New()) { return nil, errors.New("crypto/rsa: use of hash functions other than SHA-2 or SHA-3 is not allowed in FIPS 140-only mode") } + if fips140only.Enabled && !fips140only.ApprovedRandomReader(rand) { + return nil, errors.New("crypto/rsa: only crypto/rand.Reader is allowed in FIPS 140-only mode") + } if opts != nil && opts.Hash != 0 { hash = opts.Hash @@ -188,6 +194,9 @@ func EncryptOAEP(hash hash.Hash, random io.Reader, pub *PublicKey, msg []byte, l if fips140only.Enabled && !fips140only.ApprovedHash(hash) { return nil, errors.New("crypto/rsa: use of hash functions other than SHA-2 or SHA-3 is not allowed in FIPS 140-only mode") } + if fips140only.Enabled && !fips140only.ApprovedRandomReader(random) { + return nil, errors.New("crypto/rsa: only crypto/rand.Reader is allowed in FIPS 140-only mode") + } defer hash.Reset() diff --git a/src/crypto/rsa/rsa.go b/src/crypto/rsa/rsa.go index 0f58f2226f..fb23f003a6 100644 --- a/src/crypto/rsa/rsa.go +++ b/src/crypto/rsa/rsa.go @@ -322,6 +322,9 @@ func GenerateKey(random io.Reader, bits int) (*PrivateKey, error) { if fips140only.Enabled && bits%2 == 1 { return nil, errors.New("crypto/rsa: use of keys with odd size is not allowed in FIPS 140-only mode") } + if fips140only.Enabled && !fips140only.ApprovedRandomReader(random) { + return nil, errors.New("crypto/rsa: only crypto/rand.Reader is allowed in FIPS 140-only mode") + } k, err := rsa.GenerateKey(random, bits) if err != nil { diff --git a/src/crypto/rsa/rsa_test.go b/src/crypto/rsa/rsa_test.go index c395732c8b..2474ab82df 100644 --- a/src/crypto/rsa/rsa_test.go +++ b/src/crypto/rsa/rsa_test.go @@ -10,7 +10,6 @@ import ( "crypto" "crypto/internal/boring" "crypto/internal/cryptotest" - "crypto/internal/fips140" "crypto/rand" . "crypto/rsa" "crypto/sha1" @@ -782,9 +781,6 @@ type testEncryptOAEPStruct struct { } func TestEncryptOAEP(t *testing.T) { - if fips140.Enabled { - t.Skip("FIPS mode overrides the deterministic random source") - } sha1 := sha1.New() n := new(big.Int) for i, test := range testEncryptOAEPData {