Merge pull request #18 from P403n1x87/fix/pybytes-memleak

fix: memory leak in Python bytes object retrieval

echion/cpython/tasks.h

@@ -193,7 +193,7 @@ extern "C"
                 return NULL;
 
             Py_ssize_t s = 0;
-            unsigned char *c = (unsigned char *)pybytes_to_bytes_and_size(code.co_code, &s);
+            auto c = pybytes_to_bytes_and_size(code.co_code, &s);
 
             if (c[(frame.f_lasti + 1) * sizeof(_Py_CODEUNIT)] != YIELD_FROM)
                 return NULL;
@@ -233,7 +233,7 @@ extern "C"
                 return NULL;
 
             Py_ssize_t s = 0;
-            unsigned char *c = (unsigned char *)pybytes_to_bytes_and_size(code.co_code, &s);
+            auto c = pybytes_to_bytes_and_size(code.co_code, &s);
 
             if (c[f->f_lasti + sizeof(_Py_CODEUNIT)] != YIELD_FROM)
                 return NULL;

echion/frame.h

@@ -43,6 +43,15 @@ class Frame
         }
     };
 
+    class LocationError : public Error
+    {
+    public:
+        const char *what() const noexcept override
+        {
+            return "Cannot determine frame location information";
+        }
+    };
+
     std::optional<std::string> filename = std::nullopt;
     std::optional<std::string> name = std::nullopt;
     struct _location
@@ -85,20 +94,20 @@ class Frame
         return Frame::read(frame_addr, &unused);
     }
 
-    bool is_valid();
-
     static Frame &get(PyCodeObject *code, int lasti);
     static Frame &get(unw_word_t pc, const char *name, unw_word_t offset);
 
     Frame(PyCodeObject *, int);
     Frame(unw_word_t, const char *, unw_word_t);
 
 private:
+    inline bool is_valid();
+    void infer_location(PyCodeObject *, int);
+
     static inline uintptr_t key(PyCodeObject *code, int lasti)
     {
         return (((uintptr_t)(((uintptr_t)code) & MOJO_INT32) << 16) | lasti);
     }
-    int infer_location(PyCodeObject *, int);
 };
 
 #if PY_VERSION_HEX >= 0x030b0000
@@ -126,16 +135,17 @@ _read_signed_varint(unsigned char *table, ssize_t *i)
 #endif
 
 // ----------------------------------------------------------------------------
-int Frame::infer_location(PyCodeObject *code, int lasti)
+void Frame::infer_location(PyCodeObject *code, int lasti)
 {
     unsigned int lineno = code->co_firstlineno;
     Py_ssize_t len = 0;
-    unsigned char *table = NULL;
 
 #if PY_VERSION_HEX >= 0x030b0000
-    table = (unsigned char *)pybytes_to_bytes_and_size(code->co_linetable, &len);
-    if (table == NULL)
-        return 1;
+    auto table = pybytes_to_bytes_and_size(code->co_linetable, &len);
+    if (table == nullptr)
+        return;
+
+    auto table_data = table.get();
 
     for (Py_ssize_t i = 0, bc = 0; i < len; i++)
     {
@@ -148,17 +158,17 @@ int Frame::infer_location(PyCodeObject *code, int lasti)
             break;
 
         case 14: // Long form
-            lineno += _read_signed_varint(table, &i);
+            lineno += _read_signed_varint(table_data, &i);
 
             this->location.line = lineno;
-            this->location.line_end = lineno + _read_varint(table, &i);
-            this->location.column = _read_varint(table, &i);
-            this->location.column_end = _read_varint(table, &i);
+            this->location.line_end = lineno + _read_varint(table_data, &i);
+            this->location.column = _read_varint(table_data, &i);
+            this->location.column_end = _read_varint(table_data, &i);
 
             break;
 
         case 13: // No column data
-            lineno += _read_signed_varint(table, &i);
+            lineno += _read_signed_varint(table_data, &i);
 
             this->location.line = lineno;
             this->location.line_end = lineno;
@@ -191,12 +201,10 @@ int Frame::infer_location(PyCodeObject *code, int lasti)
             break;
     }
 
-    return 0;
-
 #elif PY_VERSION_HEX >= 0x030a0000
-    table = (unsigned char *)pybytes_to_bytes_and_size(code->co_linetable, &len);
-    if (table == NULL)
-        return 1;
+    auto table = pybytes_to_bytes_and_size(code->co_linetable, &len);
+    if (table == nullptr)
+        return;
 
     lasti <<= 1;
     for (int i = 0, bc = 0; i < len; i++)
@@ -219,9 +227,9 @@ int Frame::infer_location(PyCodeObject *code, int lasti)
     }
 
 #else
-    table = (unsigned char *)pybytes_to_bytes_and_size(code->co_lnotab, &len);
-    if (table == NULL)
-        return 1;
+    auto table = pybytes_to_bytes_and_size(code->co_lnotab, &len);
+    if (table == nullptr)
+        return;
 
     for (int i = 0, bc = 0; i < len; i++)
     {
@@ -241,8 +249,6 @@ int Frame::infer_location(PyCodeObject *code, int lasti)
     this->location.line_end = lineno;
     this->location.column = 0;
     this->location.column_end = 0;
-
-    return 0;
 }
 
 // ----------------------------------------------------------------------------
@@ -285,7 +291,7 @@ Frame::Frame(unw_word_t pc, const char *name, unw_word_t offset)
     this->location.line = offset;
 }
 
-bool Frame::is_valid()
+inline bool Frame::is_valid()
 {
 #if PY_VERSION_HEX >= 0x030c0000
     // Shim frames might not have location information
@@ -326,14 +332,14 @@ Frame &Frame::get(PyCodeObject *code_addr, int lasti)
     }
     catch (LRUCache<uintptr_t, Frame>::LookupError &)
     {
-
         auto new_frame = std::make_unique<Frame>(&code, lasti);
+        // DEV: Throwing an exception in the Frame constructor causes a SIGABRT
+        // even when catching it on Linux. So we keep the check like this for
+        // now.
         if (!new_frame->is_valid())
             return INVALID_FRAME;
-
         auto &f = *new_frame;
         frame_cache->store(frame_key, std::move(new_frame));
-
         return f;
     }
 }

echion/strings.h

@@ -15,24 +15,21 @@
 #include <echion/vm.h>
 
 // ----------------------------------------------------------------------------
-static char *
+static std::unique_ptr<unsigned char[]>
 pybytes_to_bytes_and_size(PyObject *bytes_addr, Py_ssize_t *size)
 {
     PyBytesObject bytes;
 
     if (copy_type(bytes_addr, bytes))
-        return NULL;
+        return nullptr;
 
     *size = bytes.ob_base.ob_size;
     if (*size < 0 || *size > (1 << 20))
-        return NULL;
-
-    char *data = new char[*size];
-    if (copy_generic(((char *)bytes_addr) + offsetof(PyBytesObject, ob_sval), data, *size))
-    {
-        delete[] data;
-        return NULL;
-    }
+        return nullptr;
+
+    auto data = std::make_unique<unsigned char[]>(*size);
+    if (copy_generic(((char *)bytes_addr) + offsetof(PyBytesObject, ob_sval), data.get(), *size))
+        return nullptr;
 
     return data;
 }