From 6628daf554c90721f2ea37478d828c83531f17ac Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Tue, 22 Dec 2020 19:36:06 -0500
Subject: [PATCH] sql: catch panics from SHOW STATISTICS code

This change adds a panic catcher around the code that processes the
statistics for SHOW STATISTICS. This statement is used internally for
statement diagnostics, so a crash here can be very bad.

Informs #58220.
Informs #56356.

Release note (bug fix): added a safeguard against crashes while
running `SHOW STATISTICS USING JSON`, which is used internally for
statement diagnostics and EXPLAIN ANALYZE (DEBUG).
---
 pkg/sql/show_stats.go | 20 +++++++++++++++++++-
 pkg/sql/stats/json.go |  6 ++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/pkg/sql/show_stats.go b/pkg/sql/show_stats.go
index 18159a0944cf..80e35c90572d 100644
--- a/pkg/sql/show_stats.go
+++ b/pkg/sql/show_stats.go
@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/stats"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
+	"github.com/cockroachdb/cockroach/pkg/util/errorutil"
 	"github.com/cockroachdb/cockroach/pkg/util/json"
 	"github.com/cockroachdb/errors"
 )
@@ -58,7 +59,7 @@ func (p *planner) ShowTableStats(ctx context.Context, n *tree.ShowTableStats) (p
 	return &delayedNode{
 		name:    n.String(),
 		columns: columns,
-		constructor: func(ctx context.Context, p *planner) (planNode, error) {
+		constructor: func(ctx context.Context, p *planner) (_ planNode, err error) {
 			// We need to query the table_statistics and then do some post-processing:
 			//  - convert column IDs to column names
 			//  - if the statistic has a histogram, we return the statistic ID as a
@@ -96,6 +97,23 @@ func (p *planner) ShowTableStats(ctx context.Context, n *tree.ShowTableStats) (p
 				numCols
 			)
 
+			// Guard against crashes in the code below (e.g. #56356).
+			defer func() {
+				if r := recover(); r != nil {
+					// This code allows us to propagate internal errors without having to add
+					// error checks everywhere throughout the code. This is only possible
+					// because the code does not update shared state and does not manipulate
+					// locks.
+					if ok, e := errorutil.ShouldCatch(r); ok {
+						err = e
+					} else {
+						// Other panic objects can't be considered "safe" and thus are
+						// propagated as crashes that terminate the session.
+						panic(r)
+					}
+				}
+			}()
+
 			v := p.newContainerValuesNode(columns, 0)
 			if n.UsingJSON {
 				result := make([]stats.JSONStatistic, len(rows))
diff --git a/pkg/sql/stats/json.go b/pkg/sql/stats/json.go
index 12b7e30a956e..b648e2116fed 100644
--- a/pkg/sql/stats/json.go
+++ b/pkg/sql/stats/json.go
@@ -54,6 +54,9 @@ type JSONHistoBucket struct {
 // SetHistogram fills in the HistogramColumnType and HistogramBuckets fields.
 func (js *JSONStatistic) SetHistogram(h *HistogramData) error {
 	typ := h.ColumnType
+	if typ == nil {
+		return fmt.Errorf("histogram type is unset")
+	}
 	js.HistogramColumnType = typ.SQLString()
 	js.HistogramBuckets = make([]JSONHistoBucket, len(h.Buckets))
 	var a rowenc.DatumAlloc
@@ -63,6 +66,9 @@ func (js *JSONStatistic) SetHistogram(h *HistogramData) error {
 		js.HistogramBuckets[i].NumRange = b.NumRange
 		js.HistogramBuckets[i].DistinctRange = b.DistinctRange
 
+		if b.UpperBound == nil {
+			return fmt.Errorf("histogram bucket upper bound is unset")
+		}
 		datum, _, err := rowenc.DecodeTableKey(&a, typ, b.UpperBound, encoding.Ascending)
 		if err != nil {
 			return err