*: fix the fmtsafe linter and fix the fallout

[knz]

Found while working on #49447.

The fmtsafe linter added in #48040 #48048 was actually
malfunctioning because it was not stripping the vendor prefix
properly.

This patch fixes it.

Fixing the linter uncovered a range of defects throughout the
remainder of the code, some benign and some outright bugs.

Examples:

  return pgerror.Wrapf(err, "while running %s", stmt)

(The second argument should be the pgcode, not the error string!)

 return errors.Errorf(`no consistency checks are defined for %s` + gen.Meta().Name)

(Incompatible use of %s and string concatenation.)

  errors.New(fmt.Sprintf("foo %s", blah))

(Should use Newf() instead to capture more data in telemetry.)

Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

Everything under sql/ LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @irfansharif, and @tbg)

[jordanlewis]

I don't like that this a requirement in our codebase, to be honest... does this really feel ergonomic or useful to you as a lint rule? I won't block this PR (thanks very much for doing this toil) but I do feel this deserves some questioning.

[RaduBerinde]

I think this rule is trying to make sure we don't pass off PII information as what the error library expects is a simple hardcoded message. The case below is more relevant - errors.Wrap(err, typedExpr.String()). Do we want the string representation of the typedExpr to be treated as non-PII?

[dt]

does the error library expect the argument to Wrap (not wrapf), to be a simple hardcoded message?

[knz]

does the error library expect the argument to Wrap (not wrapf), to be a simple hardcoded message

No it does not. Jordan is right. I'll explain more in a comment under.

[irfansharif]

LGTM. I can only imagine how cumbersome these fixes are, thanks for doing them.

[irfansharif]

[nit] Update comment to mention the enclosing fn name?

[irfansharif]

Unrelated to this PR, is it generally 'safer' to use %q over %s for byte slices? If so, is this also something we could/should use a linter for?

[irfansharif]

[nit] Here and below, I have a slight preference for pulling out the wrapping logic out of maybeDescriptorChangedError and simply wrapping at the callsites, as before. I do like the return type being changed to an error, we should keep that.

[irfansharif]

Maybe even keeping the comment block as to why we're wrapping vs. not is a good idea.

[knz]

I don't like that this a requirement in our codebase, to be honest... [...] I do feel this deserves some questioning.

Thanks Jordan for this feedback, it forced me to think more about it. I was already unsure about this when I introduced it in #48040.

Thanks to your feedback I think I found a solution for some things, but there are other things that are still giving me pause.

I think we need some brainstorming on this, so I'd like you to bear with me and provide some mind share.

Background

Henceforth I will use the word "safe" to mean "known certainly to be free of confidential information, PII or other details which neither telemetry nor support should receive without explicit permission". Then "unsafe" means the negative, when we're not sure (i.e. stuff is unsafe unless proven otherwise).

So for context here is where we are:

• for formatting calls like log.Infof, errors.Newf, errors.Wrapf (i.e. not the target of your comment), we now assume that the format string is always safe for reporting to telemetry, and then the positional arguments are conditionally safe depending on certain factors (not detailed here):

  • this assumption is actually part of the API contract for the errors library since the beginning, and for the log package since util/log,*: enforce literal strings in non-formatting log calls #48048.
  • we enforce the assumption throughout cockroachdb via a linter.

• for non-formatting calls we have the following:

  • for log calls, e.g. log.Info, we very much want to assume that the single argument is safe, because otherwise the log anonymization work proposed in util/log: automatic log redaction #48051 is seriously compromised. So I introduced this restriction in util/log,*: enforce literal strings in non-formatting log calls #48048 and is now enforced by linter.

  • for error constructions, foremost errors.New and errors.Wrap which causes us concern, there's a duality that creates tension:

    • on the one hand, error objects eventually get printed in logs and telemetry, and we want to maximize the amount of information that is considered safe (to maximize the utility of logs and sentry reports).

    • on the other hand, we do not consider the argument of these constructors as safe: it's not been part of the API so far, because I wanted to make this errors library drop-in compatible with github.com/pkg/errors and I wasn't comfortable forcing the requirement of safety to users "behind their back", risking exposing unsafe error payloads as "safe".

