Introduce unique_cstr.

This commit adds a RAII wrapper for C-strings, called `unique_cstr`.
Several places in the NRN Python bindings use `char *` when owning a
malloc allocated string.

This new class is then used to prevent leaking HOC strings on error
paths.

src/neuron/unique_cstr.hpp

@@ -0,0 +1,54 @@
+#pragma once
+
+#include <cstdlib>
+#include <utility>
+
+namespace neuron {
+
+/** A RAII wrapper for C-style strings.
+ *
+ *  The string must be null-terminated and allocated with `malloc`. The lifetime of the string is
+ * bound to the life time of the `unique_cstr`. Certain patterns in NRN require passing on
+ * ownership, this is achieved using `.release()`, which returns the contained C-string and makes
+ * this object invalid.
+ */
+class unique_cstr {
+  public:
+    unique_cstr(const unique_cstr&) = delete;
+    unique_cstr(unique_cstr&& other) {
+        *this = std::move(other);
+    }
+
+    const unique_cstr& operator=(const unique_cstr&) = delete;
+    const unique_cstr& operator=(unique_cstr&& other) {
+        this->str_ = std::exchange(other.str_, nullptr);
+        return *this;
+    }
+
+    explicit unique_cstr(char* cstr)
+        : str_(cstr) {}
+
+    ~unique_cstr() {
+        std::free((void*) str_);
+    }
+
+    /** Releases ownership of the string.
+     *
+     *  Returns the string and makes this object invalid.
+     */
+    [[nodiscard]] char* release() {
+        return std::exchange(str_, nullptr);
+    }
+
+    char* c_str() const {
+        return str_;
+    }
+    bool is_valid() const {
+        return str_ != nullptr;
+    }
+
+  private:
+    char* str_ = nullptr;
+};
+
+}  // namespace neuron

src/nrnpython/nrnpy_hoc.cpp

@@ -1,6 +1,7 @@
 #include "cabcode.h"
 #include "ivocvect.h"
 #include "neuron/container/data_handle.hpp"
+#include "neuron/unique_cstr.hpp"
 #include "nrniv_mf.h"
 #include "nrn_pyhocobject.h"
 #include "nrnoc2iv.h"
@@ -408,7 +409,7 @@ static Inst* save_pc(Inst* newpc) {
 }
 
 // also called from nrnpy_nrn.cpp
-int hocobj_pushargs(PyObject* args, std::vector<char*>& s2free) {
+int hocobj_pushargs(PyObject* args, std::vector<neuron::unique_cstr>& s2free) {
     int i, narg = PyTuple_Size(args);
     for (i = 0; i < narg; ++i) {
         PyObject* po = PyTuple_GetItem(args, i);
@@ -426,13 +427,14 @@ int hocobj_pushargs(PyObject* args, std::vector<char*>& s2free) {
                 // Exception ignored on calling ctypes callback function.
                 // So get the message, clear, and make the message
                 // part of the execerror.
-                *ts = str.get_pyerr();
-                s2free.push_back(*ts);
+                auto err = neuron::unique_cstr(str.get_pyerr());
+                *ts = err.c_str();
+                s2free.push_back(std::move(err));
                 hoc_execerr_ext("python string arg cannot decode into c_str. Pyerr message: %s",
                                 *ts);
             }
             *ts = str.c_str();
-            s2free.push_back(*ts);
+            s2free.push_back(neuron::unique_cstr(*ts));
             hoc_pushstr(ts);
         } else if (PyObject_TypeCheck(po, hocobject_type)) {
             // The PyObject_TypeCheck above used to be PyObject_IsInstance. The
@@ -479,16 +481,6 @@ int hocobj_pushargs(PyObject* args, std::vector<char*>& s2free) {
     return narg;
 }
 
-void hocobj_pushargs_free_strings(std::vector<char*>& s2free) {
-    std::vector<char*>::iterator it = s2free.begin();
-    for (; it != s2free.end(); ++it) {
-        if (*it) {
-            free(*it);
-        }
-    }
-    s2free.clear();
-}
-
 static Symbol* getsym(char* name, Object* ho, int fail) {
     Symbol* sym = 0;
     if (ho) {
@@ -727,16 +719,12 @@ static void* fcall(void* vself, void* vargs) {
         hoc_push_object(self->ho_);
     }
 
-    // TODO: this will still have some memory leaks in case of errors.
-    //      see discussion in https://github.com/neuronsimulator/nrn/pull/1437
-    std::vector<char*> strings_to_free;
-
+    std::vector<neuron::unique_cstr> strings_to_free;
     int narg = hocobj_pushargs((PyObject*) vargs, strings_to_free);
     int var_type;
     if (self->ho_) {
         self->nindex_ = narg;
         var_type = component(self);
-        hocobj_pushargs_free_strings(strings_to_free);
         switch (var_type) {
         case 2:
             return nrnpy_hoc_bool_pop();
@@ -767,7 +755,6 @@ static void* fcall(void* vself, void* vargs) {
             ((PyObject*) result)->ob_type = location->second;
         }
 
-        hocobj_pushargs_free_strings(strings_to_free);
         return result;
     } else {
         HocTopContextSet
@@ -782,7 +769,6 @@ static void* fcall(void* vself, void* vargs) {
         hoc_pc = pcsav;
         HocContextRestore;
     }
-    hocobj_pushargs_free_strings(strings_to_free);
 
     return nrnpy_hoc_pop("laststatement fcall");
 }

src/nrnpython/nrnpy_nrn.cpp

@@ -11,6 +11,7 @@
 #include "nrnpy.h"
 #include "nrnpy_utils.h"
 #include "convert_cxx_exceptions.hpp"
+#include "neuron/unique_cstr.hpp"
 
 #ifndef M_PI
 #define M_PI (3.14159265358979323846)
@@ -40,8 +41,7 @@ extern void nrn_pt3dstyle0(Section* sec);
 
 extern PyObject* nrn_ptr_richcmp(void* self_ptr, void* other_ptr, int op);
 // used to be static in nrnpy_hoc.cpp
-extern int hocobj_pushargs(PyObject*, std::vector<char*>&);
-extern void hocobj_pushargs_free_strings(std::vector<char*>&);
+extern int hocobj_pushargs(PyObject*, std::vector<neuron::unique_cstr>&);
 
 
 struct NPyAllSegOfSecIter {
@@ -1317,7 +1317,7 @@ static PyObject* NPyMechFunc_call(NPyMechFunc* self, PyObject* args) {
     // patterning after fcall
     Symbol sym{};  // in case of error, need the name.
     sym.name = (char*) self->f_->name;
-    std::vector<char*> strings_to_free;
+    std::vector<neuron::unique_cstr> strings_to_free;
     int narg = hocobj_pushargs(args, strings_to_free);
     hoc_push_frame(&sym, narg);  // get_argument uses the current frame
     try {
@@ -1329,7 +1329,6 @@ static PyObject* NPyMechFunc_call(NPyMechFunc* self, PyObject* args) {
         PyErr_SetString(PyExc_RuntimeError, oss.str().c_str());
     }
     hoc_pop_frame();
-    hocobj_pushargs_free_strings(strings_to_free);
 
     return result;
 }