Merge cockroachdb#136288

136288: pkg/util/debugutil: introduce debug.Stack() wrapper r=dhartunian a=arjunmahishi

Until now, all stack traces were treated as untyped strings and hence redacted. Stack traces don't need to be redacted. Because they don't contain any PII. This PR fixes that with the following commits:

1. Introduce a new function `Stack()` in the `debugutil` package as a replacement for the standard `debug.Stack()`. This new function wraps the stack trace contents in `redact.Safe()` to make sure that it does not get redacted.

2. Replace the all the current uses of `debug.Stack()` across the codebase with `debugutil.Stack()`

3. Add a linter rule to block future uses of `debug.Stack()`. The linter error will nudge users to use `debugutil.Stack()` instead.

Part of: CRDB-15292
Epic: CRDB-37533
Release note: None

Co-authored-by: Arjun Mahishi <[email protected]>

pkg/BUILD.bazel

@@ -692,6 +692,7 @@ ALL_TESTS = [
     "//pkg/util/ctxgroup:ctxgroup_test",
     "//pkg/util/ctxlog:ctxlog_test",
     "//pkg/util/ctxutil:ctxutil_test",
+    "//pkg/util/debugutil:debugutil_test",
     "//pkg/util/duration:duration_test",
     "//pkg/util/encoding/csv:csv_test",
     "//pkg/util/encoding:encoding_test",
@@ -2459,6 +2460,7 @@ GO_TARGETS = [
     "//pkg/util/ctxutil:ctxutil",
     "//pkg/util/ctxutil:ctxutil_test",
     "//pkg/util/debugutil:debugutil",
+    "//pkg/util/debugutil:debugutil_test",
     "//pkg/util/duration:duration",
     "//pkg/util/duration:duration_test",
     "//pkg/util/encoding/csv:csv",

pkg/cmd/roachtest/roachtestutil/mixedversion/BUILD.bazel

@@ -30,6 +30,7 @@ go_library(
         "//pkg/roachprod/vm",
         "//pkg/testutils/release",
         "//pkg/util/ctxgroup",
+        "//pkg/util/debugutil",
         "//pkg/util/intsets",
         "//pkg/util/randutil",
         "//pkg/util/retry",

pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go

@@ -13,7 +13,6 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
-	"runtime/debug"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -30,6 +29,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
+	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
@@ -868,7 +868,7 @@ func panicAsError(l *logger.Logger, f func() error) (retErr error) {
 // logPanicToErr logs the panic stack trace and returns an error with the
 // panic message.
 func logPanicToErr(l *logger.Logger, r interface{}) error {
-	l.Printf("panic stack trace:\n%s", string(debug.Stack()))
+	l.Printf("panic stack trace:\n%s", debugutil.Stack())
 	return fmt.Errorf("panic (stack trace above): %v", r)
 }
 

pkg/config/zonepb/BUILD.bazel

@@ -18,6 +18,7 @@ go_library(
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/sem/tree",
+        "//pkg/util/debugutil",
         "//pkg/util/envutil",
         "//pkg/util/log",
         "@com_github_cockroachdb_errors//:errors",

pkg/config/zonepb/zone_yaml.go

@@ -7,10 +7,10 @@ package zonepb
 
 import (
 	"fmt"
-	"runtime/debug"
 	"sort"
 	"strings"
 
+	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/errors"
 	"github.com/gogo/protobuf/proto"
 	"gopkg.in/yaml.v2"
@@ -50,13 +50,13 @@ var _ yaml.Unmarshaler = &ConstraintsConjunction{}
 // MarshalYAML implements yaml.Marshaler.
 func (c ConstraintsConjunction) MarshalYAML() (interface{}, error) {
 	return nil, fmt.Errorf(
-		"MarshalYAML should never be called directly on Constraints (%v): %v", c, debug.Stack())
+		"MarshalYAML should never be called directly on Constraints (%v): %v", c, debugutil.Stack())
 }
 
 // UnmarshalYAML implements yaml.Marshaler.
 func (c *ConstraintsConjunction) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	return fmt.Errorf(
-		"UnmarshalYAML should never be called directly on Constraints: %v", debug.Stack())
+		"UnmarshalYAML should never be called directly on Constraints: %v", debugutil.Stack())
 }
 
 // ConstraintsList is an alias for a slice of Constraints that can be

pkg/kv/kvclient/kvcoord/BUILD.bazel

@@ -65,6 +65,7 @@ go_library(
         "//pkg/util/buildutil",
         "//pkg/util/circuit",
         "//pkg/util/ctxgroup",
+        "//pkg/util/debugutil",
         "//pkg/util/envutil",
         "//pkg/util/errorutil/unimplemented",
         "//pkg/util/future",

pkg/kv/kvclient/kvcoord/txn_coord_sender.go

@@ -8,7 +8,6 @@ package kvcoord
 import (
 	"context"
 	"math/rand"
-	"runtime/debug"
 
 	"github.com/cockroachdb/cockroach/pkg/build"
 	"github.com/cockroachdb/cockroach/pkg/kv"
@@ -17,6 +16,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
+	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -734,7 +734,7 @@ func (tc *TxnCoordSender) maybeRejectClientLocked(
 			// If the client is trying to do anything other than rollback, it is
 			// unexpected for it to find the transaction already in a txnFinalized
 			// state. This may be a bug, so log a stack trace.
-			stack := string(debug.Stack())
+			stack := debugutil.Stack()
 			log.Errorf(ctx, "%s. stack:\n%s", msg, stack)
 		}
 		reason := kvpb.TransactionStatusError_REASON_UNKNOWN

pkg/kv/kvserver/BUILD.bazel

@@ -207,6 +207,7 @@ go_library(
         "//pkg/util/buildutil",
         "//pkg/util/circuit",
         "//pkg/util/ctxgroup",
+        "//pkg/util/debugutil",
         "//pkg/util/encoding",
         "//pkg/util/envutil",
         "//pkg/util/errorutil",

pkg/kv/kvserver/concurrency/BUILD.bazel

@@ -32,6 +32,7 @@ go_library(
         "//pkg/storage/enginepb",
         "//pkg/util/buildutil",
         "//pkg/util/container/list",
+        "//pkg/util/debugutil",
         "//pkg/util/hlc",
         "//pkg/util/humanizeutil",
         "//pkg/util/log",

pkg/kv/kvserver/concurrency/concurrency_manager.go

@@ -8,7 +8,6 @@ package concurrency
 import (
 	"context"
 	"fmt"
-	"runtime/debug"
 	"sync"
 
 	"github.com/cockroachdb/cockroach/pkg/kv"
@@ -22,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
+	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
@@ -302,7 +302,7 @@ func (m *managerImpl) sequenceReqWithGuard(
 				panic(redact.Safe(fmt.Sprintf("must not be holding latches\n"+
 					"this is tracked in github.com/cockroachdb/cockroach/issues/77663; please comment if seen\n"+
 					"eval_kind=%d, holding_latches=%t, branch=%d, first_iteration=%t, stack=\n%s",
-					g.EvalKind, g.HoldingLatches(), branch, firstIteration, string(debug.Stack()))))
+					g.EvalKind, g.HoldingLatches(), branch, firstIteration, debugutil.Stack())))
 			}
 			log.Event(ctx, "optimistic failed, so waiting for latches")
 			g.lg, err = m.lm.WaitUntilAcquired(ctx, g.lg)

pkg/kv/kvserver/intentresolver/BUILD.bazel

@@ -21,6 +21,7 @@ go_library(
         "//pkg/settings",
         "//pkg/settings/cluster",
         "//pkg/storage/enginepb",
+        "//pkg/util/debugutil",
         "//pkg/util/hlc",
         "//pkg/util/log",
         "//pkg/util/metric",

pkg/kv/kvserver/intentresolver/intent_resolver.go

@@ -9,7 +9,6 @@ import (
 	"bytes"
 	"context"
 	"math"
-	"runtime/debug"
 	"sort"
 	"time"
 
@@ -24,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
+	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/quotapool"
@@ -1034,7 +1034,7 @@ func (ir *IntentResolver) resolveIntents(
 	// if !build.IsRelease() && h == (kvpb.AdmissionHeader{}) && ir.everyAdmissionHeaderMissing.ShouldLog() {
 	if false {
 		log.Warningf(ctx,
-			"test-only warning: if you see this, please report to https://github.com/cockroachdb/cockroach/issues/112680. empty admission header provided by %s", string(debug.Stack()))
+			"test-only warning: if you see this, please report to https://github.com/cockroachdb/cockroach/issues/112680. empty admission header provided by %s", debugutil.Stack())
 	}
 	// Send the requests ...
 	if opts.sendImmediately {

pkg/kv/kvserver/kvserverbase/BUILD.bazel

@@ -21,6 +21,7 @@ go_library(
         "//pkg/roachpb",
         "//pkg/settings",
         "//pkg/settings/cluster",
+        "//pkg/util/debugutil",
         "//pkg/util/errorutil",
         "//pkg/util/hlc",
         "//pkg/util/log",

pkg/kv/kvserver/kvserverbase/syncing_write.go

@@ -8,12 +8,12 @@ package kvserverbase
 import (
 	"context"
 	"os"
-	"runtime/debug"
 	"strings"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
@@ -53,7 +53,7 @@ func LimitBulkIOWrite(ctx context.Context, limiter *rate.Limiter, cost int) erro
 
 	if d := timeutil.Since(begin); d > bulkIOWriteLimiterLongWait {
 		log.Warningf(ctx, "bulk io write limiter took %s (>%s):\n%s",
-			d, bulkIOWriteLimiterLongWait, debug.Stack())
+			d, bulkIOWriteLimiterLongWait, debugutil.Stack())
 	}
 	return nil
 }

pkg/kv/kvserver/metrics.go

@@ -8,7 +8,6 @@ package kvserver
 import (
 	"context"
 	"fmt"
-	"runtime/debug"
 	"sync/atomic"
 	"time"
 
@@ -24,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage/disk"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
+	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
 	"github.com/cockroachdb/cockroach/pkg/util/metric/aggmetric"
@@ -3004,15 +3004,15 @@ type tenantMetricsRef struct {
 	// in assertions on failure.
 	_stack struct {
 		syncutil.Mutex
-		string
+		debugutil.SafeStack
 	}
 }
 
 func (ref *tenantMetricsRef) assert(ctx context.Context) {
 	if atomic.LoadInt32(&ref._state) != 0 {
 		ref._stack.Lock()
 		defer ref._stack.Unlock()
-		log.FatalfDepth(ctx, 1, "tenantMetricsRef already finalized in:\n%s", ref._stack.string)
+		log.FatalfDepth(ctx, 1, "tenantMetricsRef already finalized in:\n%s", ref._stack.SafeStack)
 	}
 }
 
@@ -3162,7 +3162,7 @@ func (sm *TenantsStorageMetrics) releaseTenant(ctx context.Context, ref *tenantM
 		return          // unreachable
 	}
 	ref._stack.Lock()
-	ref._stack.string = string(debug.Stack())
+	ref._stack.SafeStack = debugutil.Stack()
 	ref._stack.Unlock()
 	m.mu.Lock()
 	defer m.mu.Unlock()

pkg/kv/kvserver/spanset/BUILD.bazel

@@ -14,6 +14,7 @@ go_library(
         "//pkg/roachpb",
         "//pkg/storage",
         "//pkg/storage/fs",
+        "//pkg/util/debugutil",
         "//pkg/util/hlc",
         "//pkg/util/log",
         "//pkg/util/protoutil",

pkg/kv/kvserver/spanset/spanset.go

@@ -8,12 +8,12 @@ package spanset
 import (
 	"context"
 	"fmt"
-	"runtime/debug"
 	"strings"
 	"sync"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/errors"
@@ -360,7 +360,7 @@ func (s *SpanSet) checkAllowed(
 		}
 	}
 
-	return errors.Errorf("cannot %s undeclared span %s\ndeclared:\n%s\nstack:\n%s", access, span, s, debug.Stack())
+	return errors.Errorf("cannot %s undeclared span %s\ndeclared:\n%s\nstack:\n%s", access, span, s, debugutil.Stack())
 }
 
 // contains returns whether s1 contains s2. Unlike Span.Contains, this function

pkg/kv/kvserver/store_merge.go

@@ -8,10 +8,10 @@ package kvserver
 import (
 	"context"
 	"runtime"
-	"runtime/debug"
 
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/readsummary/rspb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/errors"
@@ -38,7 +38,7 @@ func (s *Store) maybeAssertNoHole(ctx context.Context, from, to roachpb.RKey) fu
 	}
 
 	goroutineStopped := make(chan struct{})
-	caller := string(debug.Stack())
+	caller := debugutil.Stack()
 	if from.Equal(roachpb.RKeyMax) {
 		// There will be a hole to the right of RKeyMax but it's just the end of
 		// the addressable keyspace.

pkg/spanconfig/spanconfigsqlwatcher/BUILD.bazel

@@ -28,6 +28,7 @@ go_library(
         "//pkg/sql/sem/tree",
         "//pkg/sql/types",
         "//pkg/util",
+        "//pkg/util/debugutil",
         "//pkg/util/hlc",
         "//pkg/util/log",
         "//pkg/util/log/logcrash",

pkg/spanconfig/spanconfigsqlwatcher/zonesdecoder.go

@@ -6,15 +6,14 @@
 package spanconfigsqlwatcher
 
 import (
-	"runtime/debug"
-
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
 	"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
+	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/errors"
 )
 
@@ -40,7 +39,7 @@ func (zd *zonesDecoder) DecodePrimaryKey(key roachpb.Key) (descpb.ID, error) {
 	types := []*types.T{tbl.PublicColumns()[0].GetType()}
 	startKeyRow := make([]rowenc.EncDatum, 1)
 	if _, err := rowenc.DecodeIndexKey(zd.codec, startKeyRow, nil /* colDirs */, key); err != nil {
-		return descpb.InvalidID, errors.NewAssertionErrorWithWrappedErrf(err, "failed to decode key in system.zones %v %v", key, string(debug.Stack()))
+		return descpb.InvalidID, errors.NewAssertionErrorWithWrappedErrf(err, "failed to decode key in system.zones %v %v", key, debugutil.Stack())
 	}
 	if err := startKeyRow[0].EnsureDecoded(types[0], &zd.alloc); err != nil {
 		return descpb.InvalidID, errors.NewAssertionErrorWithWrappedErrf(err, "failed to decode key in system.zones %v", key)

pkg/sql/catalog/lease/BUILD.bazel

@@ -144,6 +144,7 @@ go_test(
         "//pkg/util/admission",
         "//pkg/util/allstacks",
         "//pkg/util/ctxgroup",
+        "//pkg/util/debugutil",
         "//pkg/util/encoding",
         "//pkg/util/hlc",
         "//pkg/util/leaktest",

pkg/sql/catalog/lease/lease_test.go

@@ -12,7 +12,6 @@ import (
 	"context"
 	gosql "database/sql"
 	"fmt"
-	"runtime/debug"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -64,6 +63,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/allstacks"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
+	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -936,7 +936,7 @@ func TestDescriptorRefreshOnRetry(t *testing.T) {
 				RemoveOnceDereferenced: true,
 				LeaseAcquiredEvent: func(desc catalog.Descriptor, _ error) {
 					if desc.GetName() == "foo" {
-						log.Infof(ctx, "lease acquirer stack trace: %s", debug.Stack())
+						log.Infof(ctx, "lease acquirer stack trace: %s", debugutil.Stack())
 						atomic.AddInt32(&fooAcquiredCount, 1)
 					}
 				},
@@ -1323,7 +1323,7 @@ func TestLeaseRenewedAutomatically(testingT *testing.T) {
 				// see a block event dump a stack to aid in debugging.
 				log.Infof(ctx,
 					"Lease acquisition of ID %d resulted in a block event. Stack trace to follow:\n%s",
-					id, debug.Stack())
+					id, debugutil.Stack())
 			},
 		},
 	}