Fix crashes, a hang, and a heap over-read in the C extensions

CHANGELOG.rst

@@ -18,6 +18,22 @@ version 1.8.1-dev
   by standard zlib, including builds linked against older ISA-L releases.
 + Fix backward seeking in igzip files by discarding buffered compressed data
   when rewinding the underlying file.
++ Fix a crash when ``igzip.decompress`` gets an object that is not
+  bytes-like, such as a ``str``. It raises a ``TypeError`` instead.
++ Fix a crash after creating ``igzip_lib.IgzipDecompressor`` objects with a
+  ``zdict``.
++ Fix a hang in ``isal_zlib.Compress``: after ``flush()`` raised an error for
+  an unsupported mode, the next call on the same object never returned.
++ Fix a crash when you call ``flush(Z_FINISH)`` twice on a level 0
+  ``isal_zlib.Compress`` object.
++ Fix ``unused_data`` of ``isal_zlib.Decompress`` and
+  ``igzip_lib.IgzipDecompressor``, which could contain bytes from outside the
+  input buffer when ``max_length`` split the end of a stream. At the end of a
+  stream, ``isal_zlib.Decompress.unconsumed_tail`` is empty, matching the
+  CPython fix for gh-158169.
++ ``igzip_threaded.open`` raises ``ValueError`` for a ``block_size`` of about
+  3.6 GiB or more and ``TypeError`` for a ``compresslevel`` that is not an
+  integer. Both raised ``SystemError`` before.
 + Extraordinarily large gzip headers with either a very long comment or
   filename will now cause an ``OverflowError`` rather than use more and more
   memory. The size is limited to the internal buffer size which is currently

src/isal/igzip_libmodule.c

@@ -34,6 +34,8 @@ typedef struct {
     Py_ssize_t avail_in_real;
     char eof;           /* T_BOOL expects a char */
     char needs_input;
+    uint8_t saved_bitbuffer[8];
+    Py_ssize_t saved_bitbuffer_size;
     /* Struct inflate state contains a massive buffer at the end. Put it at
        the end of the IgzipDecompressor so members can be accessed easily. */
     struct inflate_state state;
@@ -226,6 +228,7 @@ decompress(IgzipDecompressor *self, uint8_t *data, size_t len, Py_ssize_t max_le
         input_buffer_in_use = 0;
     }
 
+    uint8_t *input_start = self->state.next_in;
     result = decompress_buf(self, max_length);
     if(result == NULL) {
         self->state.next_in = NULL;
@@ -242,8 +245,14 @@ decompress(IgzipDecompressor *self, uint8_t *data, size_t len, Py_ssize_t max_le
                 goto error;
             char *new_data_ptr = PyBytes_AS_STRING(new_data);
             bitbuffer_copy(&(self->state), new_data_ptr, bytes_in_bitbuffer);
-            memcpy(new_data_ptr + bytes_in_bitbuffer, self->state.next_in, 
-                   self->avail_in_real);
+            if (copy_from_next_in(&(self->state), input_start,
+                                  self->saved_bitbuffer,
+                                  self->saved_bitbuffer_size,
+                                  new_data_ptr + bytes_in_bitbuffer,
+                                  self->avail_in_real) < 0) {
+                Py_DECREF(new_data);
+                goto error;
+            }
             Py_XSETREF(self->unused_data, new_data);
         }
     }
@@ -282,6 +291,10 @@ decompress(IgzipDecompressor *self, uint8_t *data, size_t len, Py_ssize_t max_le
             self->state.next_in = self->input_buffer;
         }
     }
+    if (!self->eof) {
+        save_bitbuffer(&(self->state), self->saved_bitbuffer,
+                       &self->saved_bitbuffer_size);
+    }
     return result;
 
 error:
