From 14c1de7c7602bd3889438a7e2fd00a0bd90429a3 Mon Sep 17 00:00:00 2001 From: EulerOSWander <314264452@qq.com> Date: Thu, 5 Dec 2024 19:13:59 +0800 Subject: [PATCH] backport: runtime: put ReadMemStats debug assertions behind a double-check mode --- ...o1.21-runtime-put-ReadMemStats-debug.patch | 213 ++++++++++++++++++ golang.spec | 9 +- 2 files changed, 221 insertions(+), 1 deletion(-) create mode 100644 backport-0025-release-branch.go1.21-runtime-put-ReadMemStats-debug.patch diff --git a/backport-0025-release-branch.go1.21-runtime-put-ReadMemStats-debug.patch b/backport-0025-release-branch.go1.21-runtime-put-ReadMemStats-debug.patch new file mode 100644 index 0000000..1c9e5b4 --- /dev/null +++ b/backport-0025-release-branch.go1.21-runtime-put-ReadMemStats-debug.patch @@ -0,0 +1,213 @@ +From 398023279e53f4e847d598afa41c2d3d163baa94 Mon Sep 17 00:00:00 2001 +From: Michael Anthony Knyszek +Date: Mon, 27 Nov 2023 22:27:32 +0000 +Subject: [PATCH 09/20] [release-branch.go1.21] runtime: put ReadMemStats debug + assertions behind a double-check mode +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Conflict:NA +Reference:https://go-review.googlesource.com/c/go/+/545277 +ReadMemStats has a few assertions it makes about the consistency of the +stats it's about to produce. Specifically, how those stats line up with +runtime-internal stats. These checks are generally useful, but crashing +just because some stats are wrong is a heavy price to pay. + +For a long time this wasn't a problem, but very recently it became a +real problem. It turns out that there's real benign skew that can happen +wherein sysmon (which doesn't synchronize with a STW) generates a trace +event when tracing is enabled, and may mutate some stats while +ReadMemStats is running its checks. + +Fix this by synchronizing with both sysmon and the tracer. This is a bit +heavy-handed, but better that than false positives. + +Also, put the checks behind a debug mode. We want to reduce the risk of +backporting this change, and again, it's not great to crash just because +user-facing stats are off. Still, enable this debug mode during the +runtime tests so we don't lose quite as much coverage from disabling +these checks by default. + +For #64401. +Fixes #64410. + +Change-Id: I9adb3e5c7161d207648d07373a11da8a5f0fda9a +Reviewed-on: https://go-review.googlesource.com/c/go/+/545277 +LUCI-TryBot-Result: Go LUCI +Reviewed-by: Michael Pratt +Reviewed-by: Felix Geisendörfer +(cherry picked from commit b2efd1de97402ec4b8fb4e9e0ec29c8e49e8e200) +Reviewed-on: https://go-review.googlesource.com/c/go/+/545557 +Auto-Submit: Matthew Dempsky +TryBot-Bypass: Matthew Dempsky +--- + src/runtime/export_test.go | 2 + + src/runtime/gc_test.go | 5 ++ + src/runtime/mstats.go | 114 +++++++++++++++++++++---------------- + 3 files changed, 71 insertions(+), 50 deletions(-) + +diff --git a/src/runtime/export_test.go b/src/runtime/export_test.go +index f7ce5033f587..a740993c5878 100644 +--- a/src/runtime/export_test.go ++++ b/src/runtime/export_test.go +@@ -436,6 +436,8 @@ func ReadMetricsSlow(memStats *MemStats, samplesp unsafe.Pointer, len, cap int) + startTheWorld() + } + ++var DoubleCheckReadMemStats = &doubleCheckReadMemStats ++ + // ReadMemStatsSlow returns both the runtime-computed MemStats and + // MemStats accumulated by scanning the heap. + func ReadMemStatsSlow() (base, slow MemStats) { +diff --git a/src/runtime/gc_test.go b/src/runtime/gc_test.go +index bd01e3610300..9302ea32c3f1 100644 +--- a/src/runtime/gc_test.go ++++ b/src/runtime/gc_test.go +@@ -570,6 +570,11 @@ func TestPageAccounting(t *testing.T) { + } + } + ++func init() { ++ // Enable ReadMemStats' double-check mode. ++ *runtime.DoubleCheckReadMemStats = true ++} ++ + func TestReadMemStats(t *testing.T) { + base, slow := runtime.ReadMemStatsSlow() + if base != slow { +diff --git a/src/runtime/mstats.go b/src/runtime/mstats.go +index 9cdc56513719..308bed67d7b3 100644 +--- a/src/runtime/mstats.go ++++ b/src/runtime/mstats.go +@@ -367,6 +367,11 @@ func ReadMemStats(m *MemStats) { + startTheWorld() + } + ++// doubleCheckReadMemStats controls a double-check mode for ReadMemStats that ++// ensures consistency between the values that ReadMemStats is using and the ++// runtime-internal stats. ++var doubleCheckReadMemStats = false ++ + // readmemstats_m populates stats for internal runtime values. + // + // The world must be stopped. +@@ -441,56 +446,65 @@ func readmemstats_m(stats *MemStats) { + + heapGoal := gcController.heapGoal() + +- // The world is stopped, so the consistent stats (after aggregation) +- // should be identical to some combination of memstats. In particular: +- // +- // * memstats.heapInUse == inHeap +- // * memstats.heapReleased == released +- // * memstats.heapInUse + memstats.heapFree == committed - inStacks - inWorkBufs - inPtrScalarBits +- // * memstats.totalAlloc == totalAlloc +- // * memstats.totalFree == totalFree +- // +- // Check if that's actually true. +- // +- // TODO(mknyszek): Maybe don't throw here. It would be bad if a +- // bug in otherwise benign accounting caused the whole application +- // to crash. +- if gcController.heapInUse.load() != uint64(consStats.inHeap) { +- print("runtime: heapInUse=", gcController.heapInUse.load(), "\n") +- print("runtime: consistent value=", consStats.inHeap, "\n") +- throw("heapInUse and consistent stats are not equal") +- } +- if gcController.heapReleased.load() != uint64(consStats.released) { +- print("runtime: heapReleased=", gcController.heapReleased.load(), "\n") +- print("runtime: consistent value=", consStats.released, "\n") +- throw("heapReleased and consistent stats are not equal") +- } +- heapRetained := gcController.heapInUse.load() + gcController.heapFree.load() +- consRetained := uint64(consStats.committed - consStats.inStacks - consStats.inWorkBufs - consStats.inPtrScalarBits) +- if heapRetained != consRetained { +- print("runtime: global value=", heapRetained, "\n") +- print("runtime: consistent value=", consRetained, "\n") +- throw("measures of the retained heap are not equal") +- } +- if gcController.totalAlloc.Load() != totalAlloc { +- print("runtime: totalAlloc=", gcController.totalAlloc.Load(), "\n") +- print("runtime: consistent value=", totalAlloc, "\n") +- throw("totalAlloc and consistent stats are not equal") +- } +- if gcController.totalFree.Load() != totalFree { +- print("runtime: totalFree=", gcController.totalFree.Load(), "\n") +- print("runtime: consistent value=", totalFree, "\n") +- throw("totalFree and consistent stats are not equal") +- } +- // Also check that mappedReady lines up with totalMapped - released. +- // This isn't really the same type of "make sure consistent stats line up" situation, +- // but this is an opportune time to check. +- if gcController.mappedReady.Load() != totalMapped-uint64(consStats.released) { +- print("runtime: mappedReady=", gcController.mappedReady.Load(), "\n") +- print("runtime: totalMapped=", totalMapped, "\n") +- print("runtime: released=", uint64(consStats.released), "\n") +- print("runtime: totalMapped-released=", totalMapped-uint64(consStats.released), "\n") +- throw("mappedReady and other memstats are not equal") ++ if doubleCheckReadMemStats { ++ // Only check this if we're debugging. It would be bad to crash an application ++ // just because the debugging stats are wrong. We mostly rely on tests to catch ++ // these issues, and we enable the double check mode for tests. ++ // ++ // The world is stopped, so the consistent stats (after aggregation) ++ // should be identical to some combination of memstats. In particular: ++ // ++ // * memstats.heapInUse == inHeap ++ // * memstats.heapReleased == released ++ // * memstats.heapInUse + memstats.heapFree == committed - inStacks - inWorkBufs - inPtrScalarBits ++ // * memstats.totalAlloc == totalAlloc ++ // * memstats.totalFree == totalFree ++ // ++ // Check if that's actually true. ++ // ++ // Prevent sysmon and the tracer from skewing the stats since they can ++ // act without synchronizing with a STW. See #64401. ++ lock(&sched.sysmonlock) ++ lock(&trace.lock) ++ if gcController.heapInUse.load() != uint64(consStats.inHeap) { ++ print("runtime: heapInUse=", gcController.heapInUse.load(), "\n") ++ print("runtime: consistent value=", consStats.inHeap, "\n") ++ throw("heapInUse and consistent stats are not equal") ++ } ++ if gcController.heapReleased.load() != uint64(consStats.released) { ++ print("runtime: heapReleased=", gcController.heapReleased.load(), "\n") ++ print("runtime: consistent value=", consStats.released, "\n") ++ throw("heapReleased and consistent stats are not equal") ++ } ++ heapRetained := gcController.heapInUse.load() + gcController.heapFree.load() ++ consRetained := uint64(consStats.committed - consStats.inStacks - consStats.inWorkBufs - consStats.inPtrScalarBits) ++ if heapRetained != consRetained { ++ print("runtime: global value=", heapRetained, "\n") ++ print("runtime: consistent value=", consRetained, "\n") ++ throw("measures of the retained heap are not equal") ++ } ++ if gcController.totalAlloc.Load() != totalAlloc { ++ print("runtime: totalAlloc=", gcController.totalAlloc.Load(), "\n") ++ print("runtime: consistent value=", totalAlloc, "\n") ++ throw("totalAlloc and consistent stats are not equal") ++ } ++ if gcController.totalFree.Load() != totalFree { ++ print("runtime: totalFree=", gcController.totalFree.Load(), "\n") ++ print("runtime: consistent value=", totalFree, "\n") ++ throw("totalFree and consistent stats are not equal") ++ } ++ // Also check that mappedReady lines up with totalMapped - released. ++ // This isn't really the same type of "make sure consistent stats line up" situation, ++ // but this is an opportune time to check. ++ if gcController.mappedReady.Load() != totalMapped-uint64(consStats.released) { ++ print("runtime: mappedReady=", gcController.mappedReady.Load(), "\n") ++ print("runtime: totalMapped=", totalMapped, "\n") ++ print("runtime: released=", uint64(consStats.released), "\n") ++ print("runtime: totalMapped-released=", totalMapped-uint64(consStats.released), "\n") ++ throw("mappedReady and other memstats are not equal") ++ } ++ unlock(&trace.lock) ++ unlock(&sched.sysmonlock) + } + + // We've calculated all the values we need. Now, populate stats. +-- +2.33.0 + diff --git a/golang.spec b/golang.spec index 842d5b6..3d40413 100644 --- a/golang.spec +++ b/golang.spec @@ -66,7 +66,7 @@ Name: golang Version: 1.21.4 -Release: 26 +Release: 27 Summary: The Go Programming Language License: BSD and Public Domain URL: https://golang.org/ @@ -144,6 +144,7 @@ Patch6021: backport-0021-CVE-2024-34155-track-depth-in-nested-element-lists.patc Patch6022: backport-0022-release-branch.go1.21-fix-CVE-2024-34156.patch Patch6023: backport-0023-go-build-constraint-add-parsing-limits.patch Patch6024: backport-0024-release-branch.go1.21-runtime-add-the-disablethp-GOD.patch +Patch6025: backport-0025-release-branch.go1.21-runtime-put-ReadMemStats-debug.patch ExclusiveArch: %{golang_arches} @@ -383,6 +384,12 @@ fi %files devel -f go-tests.list -f go-misc.list -f go-src.list %changelog +* Thu Dec 05 2024 hewenliang <314264452@qq.com> - 1.21.4-27 +- Type:bugfix +- CVE:NA +- SUG:NA +- DESC:put ReadMemStats debug assertions behind a double-check mode + * Mon Nov 18 2024 Vanient - 1.21.4-26 - runtime: add the disablethp GODEBUG setting -- Gitee