storage/pebbleiter: mangle unsafe buffers during positioning #96685
Conversation
|
This change is |
jbowens
force-pushed
the
assertion-iter-mangle
branch
from
72c0bff to
dccfbb0
Compare
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @bananabrick, @erikgrinaker, and @itsbilal)
pkg/storage/pebbleiter/crdb_test_on.go line 55 at r3 (raw file):
key [2][]byte val [2][]byte }
In a subsequent PR I plan to add similar mangling of RangeBounds() and RangeKeys(), but restricted to when RangeKeyChanged() to obey that more permissive contract.
jbowens
changed the title
|
Checked and this does not resolve #94141. |
There was a problem hiding this comment.
I have a slight preference for this being squashed into one commit. The first commit is introducing tests which would start failing if the second or third commit is reverted, right?
Reviewed 2 of 2 files at r1.
Reviewable status:
pkg/kv/kvserver/spanset/batch.go line 122 at r3 (raw file):
// checkAllowedCurrPosForward checks the span starting at the current iterator // position, if the current iterator position is valid. func (i *MVCCIterator) checkAllowedCurrPosForward(errIfDisallowed bool) {
We could check if i.i.Valid() and then call checkAllowed with the appropriate span from here. I guess that causes a duplicate Valid check.
pkg/kv/kvserver/spanset/batch.go line 136 at r3 (raw file):
// checkAllowedCurrPosForward checks the provided span if the current iterator // position is valid. func (i *MVCCIterator) checkAllowed(span roachpb.Span, errIfDisallowed bool) {
"// checkAllowed checks" instead of "// checkAllowedCurrPosForward checks"...
pkg/storage/sst.go line 1080 at r3 (raw file):
// 3) both iterators changed keys and the sst iterator's key is further // ahead. if sstOK && extChangedKeys && (!sstChangedKeys || !extOK || extIter.UnsafeKey().Key.Compare(sstIter.UnsafeKey().Key) < 0) {
This seems fine, but I'm not sure what the ramifications of not seeking here are. cc @itsbilal
pkg/storage/pebbleiter/crdb_test_on.go line 31 at r1 (raw file):
// MaybeWrap returns the provided Pebble iterator, wrapped with double close // detection. func MaybeWrap(iter *pebble.Iterator) Iterator {
unrelated, but reading through the rest of this code since I've never seen this code before. Why is this called MaybeWrap, when it always seems to wrap the iterator?
pkg/storage/pebbleiter/crdb_test_on.go line 94 at r1 (raw file):
func (i *assertionIter) LazyValue() pebble.LazyValue { if !i.Valid() { panic(errors.AssertionFailedf("Value() called on !Valid() pebble.Iterator"))
LazyValue called on...
pkg/storage/pebbleiter/crdb_test_on.go line 162 at r1 (raw file):
// the caller. This is used to ensure that the client respects the Pebble // iterator interface and the lifetimes of buffers it returns. func (i *assertionIter) mangleBufs() {
nit: maybeMangleBufs? And what's the benefit to doing this randomly as opposed to always?
Randomly clobber the key and value buffers in order to ensure lifetimes are respected. This commit extends the `pebbleiter.assertionIter` type used in `crdb_test` builds that sits between pkg/storage and the pebble.Iterator type. It now will randomly zero the buffers used to return the previous iterator position's key and value, to ensure code isn't improperly relying on the stability of the these keys. Epic: None Close cockroachdb#86657. Release note: None
Previously, CheckSSTConflicts would improperly call UnsafeKey on an exhausted sstIter. This could result in undefined, inconsistent behavior as UnsafeKey may point to arbitrary data once the iterator is exhausted. Informs cockroachdb#94141. Epic: None Release note: None
Previously, the spanset.MVCCIterator implementation would retrieve the contained iterator's UsafeKey() on an invalid iterator in Next, NextKey and Prev. This retrieval was harmless because it always checked the validity of the iterator before using it, but recent assertions in test builds prevent this usage. Release note: None
jbowens
force-pushed
the
assertion-iter-mangle
branch
from
070d2c3 to
a80f29c
Compare
There was a problem hiding this comment.
TFTR!
I have a slight preference for this being squashed into one commit. The first commit is introducing tests which would start failing if the second or third commit is reverted, right?
Yes, but the bug wouldn't stop existing if the second or third commit were reverted.
Reviewable status:
pkg/kv/kvserver/spanset/batch.go line 122 at r3 (raw file):
Previously, bananabrick (Arjun Nair) wrote…
We could check if
i.i.Valid()and then callcheckAllowedwith the appropriate span from here. I guess that causes a duplicateValidcheck.
yep, this structure is intended to avoid the duplicate Valid calls
pkg/kv/kvserver/spanset/batch.go line 136 at r3 (raw file):
Previously, bananabrick (Arjun Nair) wrote…
"// checkAllowed checks" instead of "// checkAllowedCurrPosForward checks"...
Done.
pkg/storage/pebbleiter/crdb_test_on.go line 31 at r1 (raw file):
Previously, bananabrick (Arjun Nair) wrote…
unrelated, but reading through the rest of this code since I've never seen this code before. Why is this called
MaybeWrap, when it always seems to wrap the iterator?
There are two implementations of MaybeWrap, controlled by build tags. The one used when the crdb_test build tag is not present does not wrap the iterator.
pkg/storage/pebbleiter/crdb_test_on.go line 94 at r1 (raw file):
Previously, bananabrick (Arjun Nair) wrote…
LazyValue called on...
Done.
pkg/storage/pebbleiter/crdb_test_on.go line 162 at r1 (raw file):
nit: maybeMangleBufs?
Sure
And what's the benefit to doing this randomly as opposed to always?
It ensures no usages are dependent on either behavior. No clients should expect the buffers to remain the same, but they also should not expect the buffers to be zeroed.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status:
pkg/storage/sst.go line 1080 at r3 (raw file):
Previously, bananabrick (Arjun Nair) wrote…
This seems fine, but I'm not sure what the ramifications of not seeking here are. cc @itsbilal
LGTM
|
TFTRs! bors r+ |
|
Build succeeded: |
This is a backport of the CheckSSTConflicts bug fix contained within cockroachdb#96685. Previously, CheckSSTConflicts would improperly access an invalid iterator's `UnsafeKey`, using it to seek another iterator. Epic: None Release note: None
Backport cockroachdb#90859, cockroachdb#96685 and cockroachdb#97222 to mangle the buffers returned by Pebble iterators to ensure MVCC and Cockroach code respects the Pebble iterator memory lifetimes. The RangeBounds() and RangeKeys() validity assertions are omitted, since 22.2 allowed these methods to be invoked on an invalid iterator or an iterator positioned at a point key. Release note: None Release justification: Non-production code changes
Backport cockroachdb#90859, cockroachdb#96685 and cockroachdb#97222 to mangle the buffers returned by Pebble iterators to ensure MVCC and Cockroach code respects the Pebble iterator memory lifetimes. The RangeBounds() and RangeKeys() validity assertions are omitted, since 22.2 allowed these methods to be invoked on an invalid iterator or an iterator positioned at a point key. Release note: None Release justification: Non-production code changes
storage/pebbleiter: mangle unsafe buffers between positioning methods
Randomly clobber the key and value buffers in order to ensure lifetimes are respected. This commit extends the
pebbleiter.assertionItertype used incrdb_testbuilds that sits between pkg/storage and the pebble.Iterator type. It now will randomly zero the buffers used to return the previous iterator position's key and value, to ensure code isn't improperly relying on the stability of the these keys.storage: fix UnsafeKey() usage of invalid iterator
Previously, CheckSSTConflicts would improperly call UnsafeKey on an exhausted
sstIter. This could result in undefined, inconsistent behavior as UnsafeKey
may point to arbitrary data once the iterator is exhausted.
kv/kvserver/spanset: fix read of invalid iterator's UnsafeKey
Previously, the spanset.MVCCIterator implementation would retrieve the
contained iterator's UsafeKey() on an invalid iterator in Next, NextKey and
Prev. This retrieval was harmless because it always checked the validity of the
iterator before using it, but recent assertions in test builds prevent this
usage.
Epic: None
Close #86657.
Informs #94141.
Release note: None