@@ -460,9 +473,11 @@ igzip_lib_IgzipDecompressor__new__(PyTypeObject *type,
     IgzipDecompressor *self = PyObject_New(IgzipDecompressor, type); 
     self->eof = 0;
     self->needs_input = 1;
+    self->saved_bitbuffer_size = 0;
     self->avail_in_real = 0;
     self->input_buffer = NULL;
     self->input_buffer_size = 0;
+    Py_XINCREF(zdict);
     self->zdict = zdict;
     self->unused_data = PyBytes_FromStringAndSize(NULL, 0);
     if (self->unused_data == NULL) {

src/isal/isal_shared.h

@@ -219,6 +219,40 @@ static int bitbuffer_copy(struct inflate_state *state, char *to, size_t n){
     return 0;
 }
 
+/* When a stream ends, isal_inflate moves next_in back over the unused bytes
+   in the bit buffer. If a previous call read those bytes, next_in points
+   before the current input. This keeps a copy of them after each call. */
+static inline void
+save_bitbuffer(struct inflate_state *state, uint8_t *saved,
+               Py_ssize_t *saved_size)
+{
+    *saved_size = bitbuffer_size(state);
+    bitbuffer_copy(state, (char *)saved, *saved_size);
+}
+
+/* Copy size bytes starting at state->next_in. Bytes before input_start come
+   from the saved bit buffer. */
+static int
+copy_from_next_in(struct inflate_state *state, uint8_t *input_start,
+                  uint8_t *saved, Py_ssize_t saved_size,
+                  char *to, Py_ssize_t size)
+{
+    uint8_t *from = state->next_in;
+    Py_ssize_t before_input = 0;
+    if ((uintptr_t)from < (uintptr_t)input_start) {
+        before_input = (Py_ssize_t)((uintptr_t)input_start - (uintptr_t)from);
+        if (before_input > saved_size || before_input > size) {
+            PyErr_SetString(PyExc_RuntimeError,
+                            "Leftover input before the buffer is unavailable");
+            return -1;
+        }
+        memcpy(to, saved + saved_size - before_input, before_input);
+        from = input_start;
+    }
+    memcpy(to + before_input, from, size - before_input);
+    return 0;
+}
+
 static inline void
 arrange_input_buffer(uint32_t *avail_in, Py_ssize_t *remains)
 {

src/isal/isal_zlibmodule.c

@@ -310,8 +310,8 @@ ParallelCompress__new__(PyTypeObject *type, PyObject *args, PyObject *kwargs)
     int level = ISAL_DEFAULT_COMPRESSION;
     static char *format = "n|i:ParallelCompress__new__";
     static char *kwarg_names[] = {"buffersize", "level", NULL};
-    if (PyArg_ParseTupleAndKeywords(args, kwargs, format, kwarg_names, 
-        &buffer_size, &level) < 0) {
+    if (!PyArg_ParseTupleAndKeywords(args, kwargs, format, kwarg_names, 
+        &buffer_size, &level)) {
             return NULL;
     }
     uint32_t level_buf_size;
@@ -323,6 +323,7 @@ ParallelCompress__new__(PyTypeObject *type, PyObject *args, PyObject *kwargs)
         PyErr_Format(PyExc_ValueError, 
         "buffersize must be at most %zd, got %zd", 
         (Py_ssize_t)UINT32_MAX, buffer_size);
+        return NULL;
     }
     ParallelCompress *self = PyObject_New(ParallelCompress, type);
     if (self == NULL) {
@@ -617,6 +618,8 @@ typedef struct
     int is_initialised;
     int method_set;
     char eof;
+    uint8_t saved_bitbuffer[8];
+    Py_ssize_t saved_bitbuffer_size;
     PyThread_type_lock lock;
     // inflate_state should be at the bottom as it contains buffers inside the struct.
     struct inflate_state zst;
@@ -665,6 +668,7 @@ newdecompobject()
     if (self == NULL)
         return NULL;
     self->eof = 0;
+    self->saved_bitbuffer_size = 0;
     self->is_initialised = 0;
     self->method_set = 0;
     self->zdict = NULL;
@@ -915,7 +919,8 @@ save_unconsumed_input(decompobject *self, Py_buffer *data, int err)
             Py_ssize_t new_size, left_size;
             PyObject *new_data;
             Py_ssize_t bytes_in_bitbuffer = bitbuffer_size(&(self->zst));
-            left_size = (uint8_t *)data->buf + data->len - self->zst.next_in;
+            left_size = (Py_ssize_t)((uintptr_t)data->buf + data->len -
+                                     (uintptr_t)self->zst.next_in);
             if (left_size + bytes_in_bitbuffer > (PY_SSIZE_T_MAX - old_size)) {
                 PyErr_NoMemory();
                 return -1;
@@ -929,9 +934,16 @@ save_unconsumed_input(decompobject *self, Py_buffer *data, int err)
             memcpy(new_data_ptr,
                    PyBytes_AS_STRING(self->unused_data), old_size);
             bitbuffer_copy(&(self->zst), new_data_ptr + old_size, bytes_in_bitbuffer);
-            memcpy(new_data_ptr + old_size + bytes_in_bitbuffer,
-                   self->zst.next_in, left_size);       
+            if (copy_from_next_in(&(self->zst), data->buf,
+                                  self->saved_bitbuffer,
+                                  self->saved_bitbuffer_size,
+                                  new_data_ptr + old_size + bytes_in_bitbuffer,
+                                  left_size) < 0) {
+                Py_DECREF(new_data);
+                return -1;
+            }
             Py_SETREF(self->unused_data, new_data);
+            self->zst.next_in = (uint8_t *)data->buf + data->len;
             self->zst.avail_in = 0;
         }
     }
@@ -948,6 +960,10 @@ save_unconsumed_input(decompobject *self, Py_buffer *data, int err)
         Py_SETREF(self->unconsumed_tail, new_data);
     }
 
+    if (self->zst.block_state != ISAL_BLOCK_FINISH) {
+        save_bitbuffer(&(self->zst), self->saved_bitbuffer,
+                       &self->saved_bitbuffer_size);
+    }
     return 0;
 }
 
@@ -1076,6 +1092,7 @@ isal_zlib_Compress_flush_impl(compobject *self, int mode)
     } else {
         PyErr_Format(IsalError, 
                      "Unsupported flush mode: %d", mode);
+        LEAVE_ZLIB(self);
         return NULL;
     }
 
