Skip to content

Commit 3e9bfe4

Browse files
committed
storage: tighten MVCCDeleteRangeUsingTombstone key span checks
We've previously disallowed writing MVCC range tombstones across local keyspace. However, this check used `keys.IsLocal()`, which simply checks whether the key has the local prefix (`\x01`), so it was possible for callers to use e.g. `\x00` as a start key and span the local keyspace anyway. This patch changes the check to ensure the start key is at or after `keys.LocalMax`. Release justification: bug fixes and low-risk updates to new functionality Release note: None
1 parent 544c6ae commit 3e9bfe4

File tree

3 files changed

+25
-3
lines changed

3 files changed

+25
-3
lines changed

pkg/kv/kvserver/mvcc_gc_queue_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -482,6 +482,7 @@ func TestFullRangeDeleteHeuristic(t *testing.T) {
482482
defer eng.Close()
483483

484484
rng, _ := randutil.NewTestRand()
485+
localMax, maxKey := keys.LocalMax, keys.MaxKey
485486

486487
var keys []roachpb.Key
487488
fillDataForStats := func(rw storage.ReadWriter, keyCount, valCount int) (enginepb.MVCCStats, hlc.Timestamp) {
@@ -505,7 +506,7 @@ func TestFullRangeDeleteHeuristic(t *testing.T) {
505506

506507
deleteWithTombstone := func(rw storage.ReadWriter, delTime hlc.Timestamp, ms *enginepb.MVCCStats) {
507508
require.NoError(t, storage.MVCCDeleteRangeUsingTombstone(
508-
ctx, rw, ms, []byte{0}, []byte{0xff}, delTime, hlc.ClockTimestamp{}, nil, nil, false, 1, nil))
509+
ctx, rw, ms, localMax, maxKey, delTime, hlc.ClockTimestamp{}, nil, nil, false, 1, nil))
509510
}
510511
deleteWithPoints := func(rw storage.ReadWriter, delTime hlc.Timestamp, ms *enginepb.MVCCStats) {
511512
for _, key := range keys {

pkg/storage/mvcc.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3187,7 +3187,7 @@ func MVCCDeleteRangeUsingTombstone(
31873187

31883188
// We currently don't allow MVCC range tombstones across the local keyspace,
31893189
// to be safe. This wouldn't handle MVCC stats (SysBytes) correctly either.
3190-
if keys.IsLocal(startKey) || keys.IsLocal(endKey) {
3190+
if startKey.Compare(keys.LocalMax) < 0 {
31913191
return errors.AssertionFailedf("can't write MVCC range tombstone across local keyspan %s",
31923192
rangeKey)
31933193
}

pkg/storage/testdata/mvcc_histories/range_tombstone_writes

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,7 @@ meta: "h"/0,0 -> txn={<nil>} ts=0,0 del=false klen=0 vlen=0 raw=/BYTES/inline me
418418
meta: "i"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=7.000000000,0 min=0,0 seq=0} ts=7.000000000,0 del=false klen=12 vlen=7 mergeTs=<nil> txnDidNotUpdateMeta=true
419419
data: "i"/7.000000000,0 -> /BYTES/i7
420420

421-
# Writing across the local keyspace should error.
421+
# Writing across the local keyspace should error, as should writing from \x00.
422422
run error
423423
del_range_ts k=%a end=%b ts=1
424424
----
@@ -439,3 +439,24 @@ meta: "h"/0,0 -> txn={<nil>} ts=0,0 del=false klen=0 vlen=0 raw=/BYTES/inline me
439439
meta: "i"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=7.000000000,0 min=0,0 seq=0} ts=7.000000000,0 del=false klen=12 vlen=7 mergeTs=<nil> txnDidNotUpdateMeta=true
440440
data: "i"/7.000000000,0 -> /BYTES/i7
441441
error: (*assert.withAssertionFailure:) can't write MVCC range tombstone across local keyspan /Local/Range/"{a"-b"}/1.000000000,0
442+
443+
run error
444+
del_range_ts k=+ end=z ts=1
445+
----
446+
>> at end:
447+
rangekey: {a-b}/[10.000000000,0=/<empty>]
448+
rangekey: {b-d}/[4.000000000,0=/<empty>]
449+
rangekey: {k-p}/[4.000000000,0=/<empty>]
450+
data: "a"/4.000000000,0 -> /<empty>
451+
data: "a"/2.000000000,0 -> /BYTES/a2
452+
data: "b"/3.000000000,0 -> /BYTES/b3
453+
meta: "d"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=7.000000000,0 min=0,0 seq=0} ts=7.000000000,0 del=false klen=12 vlen=7 mergeTs=<nil> txnDidNotUpdateMeta=true
454+
data: "d"/7.000000000,0 -> /BYTES/d7
455+
data: "d"/4.000000000,0 -> /BYTES/d4
456+
data: "f"/4.000000000,0 -> /BYTES/f4
457+
data: "g"/4.000000000,0 -> /<empty>
458+
data: "g"/2.000000000,0 -> /BYTES/g2
459+
meta: "h"/0,0 -> txn={<nil>} ts=0,0 del=false klen=0 vlen=0 raw=/BYTES/inline mergeTs=<nil> txnDidNotUpdateMeta=false
460+
meta: "i"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=7.000000000,0 min=0,0 seq=0} ts=7.000000000,0 del=false klen=12 vlen=7 mergeTs=<nil> txnDidNotUpdateMeta=true
461+
data: "i"/7.000000000,0 -> /BYTES/i7
462+
error: (*assert.withAssertionFailure:) can't write MVCC range tombstone across local keyspan {\x00-z}/1.000000000,0

0 commit comments

Comments
 (0)