Skip to content

Commit

Permalink
Merge pull request #1540 from josephschorr/zedtoken-panic-fix
Browse files Browse the repository at this point in the history
Fix a panic in Postgres revision parsing for incompatible ZedTokens
  • Loading branch information
josephschorr authored Sep 19, 2023
2 parents ebda6d1 + 5ea2da6 commit 9214148
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 0 deletions.
15 changes: 15 additions & 0 deletions internal/datastore/postgres/revisions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions internal/datastore/postgres/revisions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
12 changes: 12 additions & 0 deletions pkg/zedtoken/zedtoken_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 9214148

Please sign in to comment.