Merge pull request #286 from ptweezy/c-error-paths

Fix three out-of-memory crashes in the C extensions

Author: rhpvorderman

CHANGELOG.rst

@@ -109,6 +109,16 @@ version 1.8.1-dev
   ``igzip.IGzipFile`` was not affected.
 + Fix a crash when there is not enough memory to create an
   ``igzip_lib.IgzipDecompressor``. It raises ``MemoryError`` now.
++ Fix a crash when there is not enough memory to raise ``BadGzipFile`` for
+  data that is not in the gzip format, for example, in ``igzip.decompress``.
+  It raises ``MemoryError`` now.
++ Fix a crash in ``igzip_lib.IgzipDecompressor.decompress()`` after a call on
+  the same object raised ``MemoryError``. The next call used the input of the
+  failed call after the caller had released it. This matches the CPython fix
+  for gh-148395.
++ Fix a crash on macOS and PyPy when there is not enough memory to create the
+  internal lock of an ``isal_zlib.Compress``, ``isal_zlib.Decompress``, or
+  ``igzip_lib.IgzipDecompressor`` object. It raises ``MemoryError`` now.
 
 version 1.8.0
 -----------------

src/isal/igzip_libmodule.c

@@ -44,7 +44,9 @@ typedef struct {
 static void
 IgzipDecompressor_dealloc(IgzipDecompressor *self)
 {
-    PyThread_free_lock(self->lock);
+    if (self->lock != NULL) {
+        PyThread_free_lock(self->lock);
+    }
     PyMem_Free(self->input_buffer);
     Py_CLEAR(self->unused_data);
     Py_CLEAR(self->zdict);
@@ -298,6 +300,10 @@ decompress(IgzipDecompressor *self, uint8_t *data, size_t len, Py_ssize_t max_le
     return result;
 
 error:
+    /* next_in can point to the buffer of the caller, which is released after
+       this call. Reset it so the next call does not use it, as CPython does
+       since gh-148395. */
+    self->state.next_in = NULL;
     Py_XDECREF(result);
     return NULL;
 }
@@ -480,6 +486,7 @@ igzip_lib_IgzipDecompressor__new__(PyTypeObject *type,
     self->avail_in_real = 0;
     self->input_buffer = NULL;
     self->input_buffer_size = 0;
+    self->lock = NULL;
     Py_XINCREF(zdict);
     self->zdict = zdict;
     self->unused_data = PyBytes_FromStringAndSize(NULL, 0);

src/isal/isal_zlibmodule.c

@@ -594,7 +594,9 @@ Comp_dealloc(compobject *self)
 {
     if (self->is_initialised && self->level_buf != NULL)
         PyMem_Free(self->level_buf);
-    PyThread_free_lock(self->lock);
+    if (self->lock != NULL) {
+        PyThread_free_lock(self->lock);
+    }
     Py_XDECREF(self->zdict);
     Py_TYPE(self)->tp_free((PyObject *)self);
 }
@@ -639,7 +641,9 @@ typedef struct
 static void
 Decomp_dealloc(decompobject *self)
 {
-    PyThread_free_lock(self->lock);
+    if (self->lock != NULL) {
+        PyThread_free_lock(self->lock);
+    }
     Py_XDECREF(self->unused_data);
     Py_XDECREF(self->unconsumed_tail);
     Py_XDECREF(self->zdict);
@@ -683,6 +687,9 @@ newdecompobject()
     self->is_initialised = 0;
     self->method_set = 0;
     self->zdict = NULL;
+    // Decomp_dealloc uses these members when initialization fails
+    self->unconsumed_tail = NULL;
+    self->lock = NULL;
     self->unused_data = PyBytes_FromStringAndSize("", 0);
     if (self->unused_data == NULL) {
         Py_DECREF(self);
@@ -693,7 +700,7 @@ newdecompobject()
         Py_DECREF(self);
         return NULL;
     }
-        self->lock = PyThread_allocate_lock();
+    self->lock = PyThread_allocate_lock();
     if (self->lock == NULL) {
         Py_DECREF(self);
         PyErr_SetString(PyExc_MemoryError, "Unable to allocate lock");
@@ -1578,7 +1585,7 @@ static PyGetSetDef Decomp_properties[] = {
 
 static PyTypeObject IsalZlibDecompType = {
     .tp_name = "isal_zlib.Decompress",
-    .tp_doc = "Object returned by isal_zlib.compressobj.",
+    .tp_doc = "Object returned by isal_zlib.decompressobj.",
     .tp_basicsize = sizeof(decompobject),
     .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_DISALLOW_INSTANTIATION,
     .tp_dealloc = (destructor)Decomp_dealloc,
@@ -1845,6 +1852,9 @@ GzipReader_read_into_buffer(GzipReader *self, uint8_t *out_buffer, size_t out_bu
                     if (!(magic1 == 0x1f && magic2 == 0x8b)) {
                         Py_BLOCK_THREADS;
                         PyObject *magic_obj = PyBytes_FromStringAndSize((char *)current_pos, 2);
+                        if (magic_obj == NULL) {
+                            return -1;
+                        }
                         raise_bad_gzip_file(
                             "Not a gzipped file (%R)", 
                             magic_obj);

tests/test_compat.py

@@ -12,6 +12,9 @@
 import gzip
 import itertools
 import random
+import subprocess
+import sys
+import textwrap
 import threading
 import warnings
 import zlib
@@ -66,6 +69,28 @@ def test_compress_and_decompress_types_cannot_be_called(factory):
         type(factory())()
 
 
+@pytest.mark.skipif(sys.implementation.name != "cpython",
+                    reason="Needs _testcapi.set_nomemory")
+@pytest.mark.parametrize("factory", ["compressobj", "decompressobj"])
+def test_compressobj_decompressobj_no_memory(factory):
+    pytest.importorskip("_testcapi")
+    # Fail each memory allocation in turn, such as the one for the lock. A
+    # subprocess keeps the failing allocations away from the test runner.
+    code = textwrap.dedent(f"""
+        import _testcapi
+        from isal import isal_zlib
+        for start in range(20):
+            _testcapi.set_nomemory(start, start + 1)
+            try:
+                isal_zlib.{factory}()
+            except MemoryError:
+                pass
+            finally:
+                _testcapi.remove_mem_hooks()
+    """)
+    subprocess.run([sys.executable, "-c", code], check=True)
+
+
 @pytest.mark.parametrize(["data_size", "level"],
                          itertools.product(DATA_SIZES, range(4)))
 def test_compress(data_size, level):

tests/test_igzip.py

@@ -19,6 +19,7 @@
 import subprocess
 import sys
 import tempfile
+import textwrap
 import zlib
 from gzip import FCOMMENT, FEXTRA, FHCRC, FNAME, FTEXT  # type: ignore
 from pathlib import Path
@@ -457,6 +458,28 @@ def test_decompress_not_a_gzip():
     assert error.match(re.escape("Not a gzipped file (b'Th')"))
 
 
+@pytest.mark.skipif(sys.implementation.name != "cpython",
+                    reason="Needs _testcapi.set_nomemory")
+def test_decompress_not_a_gzip_no_memory():
+    pytest.importorskip("_testcapi")
+    # Fail each memory allocation in turn, such as the one for the bytes in
+    # the error message. A subprocess keeps the failing allocations away from
+    # the test runner. Each call raises MemoryError or BadGzipFile.
+    code = textwrap.dedent("""
+        import _testcapi
+        from isal import igzip
+        for start in range(50):
+            _testcapi.set_nomemory(start, start + 1)
+            try:
+                igzip.decompress(b"This is not a gzip data stream.")
+            except (MemoryError, igzip.BadGzipFile):
+                pass
+            finally:
+                _testcapi.remove_mem_hooks()
+    """)
+    subprocess.run([sys.executable, "-c", code], check=True)
+
+
 def test_decompress_unknown_compression_method():
     corrupted_data = COMPRESSED_DATA[:2] + b'\x09' + COMPRESSED_DATA[3:]
     with pytest.raises(igzip.BadGzipFile) as error:

tests/test_igzip_lib.py

@@ -299,18 +299,51 @@ def test_igzip_decompressor_keeps_zdict_reference():
                     reason="Needs _testcapi.set_nomemory")
 def test_igzip_decompressor_no_memory():
     pytest.importorskip("_testcapi")
-    # Fail the first memory allocation, which is the object itself. A
-    # subprocess keeps the failing allocation away from the test runner.
+    # Fail each memory allocation in turn, such as the one for the object
+    # itself or for its lock. A subprocess keeps the failing allocations away
+    # from the test runner.
     code = textwrap.dedent("""
         import _testcapi
         from isal.igzip_lib import IgzipDecompressor
-        _testcapi.set_nomemory(0, 1)
-        try:
-            IgzipDecompressor()
-        except MemoryError:
-            pass
-        finally:
-            _testcapi.remove_mem_hooks()
+        for start in range(20):
+            _testcapi.set_nomemory(start, start + 1)
+            try:
+                IgzipDecompressor()
+            except MemoryError:
+                pass
+            finally:
+                _testcapi.remove_mem_hooks()
+    """)
+    subprocess.run([sys.executable, "-c", code], check=True)
+
+
+@pytest.mark.skipif(sys.implementation.name != "cpython",
+                    reason="Needs _testcapi.set_nomemory")
+def test_igzip_decompressor_decompress_no_memory():
+    pytest.importorskip("_testcapi")
+    # After a MemoryError, the next call must not use the input of the failed
+    # call. That input is in an mmap, so using it after close() crashes.
+    code = textwrap.dedent("""
+        import mmap
+        import os
+        import _testcapi
+        from isal import igzip_lib
+        # Random data does not compress, so most of the input is left over
+        # after 10 bytes of output. decompress() copies it to a new buffer.
+        compressed = igzip_lib.compress(os.urandom(256 * 1024))
+        for start in range(20):
+            decompressor = igzip_lib.IgzipDecompressor()
+            buffer = mmap.mmap(-1, len(compressed))
+            buffer.write(compressed)
+            _testcapi.set_nomemory(start, start + 1)
+            try:
+                decompressor.decompress(buffer, 10)
+            except MemoryError:
+                pass
+            finally:
+                _testcapi.remove_mem_hooks()
+            buffer.close()
+            decompressor.decompress(b"")
     """)
     subprocess.run([sys.executable, "-c", code], check=True)