Merge #32492

32492: libroach: ensure correct lifetime for resume_key on reverse iteration r=nvanbenschoten a=nvanbenschoten 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 Co-authored-by: Nathan VanBenschoten <[email protected]>
cockroachdb · Nov 20, 2018 · 704e5b8 · 704e5b8
2 parents 9417c7c + 049fc0d
commit 704e5b8
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 2 deletions.
diff --git a/c-deps/libroach/iterator.h b/c-deps/libroach/iterator.h
@@ -31,6 +31,7 @@ struct DBIterator {
   std::unique_ptr<cockroach::chunkedBuffer> kvs;
   std::unique_ptr<rocksdb::WriteBatch> intents;
   std::unique_ptr<IteratorStats> 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
@@ -134,7 +134,16 @@ template <bool reverse> 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 <bool reverse> 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())) {