From 5ea2da60a9e16c959b5e4990066a527a1d42824c Mon Sep 17 00:00:00 2001
From: Joseph Schorr <josephschorr@users.noreply.github.com>
Date: Tue, 19 Sep 2023 15:11:45 -0400
Subject: [PATCH] Fix a panic in Postgres revision parsing for incompatible
 ZedTokens

If Postgres is passed a ZedToken from CRDB, it will (incorrectly) interpret the token and try to create a slice with too much capacity. This change caps the delta on the transaction IDs and returns an error in that scenario

Fixes #1539
---
 internal/datastore/postgres/revisions.go      | 15 +++++++++++++++
 internal/datastore/postgres/revisions_test.go |  5 +++++
 pkg/zedtoken/zedtoken_test.go                 | 12 ++++++++++++
 3 files changed, 32 insertions(+)

diff --git a/internal/datastore/postgres/revisions.go b/internal/datastore/postgres/revisions.go
index 77c68c1263..82f0443a77 100644
--- a/internal/datastore/postgres/revisions.go
+++ b/internal/datastore/postgres/revisions.go
@@ -169,6 +169,14 @@ func parseRevisionProto(revisionStr string) (datastore.Revision, error) {
 	}, nil
 }
 
+// MaxLegacyXIPDelta is the maximum allowed delta between the xmin and
+// xmax revisions IDs on a *legacy* revision stored as a revision decimal.
+// This is set to prevent a delta that is too large from blowing out the
+// memory usage of the allocated slice, or even causing a panic in the case
+// of a VERY large delta (which can be produced by, for example, a CRDB revision
+// being given to a Postgres datastore accidentally).
+const MaxLegacyXIPDelta = 1000
+
 // parseRevisionDecimal parses a deprecated decimal.Decimal encoding of the revision
 // with an optional xmin component, in the format of revision.xmin, e.g. 100.99.
 // Because we're encoding to a snapshot, we want the revision to be considered visible,
@@ -203,6 +211,13 @@ func parseRevisionDecimal(revisionStr string) (datastore.Revision, error) {
 
 	var xipList []uint64
 	if xmax > xmin {
+		// Ensure that the delta is not too large to cause memory issues or a panic.
+		if xmax-xmin > MaxLegacyXIPDelta {
+			return nil, fmt.Errorf("received revision delta in excess of that expected; are you sure you're not passing a ZedToken from an incompatible datastore?")
+		}
+
+		// TODO(jschorr): Remove this deprecated code path at some point and maybe look into
+		// a more memory-efficient encoding of the XIP list if necessary.
 		xipList = make([]uint64, 0, xmax-xmin)
 		for i := xmin; i < xid; i++ {
 			xipList = append(xipList, i)
diff --git a/internal/datastore/postgres/revisions_test.go b/internal/datastore/postgres/revisions_test.go
index 1680466bf4..474d4c40cd 100644
--- a/internal/datastore/postgres/revisions_test.go
+++ b/internal/datastore/postgres/revisions_test.go
@@ -162,6 +162,11 @@ func TestCombinedRevisionParsing(t *testing.T) {
 	}
 }
 
+func TestBrokenInvalidRevision(t *testing.T) {
+	_, err := parseRevision("1693540940373045727.0000000001")
+	require.Error(t, err)
+}
+
 func FuzzRevision(f *testing.F) {
 	// Attempt to find a decimal revision that is a valid base64 encoded proto revision
 	f.Add(uint64(0), -1)
diff --git a/pkg/zedtoken/zedtoken_test.go b/pkg/zedtoken/zedtoken_test.go
index 01154790ad..7d067b7dbe 100644
--- a/pkg/zedtoken/zedtoken_test.go
+++ b/pkg/zedtoken/zedtoken_test.go
@@ -113,6 +113,18 @@ var decodeTests = []struct {
 		expectedRevision: decimal.New(12345, -2),
 		expectError:      false,
 	},
+	{
+		format: "V1 ZedToken",
+		token:  "GiAKHjE2OTM1NDA5NDAzNzMwNDU3MjcuMDAwMDAwMDAwMQ==",
+		expectedRevision: (func() decimal.Decimal {
+			v, err := decimal.NewFromString("1693540940373045727.0000000001")
+			if err != nil {
+				panic(err)
+			}
+			return v
+		})(),
+		expectError: false,
+	},
 }
 
 func TestDecode(t *testing.T) {