db: improve use-after-free test coverage for compactions

[nicktrav]

To increase confidence that use-after-free bugs are not present on
compaction codepaths, use the invalidatingIter wrapper to mutate the
previous key and value during iteration.

Alter the invalidatingIter to avoid zeroing range deletion key / value
pairs, which aligns with the current assumption
(*compactionIter).Next().

Add an in-line comment to point out that storage of a reference to the
previous key / value pairs is safe for range deletion keys during
compaction iteration.

Enable some previously dead code to mutate released byte slices. This
allows for easier detection of use-after-free-type bugs.

[cockroach-teamcity]

This change is 

[nicktrav]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @itsbilal and @sumeerbhola)

---

iterator_test.go, line 221 at r1 (raw file):

	lastKey     *InternalKey
	lastValue   []byte
	ignoreKinds map[base.InternalKeyKind]struct{}

I had an original version of this patch that always skipped invalidation of RangeDeletes. This reduces the coupling a little.

[nicktrav]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @itsbilal and @sumeerbhola)

---

internal/cache/value_invariants.go, line 46 at r1 (raw file):

	// When "invariants" are enabled set the value contents to 0xff in order to
	// cache use-after-free bugs.
	for i := range v.buf {

cc: @petermattis - I wasn't sure if there was any particular reason or history as to why this was commented out. Hopefully it's fine to enable.

[petermattis]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @nicktrav, and @sumeerbhola)

---

internal/cache/value_invariants.go, line 46 at r1 (raw file):

Previously, nicktrav (Nick Travers) wrote…

cc: @petermattis - I wasn't sure if there was any particular reason or history as to why this was commented out. Hopefully it's fine to enable.

I have a vague recollection that this was slow for some reason. I can't recall if the slowness was when running normal tests or running the metamorphic test. And possibly I'm just completely misremembering. Doesn't seem like it should be slow. I'm fine with re-enabling and watching for any fallout.

[sumeerbhola]

code change looks fine

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @itsbilal and @nicktrav)

---

compaction_iter.go, line 270 at r1 (raw file):

			//
			// Note that storing and returning a reference to the key and value
			// here is safe. There is no risk that these stored values that are

I am not sure this is the right comment

• A blockIter provides a value stability guarantee for RANGEDELs since there is only 1 rangedel block in an sstable and this blockIter will not release the bytes for this block until it is closed.
  This statement can be added to the code comment above the blockIter struct that already talks about key stability.

• Translating this per blockIter guarantee into a key and value stability guarantee for the lifetime of the iterator tree in which this blockIter is a leaf is tricky. levelIter opens and closes iterators so there is nothing preventing the blockIter from being closed, which would release the memory. However, compactionIter construction in compaction.newInputIter is wrapping the rangedel blockIter in a noCloseIter to provide this stability guarantee.
  We already have a comment there. But we could probably use a comment at the top of compaction_iter.go saying that because of the construction scheme in compaction.newInputIter we can rely on key and value stability of rangedels for the lifetime of the compactionIter (but see next bullet -- we don't actually have a key stability guarantee)

• The use of saveKey in compactionIter has nothing to do with the key stability guarantee for rangedels, which is for the lifetime of the compactionIter, while this saveKey will overwrite much earlier. In fact saving the key and returning that saved key destroys the key stability guarantee since now we are not returning the original slice backed by the block's memory. I think this is working because the caller is doing cloneKey to restore the stability guarantee that we accidentally destroyed here. It is possible that if we returned i.iterKey here that we could remove that clone. Let us not make this change now, but add a TODO to investigate this.

• I think we use saveKey primarily as a requirement for anyone trying to transitively call nextInStripe (including calls via skipInStripe). This nextInStripe call could be because we are calling it now, or in a later call to Next because we've set skip=true before returning. nextInStripe needs the saved key to compare with the latest iterKey so that it can know when it has stepped to the next user key. I think we should add a code comment about this invariant required by nextInStripe. I think the one exception to this use of saveKey is the one here, and as I said earlier I'm not sure it is necessary -- we may be just blindly following the idiom that we return i.key (note that we say "This is the case on return from all public methods" where i.key is declared). Which is fine -- code simplicity is sometimes better than performance and rangedels are not that common.

---

iterator_test.go, line 221 at r1 (raw file):

Previously, nicktrav (Nick Travers) wrote…

I had an original version of this patch that always skipped invalidation of RangeDeletes. This reduces the coupling a little.

mild preference for doing ignoreKinds [InternalKeyKindMax]bool instead of a map.

[nicktrav]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @itsbilal, @nicktrav, and @sumeerbhola)