@@ -1111,6 +1128,7 @@ isal_zlib_Compress_flush_impl(compobject *self, int mode)
        mode is Z_FINISH, but checking both for safety*/
     if (self->zst.internal_state.state == ZSTATE_END && mode == Z_FINISH) {
         PyMem_FREE(self->level_buf);
+        self->level_buf = NULL;
         self->zst.level_buf_size = 0;
         self->zst.level_buf = NULL;
         self->is_initialised = 0;
@@ -1533,7 +1551,9 @@ static void GzipReader_dealloc(GzipReader *self)
         PyMem_Free(self->memview);
     }
     Py_XDECREF(self->fp);
-    PyThread_free_lock(self->lock);
+    if (self->lock != NULL) {
+        PyThread_free_lock(self->lock);
+    }
     Py_TYPE(self)->tp_free(self);
 }
 
@@ -1574,8 +1594,15 @@ GzipReader__new__(PyTypeObject *type, PyObject *args, PyObject *kwargs)
         return NULL;
     }
     GzipReader *self = PyObject_New(GzipReader, type);
+    if (self == NULL) {
+        return NULL;
+    }
+    // GzipReader_dealloc uses these members when initialization fails
+    self->input_buffer = NULL;
+    self->memview = NULL;
+    self->fp = NULL;
+    self->lock = NULL;
     if (PyObject_HasAttrString(fp, "read")) {
-        self->memview = NULL;
         self->buffer_size = buffer_size;
         self->input_buffer = PyMem_Malloc(self->buffer_size);
         if (self->input_buffer == NULL) {
@@ -1585,14 +1612,17 @@ GzipReader__new__(PyTypeObject *type, PyObject *args, PyObject *kwargs)
         self->buffer_end = self->input_buffer;
         self->all_bytes_read = 0;
     } else {
-        self->memview = PyMem_Malloc(sizeof(Py_buffer));
-        if (self->memview == NULL) {
+        Py_buffer *memview = PyMem_Malloc(sizeof(Py_buffer));
+        if (memview == NULL) {
+            Py_DECREF(self);
             return PyErr_NoMemory();
         }
-        if (PyObject_GetBuffer(fp, self->memview, PyBUF_SIMPLE) < 0) {
+        if (PyObject_GetBuffer(fp, memview, PyBUF_SIMPLE) < 0) {
+            PyMem_Free(memview);
             Py_DECREF(self);
             return NULL;
         }
+        self->memview = memview;
         self->buffer_size = self->memview->len;
         self->input_buffer = self->memview->buf;
         self->buffer_end = self->input_buffer + self->buffer_size;

tests/test_compat.py

@@ -11,6 +11,7 @@
 
 import gzip
 import itertools
+import threading
 import zlib
 from pathlib import Path
 
@@ -100,6 +101,28 @@ def test_compress_compressobj(data_size, level, wbits, memLevel):
     assert data == decompressed
 
 
+def test_compressobj_flush_unsupported_mode():
+    compressor = isal_zlib.compressobj()
+    with pytest.raises(isal_zlib.error) as error:
+        compressor.flush(isal_zlib.Z_PARTIAL_FLUSH)
+    error.match("Unsupported flush mode")
+    # The next call hangs if the failed flush kept the lock
+    result = []
+    thread = threading.Thread(target=lambda: result.append(
+        compressor.compress(b"data") + compressor.flush()), daemon=True)
+    thread.start()
+    thread.join(timeout=10)
+    assert result
+    assert zlib.decompress(result[0]) == b"data"
+
+
+def test_compressobj_level_0_flush_after_finish():
+    compressor = isal_zlib.compressobj(0)
+    compressed = compressor.compress(b"data") + compressor.flush()
+    assert compressor.flush() == b""
+    assert zlib.decompress(compressed) == b"data"
+
+
 @pytest.mark.parametrize(["data_size", "level", "wbits", "memLevel"],
                          itertools.product([128 * 1024], range(4),
                                            WBITS_RANGE, range(1, 10)))
@@ -162,6 +185,21 @@ def test_unused_data(unused_size, wbits):
     assert decompressor.unused_data == unused_data
 
 
+@pytest.mark.parametrize("wbits", [-15, 15, 31])
+def test_decompressobj_unconsumed_tail_after_eof(wbits):
+    source = b"abcdefghijklmnopqrstuvwxyz"
+    remainder = b"0123456789"
+    compressor = zlib.compressobj(wbits=wbits)
+    compressed = compressor.compress(source) + compressor.flush()
+    decompressor = isal_zlib.decompressobj(wbits=wbits)
+    data = decompressor.decompress(compressed + remainder, 1)
+    data += decompressor.decompress(decompressor.unconsumed_tail)
+    assert decompressor.eof
+    assert data == source
+    assert decompressor.unconsumed_tail == b""
+    assert decompressor.unused_data == remainder
+
+
 def test_zlib_dictionary_decompress():
     dictionary = b"bla"
     data = b"bladiebla"

tests/test_igzip.py

@@ -328,6 +328,18 @@ def test_decompress_empty():
     assert igzip.decompress(b"") == b""
 
 
+@pytest.mark.parametrize("data", ["x", 1, None])
+def test_decompress_not_bytes_like(data):
+    with pytest.raises(TypeError) as error:
+        igzip.decompress(data)
+    error.match("a bytes-like object is required")
+
+
+def test_decompress_not_contiguous():
+    with pytest.raises(BufferError):
+        igzip.decompress(memoryview(b"abcd")[::2])
+
+
 def headers():
     magic = b"\x1f\x8b"
     method = b"\x08"

tests/test_igzip_lib.py

@@ -257,6 +257,42 @@ def test_igzip_decompressor_raw_deflate_unused_data_gzip(test_offset):
         assert igzd.unused_data == true_unused_data
 
 
+@pytest.mark.parametrize(["flag", "wbits"], [(DECOMP_DEFLATE, -15),
+                                             (DECOMP_ZLIB, 15),
+                                             (DECOMP_GZIP, 31)])
+def test_igzip_decompressor_unused_data_after_max_length(flag, wbits):
+    data = b"hello world " * 300
+    unused_data = b"0123456789"
+    compressor = zlib.compressobj(wbits=wbits)
+    compressed = compressor.compress(data) + compressor.flush() + unused_data
+    stream_end = len(compressed) - len(unused_data)
+    # The stream can end in bytes that an earlier call read
+    for chunk_size in range(1, 25):
+        decompressor = IgzipDecompressor(flag=flag)
+        result = b""
+        position = 0
+        while not decompressor.eof:
+            chunk = b""
+            if decompressor.needs_input:
+                assert position < len(compressed)
+                chunk = compressed[position:position + chunk_size]
+                position += chunk_size
+            result += decompressor.decompress(chunk, 7)
+        assert result == data
+        assert decompressor.unused_data == compressed[stream_end:position]
+
+
+@pytest.mark.skipif(not hasattr(sys, "getrefcount"),
+                    reason="Needs reference counting")
+def test_igzip_decompressor_keeps_zdict_reference():
+    zdict = bytes(range(256))
+    refcount = sys.getrefcount(zdict)
+    decompressor = IgzipDecompressor(flag=DECOMP_ZLIB, zdict=zdict)
+    assert sys.getrefcount(zdict) == refcount + 1
+    del decompressor
+    assert sys.getrefcount(zdict) == refcount
+
+
 @pytest.mark.parametrize(["unused_size", "flag_pair", "data_size"],
                          itertools.product([26], FLAGS,
                                            [128 * 1024 - 3, 874, 81923, 9111]))