Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/util/debugutil: introduce debug.Stack() wrapper #136288

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arjunmahishi
Copy link
Contributor

@arjunmahishi arjunmahishi commented Nov 27, 2024

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 commit fixes that with the following changes:

  • 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.

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

  • 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

@arjunmahishi arjunmahishi requested review from a team as code owners November 27, 2024 12:17
@arjunmahishi arjunmahishi requested review from kyle-a-wong, aa-joshi, nameisbhaskar, vidit-bhat and itsbilal and removed request for a team November 27, 2024 12:17
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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 commit fixes that with the following changes:

  * 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.

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

  * 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
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you split the 3 bullet points into separate commits? Easier to review search/replace operations when they're confined to a commit.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @arjunmahishi, @itsbilal, @kyle-a-wong, @nameisbhaskar, and @vidit-bhat)


pkg/util/debugutil/debugutil.go line 53 at r1 (raw file):

// unnecessary redaction.
func Stack() interfaces.SafeValue {
	return redact.Safe(string(debug.Stack()))

I believe string will allocate a copy of the stack byte slice.
Can you do this via type alias and avoid an allocation?

type SafeStack []byte

func (SafeStack) SafeValue() {}

SafeStack(debug.Stack())

Alternatively, you can use unsafe.String if the above doesn't work.

Copy link
Member

@itsbilal itsbilal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly a transit-by review as I got tagged


// Stack wraps the output of debug.Stack() with redact.Safe() to avoid
// unnecessary redaction.
func Stack() interfaces.SafeValue {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given we've had issues in the past with high system CPU usage due to excessive use of debug.Stack in error handling pathways, we should probably add a warning comment here to the effect of "Calling debug.Stack grabs system-level locks and could cause high system CPU usage resulting in the Go runtime to lock up if called too frequently, even if called only in error-handling pathways. Use sporadically and only when necessary."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants