Merge #137080 #137682 #137699

137080: metamorphic: add environment-based overrides r=asg0451 a=stevendanna This allows developers to override specific metamorphic variables: ./dev test ./pkg/foo -- --test_env COCKROACH_INTERNAL_METAMORPHIC_OVERRIDES=var1=val1,var2=val2 Metamorphic values not specified are still randomly chosen. In a future commit, it would also be nice to be able to specify a file that allows you to specify all values. Epic: none Informs #114039 137682: pgwire: rename ReadBuffer.GetString to GetUnsafeString r=fqazi a=fqazi Previously, it was not clear that GetString would not copy data from the ReadBuffer. This could be problematic if the object was long lived, since the entire buffer would have been kept alive. To reduce risk, this patch renames GetString to GetUnsafeString. It also adds a GetSafeString method for cases where a copy is needed. The latter is adopted inside: parseClientProvidedSessionParameters Informs: #137627 Release note: None 137699: kvflowcontrol: fix logging redaction for a few types r=pav-kv a=pav-kv The strings inside `SafeFormat()` were not marked as safe, and caused the values being printed as redactable. Part of #136526 Co-authored-by: Steven Danna <[email protected]> Co-authored-by: Faizan Qazi <[email protected]> Co-authored-by: Pavel Kalinnikov <[email protected]>
cockroachdb · Dec 19, 2024 · 95460dd · 95460dd
4 parents 8e9e897 + 17f1619 + 82abfd4 + c0a40f9
commit 95460dd
Show file tree

Hide file tree

Showing 11 changed files with 262 additions and 109 deletions.
diff --git a/pkg/kv/kvserver/kvflowcontrol/BUILD.bazel b/pkg/kv/kvserver/kvflowcontrol/BUILD.bazel
@@ -18,7 +18,7 @@ go_library(
         "//pkg/settings/cluster",
         "//pkg/util/admission/admissionpb",
         "//pkg/util/ctxutil",
+        "//pkg/util/humanizeutil",
         "@com_github_cockroachdb_redact//:redact",
-        "@com_github_dustin_go_humanize//:go-humanize",
     ],
 )
diff --git a/pkg/kv/kvserver/kvflowcontrol/kvflowcontrol.go b/pkg/kv/kvserver/kvflowcontrol/kvflowcontrol.go
@@ -19,8 +19,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/util/admission/admissionpb"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxutil"
+	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
 	"github.com/cockroachdb/redact"
-	"github.com/dustin/go-humanize"
 )
 
 // Enabled determines whether we use flow control for replication traffic in KV.
@@ -70,12 +70,12 @@ func (m ModeT) String() string {
 }
 
 // SafeFormat implements the redact.SafeFormatter interface.
