From 276059b3614d9bc5e7a5830dc8be01cbd651d38b Mon Sep 17 00:00:00 2001
From: Davis Vaughan <davis@posit.co>
Date: Sun, 28 Jul 2024 10:32:20 -0400
Subject: [PATCH] Fix memory leak with move assignment operator

---
 NEWS.md                         |  3 +++
 cpp11test/src/test-integers.cpp | 18 ++++++++++++++++++
 cpp11test/src/test-list.cpp     | 18 ++++++++++++++++++
 inst/include/cpp11/protect.hpp  |  5 +++++
 inst/include/cpp11/r_vector.hpp |  2 +-
 5 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/NEWS.md b/NEWS.md
index 7de96b3e..03bb7251 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,5 +1,8 @@
 # cpp11 (development version)
 
+* Fixed a memory leak with the `cpp11::writable::r_vector` move assignment
+  operator (#338).
+
 * Dropped support for gcc 4.8, mainly an issue for extremely old CentOS 7
   systems which used that as their default compiler. As of June 2024, CentOS 7
   is past its Vendor end of support date and therefore also out of scope for
diff --git a/cpp11test/src/test-integers.cpp b/cpp11test/src/test-integers.cpp
index 6f876c57..4629442f 100644
--- a/cpp11test/src/test-integers.cpp
+++ b/cpp11test/src/test-integers.cpp
@@ -2,6 +2,7 @@
 #include "cpp11/doubles.hpp"
 #include "cpp11/function.hpp"
 #include "cpp11/integers.hpp"
+#include "cpp11/protect.hpp"
 #include "cpp11/strings.hpp"
 
 #include <testthat.h>
@@ -207,4 +208,21 @@ context("integers-C++") {
     int y = NA_INTEGER;
     expect_true(cpp11::is_na(y));
   }
+
+  test_that("writable integer vector temporary isn't leaked (#338)") {
+    R_xlen_t before = cpp11::preserved.count();
+
+    // +1 from `x` allocation
+    cpp11::writable::integers x(1);
+
+    // Calls move assignment operator `operator=(r_vector<T>&& rhs)`
+    // +1 from `rhs` allocation and move into `x`
+    // -1 from old `x` release
+    x = cpp11::writable::integers(1);
+
+    R_xlen_t after = cpp11::preserved.count();
+
+    expect_true(before == 0);
+    expect_true(after - before == 1);
+  }
 }
diff --git a/cpp11test/src/test-list.cpp b/cpp11test/src/test-list.cpp
index 0abf6002..e3f30d41 100644
--- a/cpp11test/src/test-list.cpp
+++ b/cpp11test/src/test-list.cpp
@@ -2,6 +2,7 @@
 #include "cpp11/integers.hpp"
 #include "cpp11/list.hpp"
 #include "cpp11/logicals.hpp"
+#include "cpp11/protect.hpp"
 #include "cpp11/raws.hpp"
 #include "cpp11/strings.hpp"
 
@@ -162,4 +163,21 @@ context("list-C++") {
     expect_true(Rf_xlength(y) == 0);
     expect_true(y != R_NilValue);
   }
+
+  test_that("writable list vector temporary isn't leaked (#338)") {
+    R_xlen_t before = cpp11::preserved.count();
+
+    // +1 from `x` allocation
+    cpp11::writable::list x(1);
+
+    // Calls move assignment operator `operator=(r_vector<T>&& rhs)`
+    // +1 from `rhs` allocation and move into `x`
+    // -1 from old `x` release
+    x = cpp11::writable::list(1);
+
+    R_xlen_t after = cpp11::preserved.count();
+
+    expect_true(before == 0);
+    expect_true(after - before == 1);
+  }
 }
diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp
index e4473e6f..b49c647c 100644
--- a/inst/include/cpp11/protect.hpp
+++ b/inst/include/cpp11/protect.hpp
@@ -286,6 +286,11 @@ static struct {
     REprintf("---\n");
   }
 
+  R_xlen_t count() {
+    // Elements in list = `Rf_xlength()` - `head` - `tail`
+    return Rf_xlength(get_preserve_list()) - 2;
+  }
+
   void release(SEXP cell) {
     if (cell == R_NilValue) {
       return;
diff --git a/inst/include/cpp11/r_vector.hpp b/inst/include/cpp11/r_vector.hpp
index 4831c2f5..66aae55c 100644
--- a/inst/include/cpp11/r_vector.hpp
+++ b/inst/include/cpp11/r_vector.hpp
@@ -841,7 +841,7 @@ inline r_vector<T>& r_vector<T>::operator=(r_vector<T>&& rhs) {
   SEXP old_protect = protect_;
 
   data_ = rhs.data_;
-  protect_ = preserved.insert(data_);
+  protect_ = rhs.protect_;
 
   preserved.release(old_protect);