Merge pull request cockroachdb#32742 from knz/backport2.1-32713

release-2.1: sql: prevent a crash and add telemetry for an assertion
tbg · Nov 30, 2018 · ef0423b · ef0423b
2 parents 78a06c1 + 3460b0a
commit ef0423b
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 2 deletions.
diff --git a/pkg/sql/cluster_wide_id.go b/pkg/sql/cluster_wide_id.go
@@ -47,6 +47,7 @@ func StringToClusterWideID(s string) (ClusterWideID, error) {
 }
 
 // BytesToClusterWideID converts raw bytes into a ClusterWideID.
+// The caller is responsible for ensuring the byte slice contains 16 bytes.
 func BytesToClusterWideID(b []byte) ClusterWideID {
 	id := uint128.FromBytes(b)
 	return ClusterWideID{Uint128: id}

diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go
@@ -36,7 +36,9 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/server/status"
+	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/settings"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/builtins"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/types"
@@ -886,11 +888,27 @@ func populateSessionsTable(
 			kvTxnIDDatum = tree.NewDString(session.KvTxnID.String())
 		}
 
-		sessionID := BytesToClusterWideID(session.ID)
+		// TODO(knz): serverpb.Session is always constructed with an ID
+		// set from a 16-byte session ID. Yet we get crash reports
+		// that fail in BytesToClusterWideID() with a byte slice that's
+		// too short. See #32517.
+		var sessionID tree.Datum
+		if session.ID == nil {
+			telemetry.RecordError(
+				pgerror.NewInternalTrackingError(32517 /* issue */, "null"))
+			sessionID = tree.DNull
+		} else if len(session.ID) != 16 {
+			telemetry.RecordError(
+				pgerror.NewInternalTrackingError(32517 /* issue */, fmt.Sprintf("len=%d", len(session.ID))))
+			sessionID = tree.NewDString("<invalid>")
+		} else {
+			clusterSessionID := BytesToClusterWideID(session.ID)
+			sessionID = tree.NewDString(clusterSessionID.String())
+		}
 
 		if err := addRow(
 			tree.NewDInt(tree.DInt(session.NodeID)),
-			tree.NewDString(sessionID.String()),
+			sessionID,
 			tree.NewDString(session.Username),
 			tree.NewDString(session.ClientAddress),
 			tree.NewDString(session.ApplicationName),

diff --git a/pkg/sql/pgwire/pgerror/errors.go b/pkg/sql/pgwire/pgerror/errors.go
@@ -158,6 +158,15 @@ func NewAssertionErrorf(format string, args ...interface{}) error {
 	return err
 }
 
+// NewInternalTrackingError instantiates an error
+// meant for use with telemetry.ReportError directly.
+func NewInternalTrackingError(issue int, detail string) error {
+	prefix := fmt.Sprintf("#%d.%s", issue, detail)
+	err := NewErrorWithDepthf(1, CodeInternalError, "internal error: %s", prefix)
+	err.InternalCommand = prefix + " " + captureTrace()
+	return err.SetHintf("See: https://github.com/cockroachdb/cockroach/issues/%d", issue)
+}
+
 func captureTrace() string {
 	var pc [50]uintptr
 	n := runtime.Callers(3, pc[:])

diff --git a/pkg/util/uint128/uint128.go b/pkg/util/uint128/uint128.go
@@ -94,6 +94,7 @@ func (u Uint128) Xor(o Uint128) Uint128 {
 }
 
 // FromBytes parses the byte slice as a 128 bit big-endian unsigned integer.
+// The caller is responsible for ensuring the byte slice contains 16 bytes.
 func FromBytes(b []byte) Uint128 {
 	hi := binary.BigEndian.Uint64(b[:8])
 	lo := binary.BigEndian.Uint64(b[8:])