-func (m ModeT) SafeFormat(p redact.SafePrinter, verb rune) {
+func (m ModeT) SafeFormat(p redact.SafePrinter, _ rune) {
 	if s, ok := modeDict[m]; ok {
-		p.Print(s)
+		p.SafeString(redact.SafeString(s))
 		return
 	}
-	p.Print("unknown-mode")
+	p.SafeString("unknown-mode")
 }
 
 // RegularTokensPerStream determines the flow tokens available for regular work
@@ -415,26 +415,25 @@ func (t Tokens) String() string {
 }
 
 // SafeFormat implements the redact.SafeFormatter interface.
-func (t Tokens) SafeFormat(p redact.SafePrinter, verb rune) {
-	sign := "+"
+func (t Tokens) SafeFormat(p redact.SafePrinter, _ rune) {
 	if t < 0 {
-		sign = "-"
-		t = -t
+		p.SafeString(humanizeutil.IBytes(int64(t)))
+		return
 	}
-	p.Printf("%s%s", sign, humanize.IBytes(uint64(t)))
+	p.Printf("+%s", humanizeutil.IBytes(int64(t)))
 }
 
 func (s Stream) String() string {
 	return redact.StringWithoutMarkers(s)
 }
 
 // SafeFormat implements the redact.SafeFormatter interface.
-func (s Stream) SafeFormat(p redact.SafePrinter, verb rune) {
+func (s Stream) SafeFormat(p redact.SafePrinter, _ rune) {
 	tenantSt := s.TenantID.String()
 	if s.TenantID.IsSystem() {
 		tenantSt = "1"
 	}
-	p.Printf("t%s/s%s", tenantSt, s.StoreID.String())
+	p.Printf("t%s/s%s", redact.SafeString(tenantSt), s.StoreID)
 }
 
 var raftAdmissionMetaKey = ctxutil.RegisterFastValueKey()

diff --git a/pkg/kv/kvserver/kvflowcontrol/replica_rac2/processor.go b/pkg/kv/kvserver/kvflowcontrol/replica_rac2/processor.go
@@ -922,7 +922,7 @@ func (p *processorImpl) AdmitRaftEntriesRaftMuLocked(ctx context.Context, e rac2
 			if isV2Encoding {
 				log.Infof(ctx,
 					"decoded v2 raft admission meta below-raft: pri=%v create-time=%d "+
-						"proposer=n%v receiver=[n%d,s%v] tenant=t%d tokens≈%d "+
+						"proposer=n%v receiver=[n%d,s%v] tenant=t%d tokens≈%v "+
 						"sideloaded=%t raft-entry=%d/%d lead-v2=%v",
 					raftpb.Priority(meta.AdmissionPriority),
 					meta.AdmissionCreateTime,
@@ -939,7 +939,7 @@ func (p *processorImpl) AdmitRaftEntriesRaftMuLocked(ctx context.Context, e rac2
 			} else {
 				log.Infof(ctx,
 					"decoded v1 raft admission meta below-raft: pri=%v create-time=%d "+
-						"proposer=n%v receiver=[n%d,s%v] tenant=t%d tokens≈%d "+
+						"proposer=n%v receiver=[n%d,s%v] tenant=t%d tokens≈%v "+
 						"sideloaded=%t raft-entry=%d/%d lead-v2=%v",
 					admissionpb.WorkPriority(meta.AdmissionPriority),
 					meta.AdmissionCreateTime,

diff --git a/pkg/sql/pgwire/auth_methods.go b/pkg/sql/pgwire/auth_methods.go
@@ -350,7 +350,7 @@ func scramAuthenticator(
 			// SASLResponse messages contain just the SASL payload.
 			//
 			rb := pgwirebase.ReadBuffer{Msg: resp}
-			reqMethod, err := rb.GetString()
+			reqMethod, err := rb.GetUnsafeString()
 			if err != nil {
 				c.LogAuthFailed(ctx, eventpb.AuthFailReason_PRE_HOOK_ERROR, err)
 				return err

diff --git a/pkg/sql/pgwire/conn.go b/pkg/sql/pgwire/conn.go
@@ -346,7 +346,7 @@ func (c *conn) handleSimpleQuery(
 	timeReceived crtime.Mono,
 	unqualifiedIntSize *types.T,
 ) error {
-	query, err := buf.GetString()
+	query, err := buf.GetUnsafeString()
 	if err != nil {
 		return c.stmtBuf.Push(ctx, sql.SendError{Err: err})
 	}
@@ -493,11 +493,11 @@ func (c *conn) handleSimpleQuery(
 // the connection should be considered toast.
 func (c *conn) handleParse(ctx context.Context, nakedIntSize *types.T) error {
 	telemetry.Inc(sqltelemetry.ParseRequestCounter)
-	name, err := c.readBuf.GetString()
+	name, err := c.readBuf.GetUnsafeString()
 	if err != nil {
 		return c.stmtBuf.Push(ctx, sql.SendError{Err: err})
 	}
-	query, err := c.readBuf.GetString()
+	query, err := c.readBuf.GetUnsafeString()
 	if err != nil {
 		return c.stmtBuf.Push(ctx, sql.SendError{Err: err})
 	}
@@ -608,7 +608,7 @@ func (c *conn) handleDescribe(ctx context.Context) error {
 	if err != nil {
 		return c.stmtBuf.Push(ctx, sql.SendError{Err: err})
 	}
-	name, err := c.readBuf.GetString()
+	name, err := c.readBuf.GetUnsafeString()
 	if err != nil {
 		return c.stmtBuf.Push(ctx, sql.SendError{Err: err})
 	}
@@ -628,7 +628,7 @@ func (c *conn) handleClose(ctx context.Context) error {
 	if err != nil {
 		return c.stmtBuf.Push(ctx, sql.SendError{Err: err})
 	}
-	name, err := c.readBuf.GetString()
+	name, err := c.readBuf.GetUnsafeString()
 	if err != nil {
 		return c.stmtBuf.Push(ctx, sql.SendError{Err: err})
 	}
@@ -650,11 +650,11 @@ var formatCodesAllText = []pgwirebase.FormatCode{pgwirebase.FormatText}
 // the connection should be considered toast.
 func (c *conn) handleBind(ctx context.Context) error {
 	telemetry.Inc(sqltelemetry.BindRequestCounter)
-	portalName, err := c.readBuf.GetString()
+	portalName, err := c.readBuf.GetUnsafeString()
 	if err != nil {
 		return c.stmtBuf.Push(ctx, sql.SendError{Err: err})
 	}
-	statementName, err := c.readBuf.GetString()
+	statementName, err := c.readBuf.GetUnsafeString()
 	if err != nil {
 		return c.stmtBuf.Push(ctx, sql.SendError{Err: err})
 	}
@@ -776,7 +776,7 @@ func (c *conn) handleExecute(
 	ctx context.Context, timeReceived crtime.Mono, followedBySync bool,
 ) error {
 	telemetry.Inc(sqltelemetry.ExecuteRequestCounter)
-	portalName, err := c.readBuf.GetString()
+	portalName, err := c.readBuf.GetUnsafeString()
 	if err != nil {
 		return c.stmtBuf.Push(ctx, sql.SendError{Err: err})
 	}

diff --git a/pkg/sql/pgwire/pgwirebase/encoding.go b/pkg/sql/pgwire/pgwirebase/encoding.go
@@ -213,8 +213,10 @@ func (b *ReadBuffer) ReadTypedMsg(rd BufferedReader) (ClientMessageType, int, er
 	return ClientMessageType(typ), n, err
 }
 
-// GetString reads a null-terminated string.
-func (b *ReadBuffer) GetString() (string, error) {
+// GetUnsafeString reads a null-terminated string as a reference.
+// Note: The underlying buffer will be prevented from GCing, so long lived
+// objects should never use this.
+func (b *ReadBuffer) GetUnsafeString() (string, error) {
 	pos := bytes.IndexByte(b.Msg, 0)
 	if pos == -1 {
 		return "", NewProtocolViolationErrorf("NUL terminator not found")
@@ -226,6 +228,16 @@ func (b *ReadBuffer) GetString() (string, error) {
 	return s, nil
 }
 
+// GetSafeString reads a null-terminated string as a copy of the original data
+// out.
+func (b *ReadBuffer) GetSafeString() (string, error) {
+	s, err := b.GetUnsafeString()
+	if err != nil {
+		return "", err
+	}
+	return strings.Clone(s), nil
+}
+
 // GetPrepareType returns the buffer's contents as a PrepareType.
 func (b *ReadBuffer) GetPrepareType() (PrepareType, error) {
 	v, err := b.GetBytes(1)

diff --git a/pkg/sql/pgwire/pre_serve_options.go b/pkg/sql/pgwire/pre_serve_options.go
@@ -54,7 +54,9 @@ func parseClientProvidedSessionParameters(
 	hasTenantSelectOption := false
 	for {
 		// Read a key-value pair from the client.
-		key, err := buf.GetString()
+		// Note: GetSafeString is used since the key/value will live well past the
+		// life of the message.
+		key, err := buf.GetSafeString()
 		if err != nil {
 			return args, pgerror.Wrap(
 				err, pgcode.ProtocolViolation,
@@ -65,7 +67,7 @@ func parseClientProvidedSessionParameters(
 			// End of parameter list.
 			break
 		}
-		value, err := buf.GetString()
+		value, err := buf.GetSafeString()
 		if err != nil {
 			return args, pgerror.Wrapf(
 				err, pgcode.ProtocolViolation,
@@ -75,10 +77,6 @@ func parseClientProvidedSessionParameters(
 
 		// Case-fold for the key for easier comparison.
 		key = strings.ToLower(key)
-		// Intentionally clone the string from above, so that the ReaderBuffer life
-		// is limited. Otherwise, the buffer will remain allocated for the life of
-		// the connection.
-		value = strings.Clone(value)
 
 		// Load the parameter.
 		switch key {

diff --git a/pkg/util/metamorphic/BUILD.bazel b/pkg/util/metamorphic/BUILD.bazel
@@ -21,7 +21,14 @@ go_library(
 
 go_test(
     name = "metamorphic_test",
-    srcs = ["constants_test.go"],
+    srcs = [
+        "constants_test.go",
+        "main_test.go",
+    ],
     embed = [":metamorphic"],
-    deps = ["@com_github_stretchr_testify//require"],
+    deps = [
+        "//pkg/util/log",
+        "//pkg/util/randutil",
+        "@com_github_stretchr_testify//require",
+    ],
 )