[#38, #40] Fix lifetime errors in sub_self_call

This is a complicated fix. Here are the things it does:

- Calculate the `ydb_buffer_t` references for the variable and
  subscripts on every loop iteration, not once per call. This is what
  avoids the unsoundness in #40, since the `buf_addr` will be updated if
  the variable is reallocated.
- Drop `t` before calling `ydb_subscript_next`. This avoids the following borrow-check errors:

  ```
  error[E0502]: cannot borrow `*self` as immutable because it is also borrowed as mutable
      --> src/simple_api/mod.rs:1163:41
       |
  1149 |             let t = self.subscripts.last_mut().unwrap_or(unsafe { self.variable.as_mut_vec() });
       |                     --------------- mutable borrow occurs here
  ...
  1163 |             let (varname, subscripts) = self.get_buffers();
       |                                         ^^^^ immutable borrow occurs here
  ...
  1183 |                 t.reserve(last_self_buffer.len_used as usize - t.len());
       |                 - mutable borrow later used here
  ```

  See code comments for details.

  It's possible in theory that this could be avoided by using raw
  pointers instead of a `&mut`, but I don't feel confident enough in my
  knowledge of unsafe Rust to do that. I would feel more confident if
  [Miri](https://github.com/rust-lang/miri/) worked on YDBRust, but
  unfortunately it [doesn't support external FFI
  calls](rust-lang/miri#11).
- Add a test for the previous undefined behavior.
- Make `get_last_buffer` an unsafe function

  It is used correctly, but knowing that requires non-local reasoning.

  This found a clippy bug: rust-lang/rust-clippy#6675

src/simple_api/mod.rs

@@ -1144,16 +1144,30 @@ impl Key {
     ) -> YDBResult<Vec<u8>> {
         let mut err_buffer_t = Self::make_out_buffer_t(&mut err_buffer);
 
-        // Get pointers to the varname and to the first subscript
-        let (varname, subscripts) = self.get_buffers();
-        let t = self.subscripts.last_mut().unwrap_or(unsafe { self.variable.as_mut_vec() });
-        let mut last_self_buffer;
+        unsafe fn get_last_buffer(this: &mut Key) -> &mut Vec<u8> {
+            // There is no performance difference, and using `or_else` causes a borrow-check error.
+            #[allow(clippy::or_fun_call)]
+            // SAFETY: This is only written to by `ydb_next_subscript` or `ydb_prev_subscript`, which only write variable names, not arbitrary data.
+            // Since variable names are always ASCII, this is sound.
+            this.subscripts.last_mut().unwrap_or(this.variable.as_mut_vec())
+        };
+
         let status = loop {
-            last_self_buffer = ydb_buffer_t {
-                buf_addr: t.as_mut_ptr() as *mut _,
-                len_alloc: t.capacity() as u32,
-                len_used: t.len() as u32,
-            };
+            // NOTE: this can't be hoisted out of the loop because the variable or subscripts could be resized on INVSTRLEN.
+            // WARNING: It's invalid for the unique reference returned by `get_last_buffer` to be
+            // active at the same time that the variable or subscripts are being read from.
+            // `last_self_buffer` only holds raw pointers, so it's ok for it not to be dropped
+            // before calling `self.get_buffers()`.
+            // NOTE: this is a purely compile time issue; the Rust compiler adds `noalias` to all
+            // mut references. It can only go wrong from a miscompilation, not from a
+            // use-after-free.
+            let mut last_self_buffer = Key::make_out_buffer_t(unsafe { get_last_buffer(self) });
+
+            // Get pointers to the varname and to the first subscript
+            // NOTE: ideally this would only update the subscript or variable that changed in the previous loop iteration.
+            // Without benchmarks, I'm not sure how much that would help performance, and this is simpler to work with.
+            // This can't be moved outside the loop because the buffers could be resized on INVSTRLEN.
+            let (varname, subscripts) = self.get_buffers();
 
             let status = unsafe {
                 func(
@@ -1165,33 +1179,40 @@ impl Key {
                     &mut last_self_buffer,
                 )
             };
+
+            // See comments on `last_self_buffer` for why this has to be recalculated.
+            let t = unsafe { get_last_buffer(self) };
+            // If these are different, the `set_len` below is very wrong and will cause UB.
+            assert_eq!(t.as_ptr() as *const _, last_self_buffer.buf_addr);
+
             // HACK: by looking at the source I saw that this only returns INVSTRLEN for variable or subscript,
             // not the error buffer (it will just write a shorter message).
             // So it's safe to only resize `t`.
             if status == YDB_ERR_INVSTRLEN {
-                // New size should be size needed + (current size - len used)
-                let new_size = (last_self_buffer.len_used - last_self_buffer.len_alloc) as usize;
-                let new_size = new_size + (t.capacity() - t.len());
-                t.reserve(new_size);
+                // From the docs for `reserve()`:
+                // > After calling reserve, capacity will be greater than or equal to self.len() + additional
+                t.reserve(last_self_buffer.len_used as usize - t.len());
                 continue;
             }
+            unsafe {
+                t.set_len(min(last_self_buffer.len_alloc, last_self_buffer.len_used) as usize);
+            }
             break status;
         };
-        // Resize the vec with the buffer to we can see the value
-        // We could end up with a buffer of a larger size if we couldn't fit the error string
-        // into the out_buffer, so make sure to pick the smaller size
+
         if status != YDB_OK as i32 {
+            // Resize the vec with the buffer to we can see the value
+            // We could end up with a buffer of a larger size if we couldn't fit the error string
+            // into the out_buffer, so make sure to pick the smaller size
             unsafe {
                 err_buffer.set_len(min(err_buffer_t.len_alloc, err_buffer_t.len_used) as usize);
             }
             // See https://gitlab.com/YottaDB/DB/YDB/-/issues/619
             debug_assert_ne!(status, YDB_ERR_TPRETRY);
-            return Err(YDBError { message: err_buffer, status, tptoken });
-        }
-        unsafe {
-            t.set_len(min(last_self_buffer.len_alloc, last_self_buffer.len_used) as usize);
+            Err(YDBError { message: err_buffer, status, tptoken })
+        } else {
+            Ok(err_buffer)
         }
-        Ok(err_buffer)
     }
 
     fn make_out_buffer_t(out_buffer: &mut Vec<u8>) -> ydb_buffer_t {

src/simple_api/tests.rs

@@ -934,3 +934,14 @@ fn release() {
     assert_eq!(&parts.next().unwrap()[0..1], "r");
     assert_eq!(parts.count(), 2);
 }
+
+#[test]
+fn variable_reallocated() {
+    let mut key = Key::variable(String::from("a"));
+    dbg!(key.variable.capacity());
+    Key::variable("averylongkeywithlotsofletters")
+        .set_st(YDB_NOTTP, Vec::new(), b"some val")
+        .unwrap();
+    key.sub_next_self_st(YDB_NOTTP, Vec::new()).unwrap();
+    dbg!(key.variable.capacity());
+}