Fix memory leak in RESP3 map parsing (#204)

* Fix memory leak in RESP3 map parsing

Fix #175

* Add memray deps installation

* Install deps only on ubuntu

* Disable memray installation on Windows

* Another attempt

* Fix memray on old macOS versions

* Disable memray for PyPy

* Exclude FreeBSD

* Revert

* Another attempt for freebsd fix

* Another attempt for freebsd fix

Author: uglide

.github/workflows/freebsd.yaml

@@ -41,7 +41,8 @@ jobs:
              curl -s https://bootstrap.pypa.io/get-pip.py -o get-pip.py
              python3.9 get-pip.py
              /usr/local/bin/pip install -U pip setuptools wheel
-             /usr/local/bin/pip install -r dev_requirements.txt
+             sed '5,6d' dev_requirements.txt > dev_requirements_without_memray.txt
+             /usr/local/bin/pip install -r dev_requirements_without_memray.txt
              /usr/local/bin/python3.9 setup.py build_ext --inplace
-             python -m pytest
-             python3.9 setup.py bdist_wheel
+             /usr/local/bin/python3.9 -m pytest
+             /usr/local/bin/python3.9 setup.py bdist_wheel

.github/workflows/integration.yaml

@@ -40,7 +40,13 @@ jobs:
            python-version: ${{ matrix.python-version }}
            cache: 'pip'
            cache-dependency-path: dev_requirements.txt
+       - name: Install memray deps
+         if: ${{ matrix.os == 'ubuntu-latest' }}
+         run: |
+           sudo apt-get install build-essential python3-dev libdebuginfod-dev libunwind-dev liblz4-dev -y -qq
        - name: run tests
+         env:
+           MACOSX_DEPLOYMENT_TARGET: 10.14
          run: |
            pip install -U pip setuptools wheel
            pip install -r dev_requirements.txt

dev_requirements.txt

@@ -1,7 +1,9 @@
 black==24.3.0
 flake8==4.0.1
 isort==5.10.1
-pytest>=7.0.0
+pytest>=7.2.0
+memray==1.17.1 ; sys_platform != 'win32' and platform_python_implementation != 'PyPy'
+pytest-memray==1.7.0 ; sys_platform != 'win32' and platform_python_implementation != 'PyPy'
 setuptools
 tox==3.24.4
 vulture>=2.3.0

src/reader.c

@@ -74,12 +74,19 @@ static void *tryParentize(const redisReadTask *task, PyObject *obj) {
             case REDIS_REPLY_MAP:
                 if (task->idx % 2 == 0) {
                     /* Set a temporary item to save the object as a key. */
-                    PyDict_SetItem(parent, obj, Py_None);
+                    int res = PyDict_SetItem(parent, obj, Py_None);
+                    Py_DECREF(obj);
+
+                    if (res == -1) {
+                        return NULL;
+                    }
                 } else {
                     /* Pop the temporary item and set proper key and value. */
                     PyObject *last_item = PyObject_CallMethod(parent, "popitem", NULL);
                     PyObject *last_key = PyTuple_GetItem(last_item, 0);
                     PyDict_SetItem(parent, last_key, obj);
+                    Py_DECREF(last_item);
+                    Py_DECREF(obj);
                 }
                 break;
             default:
@@ -359,18 +366,24 @@ static PyObject *Reader_feed(hiredis_ReaderObject *self, PyObject *args) {
 
 static PyObject *Reader_gets(hiredis_ReaderObject *self, PyObject *args) {
     PyObject *obj;
-    PyObject *err;
-    char *errstr;
 
     self->shouldDecode = 1;
     if (!PyArg_ParseTuple(args, "|i", &self->shouldDecode)) {
         return NULL;
     }
 
     if (redisReaderGetReply(self->reader, (void**)&obj) == REDIS_ERR) {
-        errstr = redisReaderGetError(self->reader);
-        /* protocolErrorClass might be a callable. call it, then use it's type */
-        err = createError(self->protocolErrorClass, errstr, strlen(errstr));
+        PyObject *err = NULL;
+        char *errstr = NULL;
+
+        // Checking if there is no error during the call to redisReaderGetReply
+        // to avoid getting a SystemError.
+        if (PyErr_Occurred() == NULL) {
+            errstr = redisReaderGetError(self->reader);
+            /* protocolErrorClass might be a callable. call it, then use it's type */
+            err = createError(self->protocolErrorClass, errstr, strlen(errstr));
+        }
+
         if (err != NULL) {
             obj = PyObject_Type(err);
             PyErr_SetString(obj, errstr);

tests/test_reader.py

@@ -146,6 +146,45 @@ def test_dict(reader):
   reader.feed(b"%2\r\n+radius\r\n,4.5\r\n+diameter\r\n:9\r\n")
   assert {b"radius": 4.5, b"diameter": 9} == reader.gets()
 
+@pytest.mark.limit_memory("50 KB")
+def test_dict_memory_leaks(reader):
+  data = (
+    b"%5\r\n"
+    b"+radius\r\n,4.5\r\n"
+    b"+diameter\r\n:9\r\n"
+    b"+nested_map\r\n"
+    b"%2\r\n"
+    b"+key1\r\n+value1\r\n"
+    b"+key2\r\n:42\r\n"
+    b"+nested_array\r\n"
+    b"*2\r\n"
+    b"+item1\r\n"
+    b"+item2\r\n"
+    b"+nested_set\r\n"
+    b"~2\r\n"
+    b"+element1\r\n"
+    b"+element2\r\n"
+  )
+  for i in range(10000):
+    reader.feed(data)
+    res = reader.gets()
+    assert {
+             b"radius": 4.5,
+             b"diameter": 9,
+             b"nested_map": {b"key1": b"value1", b"key2": 42},
+             b"nested_array": [b"item1", b"item2"],
+             b"nested_set": [b"element1", b"element2"],
+           } == res
+
+def test_dict_with_unhashable_key(reader):
+    reader.feed(
+      b"%1\r\n"
+      b"%1\r\n+key1\r\n+value1\r\n"
+      b":9\r\n"
+    )
+    with pytest.raises(TypeError):
+      reader.gets()
+
 def test_vector(reader):
   reader.feed(b">4\r\n+pubsub\r\n+message\r\n+channel\r\n+message\r\n")
   assert [b"pubsub", b"message", b"channel", b"message"] == reader.gets()