From 06ba0738c7e3b8f1daab5afeff805ce737ad8e8c Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz <mina86@mina86.com>
Date: Mon, 25 Jul 2022 18:56:27 +0200
Subject: [PATCH] =?UTF-8?q?Revert=20"Don=E2=80=99t=20hold=20onto=20ReadOpt?=
 =?UTF-8?q?ions.inner=20when=20iterating"=20(#661)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 src/db_iterator.rs | 19 +++++++++++--------
 src/db_options.rs  |  4 ++--
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/src/db_iterator.rs b/src/db_iterator.rs
index d671470..3fa22d1 100644
--- a/src/db_iterator.rs
+++ b/src/db_iterator.rs
@@ -72,10 +72,15 @@ pub type DBRawIterator<'a> = DBRawIteratorWithThreadMode<'a, DB>;
 pub struct DBRawIteratorWithThreadMode<'a, D: DBAccess> {
     inner: std::ptr::NonNull<ffi::rocksdb_iterator_t>,
 
-    /// When iterate_upper_bound or iterate_lower_bound is set, the inner
-    /// C iterator keeps a pointer to the bounds.  We need to hold onto those
-    /// slices or else C iterator ends up reading freed memory.
-    _bounds: (Option<Vec<u8>>, Option<Vec<u8>>),
+    /// When iterate_lower_bound or iterate_upper_bound are set, the inner
+    /// C iterator keeps a pointer to the upper bound inside `_readopts`.
+    /// Storing this makes sure the upper bound is always alive when the
+    /// iterator is being used.
+    ///
+    /// And yes, we need to store the entire ReadOptions structure since C++
+    /// ReadOptions keep reference to C rocksdb_readoptions_t wrapper which
+    /// point to vectors we own.  See issue #660.
+    _readopts: ReadOptions,
 
     db: PhantomData<&'a D>,
 }
@@ -96,17 +101,15 @@ impl<'a, D: DBAccess> DBRawIteratorWithThreadMode<'a, D> {
         Self::from_inner(inner, readopts)
     }
 
-    fn from_inner(inner: *mut ffi::rocksdb_iterator_t, mut readopts: ReadOptions) -> Self {
+    fn from_inner(inner: *mut ffi::rocksdb_iterator_t, readopts: ReadOptions) -> Self {
         // This unwrap will never fail since rocksdb_create_iterator and
         // rocksdb_create_iterator_cf functions always return non-null.  They
         // use new and deference the result so any nulls would end up in SIGSEGV
         // there and we have bigger issue.
         let inner = std::ptr::NonNull::new(inner).unwrap();
-        let lower = readopts.iterate_lower_bound.take();
-        let upper = readopts.iterate_upper_bound.take();
         Self {
             inner,
-            _bounds: (lower, upper),
+            _readopts: readopts,
             db: PhantomData,
         }
     }
diff --git a/src/db_options.rs b/src/db_options.rs
index b58a0b4..d1e5152 100644
--- a/src/db_options.rs
+++ b/src/db_options.rs
@@ -336,8 +336,8 @@ pub struct BlockBasedOptions {
 
 pub struct ReadOptions {
     pub(crate) inner: *mut ffi::rocksdb_readoptions_t,
-    pub(crate) iterate_upper_bound: Option<Vec<u8>>,
-    pub(crate) iterate_lower_bound: Option<Vec<u8>>,
+    iterate_upper_bound: Option<Vec<u8>>,
+    iterate_lower_bound: Option<Vec<u8>>,
 }
 
 /// Configuration of cuckoo-based storage.