From 049fc0d69309f6a5a66b12b80721a9839e2ce94f Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Mon, 19 Nov 2018 18:23:21 -0500 Subject: [PATCH] libroach: ensure correct lifetime for resume_key on reverse iteration Fixes #32149. Before this change, it was possible for `DBScanResults.resume_key` to point into memory owned by `mvccScanner`, which went out of scope after `MVCCScan` returned. This allowed for memory corruption when returning the key to Go. This change fixes this corruption by copying the memory to the `DBIterator` before returning, which should have a lifetime which exceeds that of the `DBScanResults`. Release note: None --- c-deps/libroach/iterator.h | 1 + c-deps/libroach/mvcc.h | 13 +++++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/c-deps/libroach/iterator.h b/c-deps/libroach/iterator.h index 9beb29ae1ce4..cbd7fa7a60c8 100644 --- a/c-deps/libroach/iterator.h +++ b/c-deps/libroach/iterator.h @@ -31,6 +31,7 @@ struct DBIterator { std::unique_ptr kvs; std::unique_ptr intents; std::unique_ptr stats; + std::string rev_resume_key; rocksdb::ReadOptions read_opts; std::string lower_bound_str; diff --git a/c-deps/libroach/mvcc.h b/c-deps/libroach/mvcc.h index b06208d41f49..37096460c651 100644 --- a/c-deps/libroach/mvcc.h +++ b/c-deps/libroach/mvcc.h @@ -134,7 +134,16 @@ template class mvccScanner { } if (kvs_->Count() == max_keys_ && advanceKey()) { - results_.resume_key = ToDBSlice(cur_key_); + if (reverse) { + // It is possible for cur_key_ to be pointing into mvccScanner.saved_buf_ + // instead of iter_rep_'s underlying storage if iterating in reverse (see + // iterPeekPrev), so copy the key onto the DBIterator struct to ensure it + // has a lifetime that outlives the DBScanResults. + iter_->rev_resume_key.assign(cur_key_.data(), cur_key_.size()); + results_.resume_key = ToDBSlice(iter_->rev_resume_key); + } else { + results_.resume_key = ToDBSlice(cur_key_); + } } return fillResults(); @@ -199,7 +208,7 @@ template class mvccScanner { } if (cur_value_.size() == 0) { - return setStatus(FmtStatus("zero-length mvcc metadata")); + return setStatus(FmtStatus("zero-length mvcc metadata")); } if (!meta_.ParseFromArray(cur_value_.data(), cur_value_.size())) {