tldr: errors.New() / errors.Wrap() capture unsafe data, but we'd prefer safe data instead.

Problems solved here (probably no discussion needed)

• we had many calls like errors.New(fmt.Sprintf("safe details: %v", xx)).

  These needed to be fixed. The linter proposed here suggests errors.Newf("safe details: %v", xx)) which is better.

• we had many calls to errors.New(xx) / errors.Wrap(.., xx) where xx was assigned the result of fmt.Sprintf or some bytes.Buffer just before.

  These benefit from a formatting calls that capture more safe details. The linter proposed here invited that.

Problem 1: dynamic, foremost-unsafe payloads

We have a bunch of calls to errors.New() and errors.Wrap() that use a dynamically-generated, foremost-unsafe explanatory payload.

Both you and I agree that errors.Wrapf(err, "%s", payload) is objectively worse than errors.Wrap(err, payload)

Here's a proposal:

// Here 'payload' is turned from 'string' into 'interface{}'.
// The WrapV function inspects the type, and considers the value as unsafe
// unless it was constructed with errors.Safe().
func WrapV(err error, maybeSafeVal interface{})

Use with:

errors.WrapV(xx, errors.Safe(myVal)) (safe)

errors.WrapV(xx, myVal) (unsafe)

This effectively makes errors.WrapV(err, x) an alias for errors.Wrapf(err, "%v", x).

Then we'd change the linter to mandate errors.WrapV for every call that uses a variable argument.

This aligns the API with what we already do for logging.

Problem 2 (biggest) literal constants assumed unsafe

Consider all the calls to errors.New() / errors.Wrap() that use a literal constant argument, especially all the calls like var errSentinel = errors.New("some constant text").

The API consider the argument unsafe. This is the elephant in the room.

• In an ideal world (like in C/C++/Rust/Java/ML), Go would let me inspect in a function body whether a string argument was allocated, or coming from the read-only data segment of the executable (a literal). Then I would use this information conditionally to decide whether a string argument is safe for reporting or not.

  Unfortunately, the go linker (in contrast to every other linker out there) does not distinguish read-only and read/write static data.

• In another ideal world (C++/Rust/Java), my type system would have constexpr and overloads, then I'd have errors.New(v constexpr string) and errors.New(v string) side-by-side and consider one safe, the other one unsafe.

  Unfortunately the go type system is not adequate in this way.

• In another ideal world (C/C++/Rust), I'd have a preprocessor, and then I'd be preprocessing the source code automatically to transform errors.New("xxx") to errors.NewWithSafeString("xxx") conditionally depending on whether the processor saw a literal string passed as argument or nto.

  Unfortunately go does not give me a preprocessor or a macro system.

Alternative solutions to problem 2

Here are the solutions I have considered:

1. do the same for errors.New() and errors.Wrap() as for the log.Info calls:

   • change the API to start assuming the argument is safe for reporting
   • enforce that in a linter by enforcing it to be a literal string
   • for convenience, provide an alternative errors.NewV(arg ...interface{}) that conditionally decides whether the arg is safe or not, as a shorthand for errors.New("%v", arg), ditto errors.WrapV() see this pr.
   • pros makes most of current error sentinels reportable in one swoop
   • cons breaking API change for the errors library, makes it impossible to substitute cockroachdb/errors for other errors libraries in 3rd party packages without auditing.

2. keep the API contract for errors.New() / errors.Wrap(), but introduce errors.SafeNew() / errors.SafeWrap() which always consider their argument as safe.

   • create a linter that requires all calls to errors.New() / errors.Wrap() with a literal argument to be rewritten to use SafeNew() / SafeWrap() instead.
   • pros backward-compatibility, maximum clarity when reading the code
   • cons maximum utility can only be obtained by converting all our existing error calls - that's a massive code lift.

