Skip to content

Commit

Permalink
storage: prevent iter panic on meta1 split key
Browse files Browse the repository at this point in the history
It was possible that a load based split was suggested for `meta1`, which
would call `MVCCFirstSplitKey` and panic as the `meta1` start key
`\Min` is local, whilst the `meta1` end key is global `0x02 0xff 0xff`.

Add a check that the start key is greater than the `meta1` end key before
processing in `MVCCFirstSplitKey` to prevent the panic.

Note `meta1` would never be split regardless, as
`storage.IsValidSplitKey` would fail after finding a split key.

Also note that if the desired split key is a local key, the same problem
doesn't exist as the minimum split key would be used to seek the first
split key instead.

Fixes: cockroachdb#104007

Release note: None
  • Loading branch information
kvoli committed Jun 7, 2023
1 parent 1cb5e6c commit 7864aae
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
8 changes: 7 additions & 1 deletion pkg/storage/mvcc.go
Original file line number Diff line number Diff line change
Expand Up @@ -5496,10 +5496,16 @@ func mvccMinSplitKey(it MVCCIterator, startKey roachpb.Key) (roachpb.Key, error)
// 4. Not in between the start and end of a row for table ranges.
//
// The returned split key is NOT guaranteed to be outside a no-split span, such
// as Meta1, Meta2Max or Node Liveness.
// as Meta2Max or Node Liveness.
func MVCCFirstSplitKey(
_ context.Context, reader Reader, desiredSplitKey, startKey, endKey roachpb.RKey,
) (roachpb.Key, error) {
// If the start key of the range is within the meta1 key space, the range
// cannot be split.
if startKey.Less(roachpb.RKey(keys.LocalMax)) {
return nil, nil
}

it := reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: endKey.AsRawKey()})
defer it.Close()

Expand Down
20 changes: 20 additions & 0 deletions pkg/storage/mvcc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4530,6 +4530,25 @@ func TestMVCCFirstSplitKey(t *testing.T) {
{desired: roachpb.Key("z"), expected: nil},
},
},
{
// meta1 cannot be split. Previously, this test would cause a panic in
// mvccMinSplitKey, called by MVCCFirstSplitKey. The iterator is
// initialized with a global key constraint from the endKey
// ("\x02\xff\xff"), but we seekGE the start key (MinKey="") which is
// local because it is before LocalMax (0x02).
keys: []roachpb.Key{
roachpb.Key("\x02"),
roachpb.Key("\x02\x00"),
roachpb.Key("\x02\xff"),
},
startKey: keys.MinKey,
endKey: keys.Meta1KeyMax,
splits: []splitExpect{
{desired: keys.MinKey, expected: nil},
{desired: roachpb.Key("\x02"), expected: nil},
{desired: roachpb.Key("\x02\x00"), expected: nil},
},
},
{
// All keys are outside the range, no keys to spit at so expect no
// splits.
Expand Down Expand Up @@ -4581,6 +4600,7 @@ func TestMVCCFirstSplitKey(t *testing.T) {
{desired: roachpb.Key("0"), expected: roachpb.Key("c")},
{desired: roachpb.Key("b"), expected: roachpb.Key("c")},
{desired: roachpb.Key("c"), expected: roachpb.Key("c")},
{desired: keys.MinKey, expected: roachpb.Key("c")},
// Desired split key is after the last key in the range (c), shouldn't
// split.
{desired: roachpb.Key("d"), expected: nil},
Expand Down

0 comments on commit 7864aae

Please sign in to comment.