From dd25c63cd7f14c715f87ced85a80bf001b7e1fe7 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Mon, 23 Oct 2023 11:02:08 +0200 Subject: [PATCH 01/19] Increment version --- setup.py | 2 +- src/isal/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/setup.py b/setup.py index 9bc284fd..1cb13991 100644 --- a/setup.py +++ b/setup.py @@ -135,7 +135,7 @@ def build_isa_l(): setup( name="isal", - version="1.5.1", + version="1.6.0-dev", description="Faster zlib and gzip compatible compression and " "decompression by providing python bindings for the ISA-L " "library.", diff --git a/src/isal/__init__.py b/src/isal/__init__.py index 9306adfc..f7257098 100644 --- a/src/isal/__init__.py +++ b/src/isal/__init__.py @@ -27,4 +27,4 @@ "__version__" ] -__version__ = "1.5.1" +__version__ = "1.6.0-dev" From 367745ac3f5403cc6bc8b7c61882e53c6042da28 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Mon, 23 Oct 2023 13:42:34 +0200 Subject: [PATCH 02/19] Fix readthedocs build configuration --- .readthedocs.yml | 17 +++++++++++++---- docs/conda-environment.yml | 12 ------------ 2 files changed, 13 insertions(+), 16 deletions(-) delete mode 100644 docs/conda-environment.yml diff --git a/.readthedocs.yml b/.readthedocs.yml index 2866edaf..910b7650 100644 --- a/.readthedocs.yml +++ b/.readthedocs.yml @@ -3,7 +3,16 @@ formats: [] # Do not build epub and pdf python: install: - - method: pip - path: . -conda: - environment: docs/conda-environment.yml \ No newline at end of file + - requirements: "requirements-docs.txt" + - method: "pip" + path: "." + +sphinx: + configuration: docs/conf.py + +build: + os: "ubuntu-22.04" + tools: + python: "3" + apt_packages: + - libisal-dev diff --git a/docs/conda-environment.yml b/docs/conda-environment.yml deleted file mode 100644 index a23cb482..00000000 --- a/docs/conda-environment.yml +++ /dev/null @@ -1,12 +0,0 @@ -name: rtd -channels: - - conda-forge - - defaults -dependencies: - - isa-l - - python >=3.6 - - sphinx - - setuptools - - pip: - - sphinx-rtd-theme - - sphinx-argparse \ No newline at end of file From 4afcfaa4d8dbbd79d4a65abe17520da7eaca16ef Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Wed, 25 Oct 2023 19:51:10 +0200 Subject: [PATCH 03/19] Add asan environment --- tox.ini | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tox.ini b/tox.ini index c94c6690..7e74d40a 100644 --- a/tox.ini +++ b/tox.ini @@ -22,6 +22,15 @@ commands = coverage html -i coverage xml -i +[testenv:asan] +setenv= + PYTHONDEVMODE=1 + PYTHONMALLOC=malloc + CFLAGS=-lasan -fsanitize=address -fno-omit-frame-pointer +allowlist_externals=bash +commands= + bash -c 'LD_PRELOAD=$(find /usr/lib -name "libasan.so*" | head -n 1) pytest tests' + [testenv:compliance] deps=pytest commands= From b78ef86d837128d4ce65dc997aa8a20e72c2b483 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Thu, 26 Oct 2023 08:22:46 +0200 Subject: [PATCH 04/19] Add missing decref when error is thrown in decompressobj --- src/isal/isal_zlibmodule.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/isal/isal_zlibmodule.c b/src/isal/isal_zlibmodule.c index 257260f8..41971445 100644 --- a/src/isal/isal_zlibmodule.c +++ b/src/isal/isal_zlibmodule.c @@ -793,6 +793,7 @@ isal_zlib_decompressobj_impl(PyObject *module, int wbits, PyObject *zdict) err = wbits_to_flag_and_hist_bits_inflate(wbits, &hist_bits, &flag); if (err < 0) { PyErr_Format(PyExc_ValueError, "Invalid wbits value: %d", wbits); + Py_DECREF(self); return NULL; } else if (err == 0) { From 47da5dbae7f3e00accccbea9f74d6c9a1793e14a Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 27 Oct 2023 07:11:36 +0200 Subject: [PATCH 05/19] Properly decref magic bytes representation --- src/isal/isal_zlibmodule.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/isal/isal_zlibmodule.c b/src/isal/isal_zlibmodule.c index 41971445..2c5c8a92 100644 --- a/src/isal/isal_zlibmodule.c +++ b/src/isal/isal_zlibmodule.c @@ -1684,9 +1684,11 @@ 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); PyErr_Format(BadGzipFile, "Not a gzipped file (%R)", - PyBytes_FromStringAndSize((char *)current_pos, 2)); + magic_obj); + Py_DECREF(magic_obj); return -1; }; uint8_t method = current_pos[2]; From 9a27be448d79f8cafa20ccc27b64a58e9ef3d68f Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 27 Oct 2023 07:36:23 +0200 Subject: [PATCH 06/19] Also build with address sanitizer --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 338a177c..6eb1cd64 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,6 +34,7 @@ jobs: tox_env: - docs - twine_check + - asan runs-on: ubuntu-latest steps: - uses: actions/checkout@v2.3.4 From a259b509c43d3d4a42b2d9be6695ce97673119af Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 27 Oct 2023 07:40:12 +0200 Subject: [PATCH 07/19] Make sure libasan is installed --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6eb1cd64..6bd63f45 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,7 +45,7 @@ jobs: with: python-version: 3.8 - name: Install isal - run: sudo apt-get install libisal-dev + run: sudo apt-get install libisal-dev libasan8 - name: Install tox and upgrade setuptools and pip run: pip install --upgrade tox setuptools pip - name: Run tox -e ${{ matrix.tox_env }} From 0267edc2069a98e0c9d49796313e7b0ad18a4e3a Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 27 Oct 2023 07:50:55 +0200 Subject: [PATCH 08/19] Make test a bit more debuggable on the CI --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 7e74d40a..a42fde97 100644 --- a/tox.ini +++ b/tox.ini @@ -29,7 +29,7 @@ setenv= CFLAGS=-lasan -fsanitize=address -fno-omit-frame-pointer allowlist_externals=bash commands= - bash -c 'LD_PRELOAD=$(find /usr/lib -name "libasan.so*" | head -n 1) pytest tests' + bash -c 'export LD_PRELOAD=$(find /usr/lib -name "libasan.so*" | head -n 1) && printenv LD_PRELOAD && pytest tests' [testenv:compliance] deps=pytest From 42e5ba28d5de7167290774003e1931c24928e3e7 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 27 Oct 2023 07:55:20 +0200 Subject: [PATCH 09/19] First import in order to check errors --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index a42fde97..52f46a39 100644 --- a/tox.ini +++ b/tox.ini @@ -29,7 +29,7 @@ setenv= CFLAGS=-lasan -fsanitize=address -fno-omit-frame-pointer allowlist_externals=bash commands= - bash -c 'export LD_PRELOAD=$(find /usr/lib -name "libasan.so*" | head -n 1) && printenv LD_PRELOAD && pytest tests' + bash -c 'export LD_PRELOAD=$(find /usr/lib -name "libasan.so*" | head -n 1) && printenv LD_PRELOAD && python -c "from isal import isal_zlib" && pytest tests' [testenv:compliance] deps=pytest From fe6d7596f9617efdaf481ca749ecdda67158a533 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 27 Oct 2023 08:02:42 +0200 Subject: [PATCH 10/19] Separate asan structure --- .github/workflows/ci.yml | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6bd63f45..8f6a3851 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,7 +34,6 @@ jobs: tox_env: - docs - twine_check - - asan runs-on: ubuntu-latest steps: - uses: actions/checkout@v2.3.4 @@ -45,7 +44,7 @@ jobs: with: python-version: 3.8 - name: Install isal - run: sudo apt-get install libisal-dev libasan8 + run: sudo apt-get install libisal-dev - name: Install tox and upgrade setuptools and pip run: pip install --upgrade tox setuptools pip - name: Run tox -e ${{ matrix.tox_env }} @@ -100,6 +99,23 @@ jobs: - name: Upload coverage report uses: codecov/codecov-action@v1 + test-asan: + runs-on: "ubuntu-latest" + steps: + - uses: actions/checkout@v2.3.4 + with: + submodules: recursive + - name: Set up Python 3.8 + uses: actions/setup-python@v2.2.1 + with: + python-version: "3.8" + - name: Install tox and upgrade setuptools + run: pip install --upgrade tox setuptools + - name: Install build dependencies (Linux) # Yasm in pypa/manylinux images. + run: sudo apt install nasm + - name: Run asan tests + run: tox -e asan + test-arch: if: startsWith(github.ref, 'refs/tags') || github.ref == 'refs/heads/develop' || github.ref == 'refs/heads/main' runs-on: "ubuntu-latest" From 3fbc980f409dae111fc97c0a8f90244fe2105e6f Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 27 Oct 2023 08:06:36 +0200 Subject: [PATCH 11/19] Revert github ci changes --- .github/workflows/ci.yml | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8f6a3851..338a177c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -99,23 +99,6 @@ jobs: - name: Upload coverage report uses: codecov/codecov-action@v1 - test-asan: - runs-on: "ubuntu-latest" - steps: - - uses: actions/checkout@v2.3.4 - with: - submodules: recursive - - name: Set up Python 3.8 - uses: actions/setup-python@v2.2.1 - with: - python-version: "3.8" - - name: Install tox and upgrade setuptools - run: pip install --upgrade tox setuptools - - name: Install build dependencies (Linux) # Yasm in pypa/manylinux images. - run: sudo apt install nasm - - name: Run asan tests - run: tox -e asan - test-arch: if: startsWith(github.ref, 'refs/tags') || github.ref == 'refs/heads/develop' || github.ref == 'refs/heads/main' runs-on: "ubuntu-latest" From 114d4f4d762bc5d9aa39ab5ce1ea7247c1bb0f58 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 27 Oct 2023 11:39:17 +0200 Subject: [PATCH 12/19] Make ASAN checks of release procedure --- .github/release_checklist.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/release_checklist.md b/.github/release_checklist.md index c47123ba..7093fe46 100644 --- a/.github/release_checklist.md +++ b/.github/release_checklist.md @@ -5,6 +5,7 @@ Release checklist - [ ] Set version to a stable number. - [ ] Change current development version in `CHANGELOG.rst` to stable version. - [ ] Change the version in `__init__.py` +- [ ] Check if the address sanitizer does not find any problems using `tox -e asan` - [ ] Merge the release branch into `main`. - [ ] Created an annotated tag with the stable version number. Include changes from CHANGELOG.rst. From bab10a9f952afae8d6dbba78d745b3b18823ca36 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 27 Oct 2023 11:41:20 +0200 Subject: [PATCH 13/19] Add memory leak fixes to changelog --- CHANGELOG.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0f1eaff3..d51e515b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,6 +7,13 @@ Changelog .. This document is user facing. Please word the changes in such a way .. that users understand how the changes affect the new version. +version 1.6.0-dev +----------------- ++ Fix a memory leak that occurred when an error was thrown for a gzip header + with the wrong magic numbers. ++ Fix a memory leak that occurred when isal_zlib.decompressobj was given a + wrong wbits value. + version 1.5.1 ----------------- + Fix a memory leak in the GzipReader.readall implementation. From e6778171988fdca1b40d14afafc8cbee7b0f0461 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 27 Oct 2023 17:04:24 +0200 Subject: [PATCH 14/19] Use gcc command rather than find --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 52f46a39..256ee95e 100644 --- a/tox.ini +++ b/tox.ini @@ -29,7 +29,7 @@ setenv= CFLAGS=-lasan -fsanitize=address -fno-omit-frame-pointer allowlist_externals=bash commands= - bash -c 'export LD_PRELOAD=$(find /usr/lib -name "libasan.so*" | head -n 1) && printenv LD_PRELOAD && python -c "from isal import isal_zlib" && pytest tests' + bash -c 'export LD_PRELOAD=$(gcc -print-file-name=libasan.so) && printenv LD_PRELOAD && python -c "from isal import isal_zlib" && pytest tests' [testenv:compliance] deps=pytest From c84f90d3302db1128a8dd651afa9de96f16c0e1c Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 3 Nov 2023 15:58:52 +0100 Subject: [PATCH 15/19] Reproduce resourcewarning --- tests/test_igzip_threaded.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/test_igzip_threaded.py b/tests/test_igzip_threaded.py index a0f581c6..bd1282c2 100644 --- a/tests/test_igzip_threaded.py +++ b/tests/test_igzip_threaded.py @@ -139,10 +139,11 @@ def test_writer_not_readable(): def test_writer_wrong_level(): - with pytest.raises(ValueError) as error: - igzip_threaded._ThreadedGzipWriter(io.BytesIO(), level=42) - error.match("Invalid compression level") - error.match("42") + with tempfile.NamedTemporaryFile("wb") as tmp: + with pytest.raises(ValueError) as error: + igzip_threaded.open(tmp.name, mode="wb", compresslevel=42) + error.match("Invalid compression level") + error.match("42") def test_writer_too_low_threads(): From 54ebf53379c346b2bbec5cb4cfcd94eb5749d5cf Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 3 Nov 2023 16:03:58 +0100 Subject: [PATCH 16/19] Prevent unclosed filehandles by moving opening logic after other tests --- src/isal/igzip_threaded.py | 30 +++++++++++++++++------------- tests/test_igzip_threaded.py | 2 +- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/isal/igzip_threaded.py b/src/isal/igzip_threaded.py index 20142e7a..b42a8186 100644 --- a/src/isal/igzip_threaded.py +++ b/src/isal/igzip_threaded.py @@ -57,19 +57,13 @@ def open(filename, mode="rb", compresslevel=igzip._COMPRESS_LEVEL_TRADEOFF, except: # noqa: E722 threads = 1 open_mode = mode.replace("t", "b") - if isinstance(filename, (str, bytes)) or hasattr(filename, "__fspath__"): - binary_file = builtins.open(filename, open_mode) - elif hasattr(filename, "read") or hasattr(filename, "write"): - binary_file = filename - else: - raise TypeError("filename must be a str or bytes object, or a file") if "r" in mode: gzip_file = io.BufferedReader( - _ThreadedGzipReader(binary_file, block_size=block_size)) + _ThreadedGzipReader(filename, block_size=block_size)) else: gzip_file = io.BufferedWriter( _ThreadedGzipWriter( - fp=binary_file, + filename, block_size=block_size, level=compresslevel, threads=threads @@ -81,10 +75,20 @@ def open(filename, mode="rb", compresslevel=igzip._COMPRESS_LEVEL_TRADEOFF, return gzip_file +def open_as_binary_stream(filename, open_mode): + if isinstance(filename, (str, bytes)) or hasattr(filename, "__fspath__"): + binary_file = builtins.open(filename, open_mode) + elif hasattr(filename, "read") or hasattr(filename, "write"): + binary_file = filename + else: + raise TypeError("filename must be a str or bytes object, or a file") + return binary_file + + class _ThreadedGzipReader(io.RawIOBase): - def __init__(self, fp, queue_size=2, block_size=1024 * 1024): - self.raw = fp - self.fileobj = igzip._IGzipReader(fp, buffersize=8 * block_size) + def __init__(self, filename, queue_size=2, block_size=1024 * 1024): + self.raw = open_as_binary_stream(filename, "rb") + self.fileobj = igzip._IGzipReader(self.raw, buffersize=8 * block_size) self.pos = 0 self.read_file = False self.queue = queue.Queue(queue_size) @@ -193,7 +197,7 @@ class _ThreadedGzipWriter(io.RawIOBase): compressing and output is handled in one thread. """ def __init__(self, - fp: BinaryIO, + filename, level: int = isal_zlib.ISAL_DEFAULT_COMPRESSION, threads: int = 1, queue_size: int = 1, @@ -201,7 +205,6 @@ def __init__(self, ): self.lock = threading.Lock() self.exception: Optional[Exception] = None - self.raw = fp self.level = level self.previous_block = b"" # Deflating random data results in an output a little larger than the @@ -236,6 +239,7 @@ def __init__(self, self.running = False self._size = 0 self._closed = False + self.raw = open_as_binary_stream(filename, "wb") self._write_gzip_header() self.start() diff --git a/tests/test_igzip_threaded.py b/tests/test_igzip_threaded.py index bd1282c2..be6c3ca4 100644 --- a/tests/test_igzip_threaded.py +++ b/tests/test_igzip_threaded.py @@ -99,7 +99,7 @@ def test_threaded_write_oversized_block_no_error(threads): @pytest.mark.parametrize("threads", [1, 3]) def test_threaded_write_error(threads): f = igzip_threaded._ThreadedGzipWriter( - fp=io.BytesIO(), level=3, + io.BytesIO(), level=3, threads=threads, block_size=8 * 1024) # Bypass the write method which should not allow blocks larger than # block_size. From fbe1eb300876e566c50cd9804b0b8900c6b1ab40 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 3 Nov 2023 16:04:43 +0100 Subject: [PATCH 17/19] Fix linter issues --- src/isal/igzip_threaded.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/isal/igzip_threaded.py b/src/isal/igzip_threaded.py index b42a8186..dd34e704 100644 --- a/src/isal/igzip_threaded.py +++ b/src/isal/igzip_threaded.py @@ -12,7 +12,7 @@ import queue import struct import threading -from typing import BinaryIO, List, Optional, Tuple +from typing import List, Optional, Tuple from . import igzip, isal_zlib @@ -56,7 +56,6 @@ def open(filename, mode="rb", compresslevel=igzip._COMPRESS_LEVEL_TRADEOFF, threads = multiprocessing.cpu_count() except: # noqa: E722 threads = 1 - open_mode = mode.replace("t", "b") if "r" in mode: gzip_file = io.BufferedReader( _ThreadedGzipReader(filename, block_size=block_size)) From 27f3f7bdb8b2edbb5841d34fd0aa1ca3dadbe26d Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 3 Nov 2023 16:09:04 +0100 Subject: [PATCH 18/19] Add bugfix to changelog --- CHANGELOG.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d51e515b..65eab10e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -9,6 +9,8 @@ Changelog version 1.6.0-dev ----------------- ++ Fix a bug where a filehandle remained opened when ``igzip_threaded.open`` + was used for writing with a wrong compression level. + Fix a memory leak that occurred when an error was thrown for a gzip header with the wrong magic numbers. + Fix a memory leak that occurred when isal_zlib.decompressobj was given a From b158a268ea96f512938c472acadd053d5c12e065 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Fri, 3 Nov 2023 16:30:58 +0100 Subject: [PATCH 19/19] Set stable version --- CHANGELOG.rst | 2 +- setup.py | 2 +- src/isal/__init__.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 65eab10e..3e936f62 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,7 +7,7 @@ Changelog .. This document is user facing. Please word the changes in such a way .. that users understand how the changes affect the new version. -version 1.6.0-dev +version 1.5.2 ----------------- + Fix a bug where a filehandle remained opened when ``igzip_threaded.open`` was used for writing with a wrong compression level. diff --git a/setup.py b/setup.py index 1cb13991..22c16742 100644 --- a/setup.py +++ b/setup.py @@ -135,7 +135,7 @@ def build_isa_l(): setup( name="isal", - version="1.6.0-dev", + version="1.5.2", description="Faster zlib and gzip compatible compression and " "decompression by providing python bindings for the ISA-L " "library.", diff --git a/src/isal/__init__.py b/src/isal/__init__.py index f7257098..8a39a81a 100644 --- a/src/isal/__init__.py +++ b/src/isal/__init__.py @@ -27,4 +27,4 @@ "__version__" ] -__version__ = "1.6.0-dev" +__version__ = "1.5.2"