3. introduce a Go preprocessor, which detects calls to errors.New() on a literal constant and automatically coverts that to errors.SafeNew() during compilation.

   • pros best ergonomics! drop-in compatible.
   • pros a preprocessor will help us solve many other problems we already have throughout CockroachDB, especially around logging, tracing and testing knobs.
   • cons can't rely on the go front-end program to drive the entire build. Likely needs us to move to bazel first, otherwise we'd be in Makefile hell trying to track the go dependencies ourselves.

4. (Proposed by @RaduBerinde ) define type SafeString string and then change errors.New() and errors.Wrap() to accept an SafeString instead of a string. The Go typing rules ensure that literals can be passed, but not string values without an explicit cast.

   • a linter could then disallow SafeString casts altogether, and suggest using NewV / WrapV instead
   • pros we get the ergonomics we like: literals are safe automatically, other values are only safe if designated explicitly to be
   • cons breaking API change for the errors library, makes it impossible to substitute cockroachdb/errors for other errors libraries in 3rd party packages without auditing. (same as (1) above)

For the record my preference goes for (3).

The next best solution IMHO is (1), which is where this PR (and #48040) are going. We haven't yet made the switch to change the API contract, but we need to have the linter and convert the calls first before we can start assuming the argument safe.

Edit after Radu's comment below: maybe (4) is better than (1), need to investigate.

Any other suggestions?

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @irfansharif, and @tbg)

---

pkg/cmd/github-post/main.go, line 165 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Unrelated to this PR, is it generally 'safer' to use %q over %s for byte slices? If so, is this also something we could/should use a linter for?

No there is nothing "safer" about it.

It's a question of aesthetics: if the byte slice contains whitespace or other special characters, %q ensures it remains on a single line of output.
(There's nothing semantically wrong for an error message to span multiple lines, but it's likely to confuse the human reader)

---

pkg/kv/kvserver/replica_command.go, line 409 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Maybe even keeping the comment block as to why we're wrapping vs. not is a good idea.

Good idea, done.

---

pkg/testutils/lint/passes/fmtsafe/fmtsafe.go, line 99 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

[nit] Update comment to mention the enclosing fn name?

Done.

[RaduBerinde]

Any other suggestions?

What if we define type SafeString string and use that as the argument for New and Wrap? Then any string literals can be passed directly, but you have to do a manual cast for anything else or use the f variants (which is what we want - the user should to be forced to do something if they want to use a non-literal as something safe). The only disadvantage AFAICT is that it won't be a drop-in for github.com/pkg/errors.

[dt]

I’m more inclined towards 1) I guess — I’m not sure how 3) would work even with bazel — most of our codegen generates entire files but it doesn’t modify the files I wrote, which feels like another level of spooky and hard to reason about. I’d be more inclined to do something simple (build-wise) like say (and lint to enforce that) we only pass string literals to errors.Wrap and if you want to pass a dynamically generated string, you need use another method like Wrapf or the proposed Wrapv

[knz]

What if we define type SafeString string and use that as the argument for New and Wrap? Then any string literals can be passed directly, but you have to do a manual cast for anything else

That is clever and intriguing. I need to think about it a bit more and run some experiments. It'll be alternative (4).

[knz]

Some more input here:

• Both options (1) and (4) are two flavors of the same. Option (4) is marginally better because it enforces the contract without requiring a linter, but the trade-offs remain the same.

• @bdarnell has expressed a preference for (1) to me as well.

[knz]

Ok so I'd like to proceed with this PR with the understanding we're going to shift to solution (1) (or perhaps (4) ) at some point soon.

In this later phase, I will extend the linter to recommend using errors.New / errors.Wrap every time that ...("%v", val) is used, and then proceed to perform the simplification.

@jordanlewis is this acceptable to you?

[jordanlewis]

Yes, by no means do I intend to block this work. I'm fine merging this, thanks.

[knz]

Filed #49752 to track this followup work.

[knz]

bors r=irfansharif

[craig]

Build succeeded

• GitHub CI (Cockroach)