Skip to content

Commit 23a5ba1

Browse files
authored
gh-160549: Fix crash when freeing a busy multi-threaded ZstdCompressor (GH-160550)
ZSTD_freeCCtx() frees a dictionary loaded with ZSTD_CCtx_loadDictionary() before it stops the worker threads. Make libzstd wait for unfinished jobs before freeing the compression context of a ZstdCompressor that uses worker threads and a ZstdDict. Works around facebook/zstd#4862
1 parent b949033 commit 23a5ba1

3 files changed

Lines changed: 77 additions & 0 deletions

File tree

‎Lib/test/test_zstd.py‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,6 +371,58 @@ def test_zstd_multithread_compress(self):
371371
with ZstdFile(io.BytesIO(), 'w', options=options) as f:
372372
f.write(b)
373373

374+
@unittest.skipIf(not SUPPORT_MULTITHREADING,
375+
"zstd build doesn't support multi-threaded compression")
376+
def test_free_busy_multithread_compressor_with_dict(self):
377+
# A compressor can be freed while a worker thread is still
378+
# compressing with a dictionary that the compression context owns.
379+
# Whether a failure is detected depends on the memory allocator,
380+
# so use a fresh process for each case.
381+
code = textwrap.dedent("""
382+
import gc, random, sys
383+
from compression.zstd import (CompressionParameter,
384+
ZstdCompressor, ZstdDict)
385+
386+
job_size = 1 << 20
387+
zd = ZstdDict(random.randbytes(1 << 10) * (1 << 10), is_raw=True)
388+
options = {CompressionParameter.compression_level: 10,
389+
CompressionParameter.nb_workers: 1,
390+
CompressionParameter.job_size: job_size}
391+
c = ZstdCompressor(options=options, zstd_dict=zd)
392+
# Returns while the worker thread is compressing the first job.
393+
c.compress(random.randbytes(1 << 10) * ((job_size >> 10) + 64))
394+
if sys.argv[1] == 'error':
395+
# A failed call does not wait for the worker thread either.
396+
import _testcapi
397+
_testcapi.set_nomemory(0)
398+
try:
399+
c.compress(b'x')
400+
except MemoryError:
401+
pass
402+
finally:
403+
_testcapi.remove_mem_hooks()
404+
if sys.argv[1] == 'gc':
405+
# The garbage collector clears the compressor before
406+
# deallocating it.
407+
cycle = [c]
408+
cycle.append(cycle)
409+
del c, cycle
410+
gc.collect()
411+
elif sys.argv[1] != 'exit':
412+
del c
413+
# Otherwise the compressor is freed at interpreter exit.
414+
""")
415+
cases = ['del', 'gc', 'exit']
416+
try:
417+
import _testcapi # noqa: F401
418+
except ImportError:
419+
pass
420+
else:
421+
cases.append('error')
422+
for case in cases:
423+
with self.subTest(case=case):
424+
assert_python_ok('-c', code, case)
425+
374426
def test_compress_flushblock(self):
375427
point = len(THIS_FILE_BYTES) // 2
376428

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix a crash when a :class:`compression.zstd.ZstdCompressor` that uses worker
2+
threads and a :class:`~compression.zstd.ZstdDict` is deallocated, or the
3+
interpreter exits, while data passed to
4+
:meth:`~compression.zstd.ZstdCompressor.compress` is still being compressed.

‎Modules/_zstd/compressor.c‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ typedef struct {
3636
/* (nbWorker >= 1) ? 1 : 0 */
3737
int use_multithread;
3838

39+
/* Nonzero if worker threads may still be compressing submitted data:
40+
set by any call that did not complete a frame, including a failed one */
41+
int mt_maybe_busy;
42+
3943
/* Compression level */
4044
int compression_level;
4145

@@ -347,6 +351,7 @@ _zstd_ZstdCompressor_new_impl(PyTypeObject *type, PyObject *level,
347351
}
348352

349353
self->use_multithread = 0;
354+
self->mt_maybe_busy = 0;
350355
self->compression_level = ZSTD_CLEVEL_DEFAULT;
351356
self->dict = NULL;
352357
self->lock = (PyMutex){0};
@@ -427,6 +432,18 @@ ZstdCompressor_dealloc(PyObject *ob)
427432

428433
/* Free compression context */
429434
if (self->cctx) {
435+
if (self->mt_maybe_busy) {
436+
/* ZSTD_freeCCtx() (as of libzstd 1.5.7) frees the context's own
437+
copy of a dictionary loaded with ZSTD_CCtx_loadDictionary()
438+
before it stops the worker threads, which may still be
439+
reading it. Starting a new frame makes libzstd wait for the
440+
jobs of the previous one, and with no input it starts none. */
441+
ZSTD_inBuffer in = {NULL, 0, 0};
442+
ZSTD_outBuffer out = {NULL, 0, 0};
443+
(void)ZSTD_CCtx_reset(self->cctx, ZSTD_reset_session_only);
444+
(void)ZSTD_compressStream2(self->cctx, &out, &in,
445+
ZSTD_e_continue);
446+
}
430447
ZSTD_freeCCtx(self->cctx);
431448
}
432449

@@ -637,6 +654,8 @@ _zstd_ZstdCompressor_compress_impl(ZstdCompressor *self, Py_buffer *data,
637654
/* Resetting cctx's session never fail */
638655
ZSTD_CCtx_reset(self->cctx, ZSTD_reset_session_only);
639656
}
657+
self->mt_maybe_busy = (self->use_multithread
658+
&& (ret == NULL || mode != ZSTD_e_end));
640659
PyMutex_Unlock(&self->lock);
641660

642661
return ret;
@@ -685,6 +704,8 @@ _zstd_ZstdCompressor_flush_impl(ZstdCompressor *self, int mode)
685704
/* Resetting cctx's session never fail */
686705
ZSTD_CCtx_reset(self->cctx, ZSTD_reset_session_only);
687706
}
707+
self->mt_maybe_busy = (self->use_multithread
708+
&& (ret == NULL || mode != ZSTD_e_end));
688709
PyMutex_Unlock(&self->lock);
689710

690711
return ret;

0 commit comments

Comments
 (0)