---

iterator_test.go, line 221 at r1 (raw file):

Previously, sumeerbhola wrote…

mild preference for doing ignoreKinds [InternalKeyKindMax]bool instead of a map.

👍

---

internal/cache/value_invariants.go, line 46 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I have a vague recollection that this was slow for some reason. I can't recall if the slowness was when running normal tests or running the metamorphic test. And possibly I'm just completely misremembering. Doesn't seem like it should be slow. I'm fine with re-enabling and watching for any fallout.

Cool, thanks. Will keep for now, and monitor.

[nicktrav]

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @itsbilal and @sumeerbhola)

---

compaction_iter.go, line 270 at r1 (raw file):

Previously, sumeerbhola wrote…

I am not sure this is the right comment

• A blockIter provides a value stability guarantee for RANGEDELs since there is only 1 rangedel block in an sstable and this blockIter will not release the bytes for this block until it is closed.
  This statement can be added to the code comment above the blockIter struct that already talks about key stability.

• Translating this per blockIter guarantee into a key and value stability guarantee for the lifetime of the iterator tree in which this blockIter is a leaf is tricky. levelIter opens and closes iterators so there is nothing preventing the blockIter from being closed, which would release the memory. However, compactionIter construction in compaction.newInputIter is wrapping the rangedel blockIter in a noCloseIter to provide this stability guarantee.
  We already have a comment there. But we could probably use a comment at the top of compaction_iter.go saying that because of the construction scheme in compaction.newInputIter we can rely on key and value stability of rangedels for the lifetime of the compactionIter (but see next bullet -- we don't actually have a key stability guarantee)

• The use of saveKey in compactionIter has nothing to do with the key stability guarantee for rangedels, which is for the lifetime of the compactionIter, while this saveKey will overwrite much earlier. In fact saving the key and returning that saved key destroys the key stability guarantee since now we are not returning the original slice backed by the block's memory. I think this is working because the caller is doing cloneKey to restore the stability guarantee that we accidentally destroyed here. It is possible that if we returned i.iterKey here that we could remove that clone. Let us not make this change now, but add a TODO to investigate this.

• I think we use saveKey primarily as a requirement for anyone trying to transitively call nextInStripe (including calls via skipInStripe). This nextInStripe call could be because we are calling it now, or in a later call to Next because we've set skip=true before returning. nextInStripe needs the saved key to compare with the latest iterKey so that it can know when it has stepped to the next user key. I think we should add a code comment about this invariant required by nextInStripe. I think the one exception to this use of saveKey is the one here, and as I said earlier I'm not sure it is necessary -- we may be just blindly following the idiom that we return i.key (note that we say "This is the case on return from all public methods" where i.key is declared). Which is fine -- code simplicity is sometimes better than performance and rangedels are not that common.

Thanks for the clarification. I've added some extra documentation elsewhere, and removed the original comment. This should be good for another look.

[sumeerbhola]

Reviewed 1 of 1 files at r2, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @itsbilal, @nicktrav, and @sumeerbhola)

---

compaction_iter.go, line 451 at r4 (raw file):

// indicating how its state changed.
//
// Calls to nextInStripe are typically preceded by a call to saveKey to retain a

nit: I think this could be more strongly stated as "Calls to nextInStripe must be preceded by a call ...".
We need that for the i.cmp(i.key.UserKey, key.UserKey) calls in this function.

---

iterator_test.go, line 227 at r4 (raw file):

	return &invalidatingIter{
		iter:        iter,
		ignoreKinds: make([]bool, 1<<8 /* 8 bits for key kinds */),

nit: any reason to use a slice instead of the array sized using InternalKeyKindMax? Also, it would be nice if we didn't have 1<<8 here and could use an existing enum for sizing.

[nicktrav]

Dismissed @sumeerbhola from a discussion.
Reviewable status: 3 of 5 files reviewed, all discussions resolved (waiting on @itsbilal and @sumeerbhola)

---

compaction_iter.go, line 451 at r4 (raw file):

Previously, sumeerbhola wrote…

nit: I think this could be more strongly stated as "Calls to nextInStripe must be preceded by a call ...".
We need that for the i.cmp(i.key.UserKey, key.UserKey) calls in this function.

Done.

---

iterator_test.go, line 227 at r4 (raw file):

Previously, sumeerbhola wrote…

nit: any reason to use a slice instead of the array sized using InternalKeyKindMax? Also, it would be nice if we didn't have 1<<8 here and could use an existing enum for sizing.

Ah, I see what you meant now. That field isn't part of the file format and can change. Updated to use that.

[nicktrav]

TFTR!