From ea7b5c5b667b0bc80a70f9ca4ee123e82450c123 Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Tue, 16 Jul 2024 10:13:26 +0200 Subject: [PATCH 1/7] Lock around usages of imaging memory arenas --- src/_imaging.c | 48 +++++++++++++++++++++++++++++++--------- src/libImaging/Imaging.h | 12 ++++++++++ src/libImaging/Storage.c | 20 ++++++++++++----- 3 files changed, 65 insertions(+), 15 deletions(-) diff --git a/src/_imaging.c b/src/_imaging.c index ac6310a445e..66709d9bc66 100644 --- a/src/_imaging.c +++ b/src/_imaging.c @@ -92,6 +92,7 @@ #define _USE_MATH_DEFINES #include +#include /* Configuration stuff. Feel free to undef things you don't need. */ #define WITH_IMAGECHOPS /* ImageChops support */ @@ -3934,7 +3935,6 @@ static PyObject * _get_stats(PyObject *self, PyObject *args) { PyObject *d; PyObject *v; - ImagingMemoryArena arena = &ImagingDefaultArena; if (!PyArg_ParseTuple(args, ":get_stats")) { return NULL; @@ -3944,6 +3944,10 @@ _get_stats(PyObject *self, PyObject *args) { if (!d) { return NULL; } + + MUTEX_LOCK(&ImagingDefaultArena.mutex); + ImagingMemoryArena arena = &ImagingDefaultArena; + v = PyLong_FromLong(arena->stats_new_count); PyDict_SetItemString(d, "new_count", v ? v : Py_None); Py_XDECREF(v); @@ -3967,22 +3971,25 @@ _get_stats(PyObject *self, PyObject *args) { v = PyLong_FromLong(arena->blocks_cached); PyDict_SetItemString(d, "blocks_cached", v ? v : Py_None); Py_XDECREF(v); + + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); return d; } static PyObject * _reset_stats(PyObject *self, PyObject *args) { - ImagingMemoryArena arena = &ImagingDefaultArena; - if (!PyArg_ParseTuple(args, ":reset_stats")) { return NULL; } + MUTEX_LOCK(&ImagingDefaultArena.mutex); + ImagingMemoryArena arena = &ImagingDefaultArena; arena->stats_new_count = 0; arena->stats_allocated_blocks = 0; arena->stats_reused_blocks = 0; arena->stats_reallocated_blocks = 0; arena->stats_freed_blocks = 0; + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); Py_INCREF(Py_None); return Py_None; @@ -3994,7 +4001,10 @@ _get_alignment(PyObject *self, PyObject *args) { return NULL; } - return PyLong_FromLong(ImagingDefaultArena.alignment); + MUTEX_LOCK(&ImagingDefaultArena.mutex); + int alignment = ImagingDefaultArena.alignment; + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); + return PyLong_FromLong(alignment); } static PyObject * @@ -4003,7 +4013,10 @@ _get_block_size(PyObject *self, PyObject *args) { return NULL; } - return PyLong_FromLong(ImagingDefaultArena.block_size); + MUTEX_LOCK(&ImagingDefaultArena.mutex); + int block_size = ImagingDefaultArena.block_size; + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); + return PyLong_FromLong(block_size); } static PyObject * @@ -4012,7 +4025,10 @@ _get_blocks_max(PyObject *self, PyObject *args) { return NULL; } - return PyLong_FromLong(ImagingDefaultArena.blocks_max); + MUTEX_LOCK(&ImagingDefaultArena.mutex); + int blocks_max = ImagingDefaultArena.blocks_max; + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); + return PyLong_FromLong(blocks_max); } static PyObject * @@ -4032,7 +4048,9 @@ _set_alignment(PyObject *self, PyObject *args) { return NULL; } + MUTEX_LOCK(&ImagingDefaultArena.mutex); ImagingDefaultArena.alignment = alignment; + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); Py_INCREF(Py_None); return Py_None; @@ -4055,7 +4073,9 @@ _set_block_size(PyObject *self, PyObject *args) { return NULL; } + MUTEX_LOCK(&ImagingDefaultArena.mutex); ImagingDefaultArena.block_size = block_size; + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); Py_INCREF(Py_None); return Py_None; @@ -4071,14 +4091,20 @@ _set_blocks_max(PyObject *self, PyObject *args) { if (blocks_max < 0) { PyErr_SetString(PyExc_ValueError, "blocks_max should be greater than 0"); return NULL; - } else if ( - (unsigned long)blocks_max > - SIZE_MAX / sizeof(ImagingDefaultArena.blocks_pool[0])) { + } + + MUTEX_LOCK(&ImagingDefaultArena.mutex); + size_t blocksize = sizeof(ImagingDefaultArena.blocks_pool[0]); + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); + if ((unsigned long)blocks_max > SIZE_MAX / blocksize) { PyErr_SetString(PyExc_ValueError, "blocks_max is too large"); return NULL; } - if (!ImagingMemorySetBlocksMax(&ImagingDefaultArena, blocks_max)) { + MUTEX_LOCK(&ImagingDefaultArena.mutex); + int status = ImagingMemorySetBlocksMax(&ImagingDefaultArena, blocks_max); + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); + if (!status) { return ImagingError_MemoryError(); } @@ -4094,7 +4120,9 @@ _clear_cache(PyObject *self, PyObject *args) { return NULL; } + MUTEX_LOCK(&ImagingDefaultArena.mutex); ImagingMemoryClearCache(&ImagingDefaultArena, i); + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); Py_INCREF(Py_None); return Py_None; diff --git a/src/libImaging/Imaging.h b/src/libImaging/Imaging.h index 1f2c03e934f..28311121607 100644 --- a/src/libImaging/Imaging.h +++ b/src/libImaging/Imaging.h @@ -161,6 +161,9 @@ typedef struct ImagingMemoryArena { int stats_reallocated_blocks; /* Number of blocks which were actually reallocated after retrieving */ int stats_freed_blocks; /* Number of freed blocks */ +#ifdef Py_GIL_DISABLED + PyMutex mutex; +#endif } *ImagingMemoryArena; /* Objects */ @@ -690,6 +693,15 @@ _imaging_tell_pyFd(PyObject *fd); #include "ImagingUtils.h" extern UINT8 *clip8_lookups; +/* Mutex lock/unlock helpers */ +#ifdef Py_GIL_DISABLED +#define MUTEX_LOCK(m) PyMutex_Lock(m) +#define MUTEX_UNLOCK(m) PyMutex_Unlock(m) +#else +#define MUTEX_LOCK(m) +#define MUTEX_UNLOCK(m) +#endif + #if defined(__cplusplus) } #endif diff --git a/src/libImaging/Storage.c b/src/libImaging/Storage.c index b27195a3587..9331ed5d538 100644 --- a/src/libImaging/Storage.c +++ b/src/libImaging/Storage.c @@ -219,7 +219,9 @@ ImagingNewPrologueSubtype(const char *mode, int xsize, int ysize, int size) { break; } + MUTEX_LOCK(&ImagingDefaultArena.mutex); ImagingDefaultArena.stats_new_count += 1; + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); return im; } @@ -267,7 +269,8 @@ struct ImagingMemoryArena ImagingDefaultArena = { 0, 0, 0, - 0 // Stats + 0, // Stats + {0}, }; int @@ -365,7 +368,9 @@ ImagingDestroyArray(Imaging im) { if (im->blocks) { while (im->blocks[y].ptr) { + MUTEX_LOCK(&ImagingDefaultArena.mutex); memory_return_block(&ImagingDefaultArena, im->blocks[y]); + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); y += 1; } free(im->blocks); @@ -373,9 +378,8 @@ ImagingDestroyArray(Imaging im) { } Imaging -ImagingAllocateArray(Imaging im, int dirty, int block_size) { +ImagingAllocateArray(Imaging im, ImagingMemoryArena arena, int dirty, int block_size) { int y, line_in_block, current_block; - ImagingMemoryArena arena = &ImagingDefaultArena; ImagingMemoryBlock block = {NULL, 0}; int aligned_linesize, lines_per_block, blocks_count; char *aligned_ptr = NULL; @@ -498,14 +502,20 @@ ImagingNewInternal(const char *mode, int xsize, int ysize, int dirty) { return NULL; } - if (ImagingAllocateArray(im, dirty, ImagingDefaultArena.block_size)) { + MUTEX_LOCK(&ImagingDefaultArena.mutex); + Imaging tmp = ImagingAllocateArray(im, &ImagingDefaultArena, dirty, ImagingDefaultArena.block_size); + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); + if (tmp) { return im; } ImagingError_Clear(); // Try to allocate the image once more with smallest possible block size - if (ImagingAllocateArray(im, dirty, IMAGING_PAGE_SIZE)) { + MUTEX_LOCK(&ImagingDefaultArena.mutex); + tmp = ImagingAllocateArray(im, &ImagingDefaultArena, dirty, IMAGING_PAGE_SIZE); + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); + if (tmp) { return im; } From 9f110aa702b0faef7f1ecc218fce3984d89b389a Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 16 Jul 2024 14:47:12 +0000 Subject: [PATCH 2/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/libImaging/Storage.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libImaging/Storage.c b/src/libImaging/Storage.c index 9331ed5d538..8b6d46da6a6 100644 --- a/src/libImaging/Storage.c +++ b/src/libImaging/Storage.c @@ -503,7 +503,8 @@ ImagingNewInternal(const char *mode, int xsize, int ysize, int dirty) { } MUTEX_LOCK(&ImagingDefaultArena.mutex); - Imaging tmp = ImagingAllocateArray(im, &ImagingDefaultArena, dirty, ImagingDefaultArena.block_size); + Imaging tmp = ImagingAllocateArray( + im, &ImagingDefaultArena, dirty, ImagingDefaultArena.block_size); MUTEX_UNLOCK(&ImagingDefaultArena.mutex); if (tmp) { return im; From 5999b9b0ccefe916c364490d7de86a71ea383b96 Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Tue, 16 Jul 2024 16:56:15 +0200 Subject: [PATCH 3/7] Initialize PyMutex only under the free-threaded build --- src/libImaging/Storage.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libImaging/Storage.c b/src/libImaging/Storage.c index 8b6d46da6a6..3a49113b62b 100644 --- a/src/libImaging/Storage.c +++ b/src/libImaging/Storage.c @@ -270,7 +270,9 @@ struct ImagingMemoryArena ImagingDefaultArena = { 0, 0, 0, // Stats +#ifdef Py_GIL_DISABLED {0}, +#endif }; int From 06767fc3257fdd7932c2c04d7bc83cc26a0899a5 Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Tue, 16 Jul 2024 17:00:14 +0200 Subject: [PATCH 4/7] Address feedback; do not lock in a loop --- src/libImaging/Storage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libImaging/Storage.c b/src/libImaging/Storage.c index 3a49113b62b..a680fe63cd6 100644 --- a/src/libImaging/Storage.c +++ b/src/libImaging/Storage.c @@ -369,12 +369,12 @@ ImagingDestroyArray(Imaging im) { int y = 0; if (im->blocks) { + MUTEX_LOCK(&ImagingDefaultArena.mutex); while (im->blocks[y].ptr) { - MUTEX_LOCK(&ImagingDefaultArena.mutex); memory_return_block(&ImagingDefaultArena, im->blocks[y]); - MUTEX_UNLOCK(&ImagingDefaultArena.mutex); y += 1; } + MUTEX_UNLOCK(&ImagingDefaultArena.mutex); free(im->blocks); } } From 98b173928ad434d53c0e9ced859fcf1f14cf8e8f Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Tue, 16 Jul 2024 21:31:29 +0200 Subject: [PATCH 5/7] Address more feedback; don't unlock around sizeof --- src/_imaging.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/_imaging.c b/src/_imaging.c index 66709d9bc66..15520040f27 100644 --- a/src/_imaging.c +++ b/src/_imaging.c @@ -4093,10 +4093,8 @@ _set_blocks_max(PyObject *self, PyObject *args) { return NULL; } - MUTEX_LOCK(&ImagingDefaultArena.mutex); - size_t blocksize = sizeof(ImagingDefaultArena.blocks_pool[0]); - MUTEX_UNLOCK(&ImagingDefaultArena.mutex); - if ((unsigned long)blocks_max > SIZE_MAX / blocksize) { + if ((unsigned long)blocks_max > SIZE_MAX / + sizeof(ImagingDefaultArena.blocks_pool[0])) { PyErr_SetString(PyExc_ValueError, "blocks_max is too large"); return NULL; } From e144707520f2db6256927028cbaee02f8337b73d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 16 Jul 2024 19:31:59 +0000 Subject: [PATCH 6/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/_imaging.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/_imaging.c b/src/_imaging.c index 15520040f27..f1071b1ceab 100644 --- a/src/_imaging.c +++ b/src/_imaging.c @@ -4093,8 +4093,8 @@ _set_blocks_max(PyObject *self, PyObject *args) { return NULL; } - if ((unsigned long)blocks_max > SIZE_MAX / - sizeof(ImagingDefaultArena.blocks_pool[0])) { + if ((unsigned long)blocks_max > + SIZE_MAX / sizeof(ImagingDefaultArena.blocks_pool[0])) { PyErr_SetString(PyExc_ValueError, "blocks_max is too large"); return NULL; } From aa8d87696b27068a0e8f4303d7e631e00c3a5225 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 19 Jul 2024 12:14:51 +0000 Subject: [PATCH 7/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/_imaging.c | 2 +- src/libImaging/Storage.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/_imaging.c b/src/_imaging.c index 21c6b0a02e9..a2606df156e 100644 --- a/src/_imaging.c +++ b/src/_imaging.c @@ -4131,7 +4131,7 @@ _set_blocks_max(PyObject *self, PyObject *args) { } if ((unsigned long)blocks_max > - SIZE_MAX / sizeof(ImagingDefaultArena.blocks_pool[0])) { + SIZE_MAX / sizeof(ImagingDefaultArena.blocks_pool[0])) { PyErr_SetString(PyExc_ValueError, "blocks_max is too large"); return NULL; } diff --git a/src/libImaging/Storage.c b/src/libImaging/Storage.c index ae4bf72fffe..522e9f37557 100644 --- a/src/libImaging/Storage.c +++ b/src/libImaging/Storage.c @@ -506,7 +506,8 @@ ImagingNewInternal(const char *mode, int xsize, int ysize, int dirty) { MUTEX_LOCK(&ImagingDefaultArena.mutex); Imaging tmp = ImagingAllocateArray( - im, &ImagingDefaultArena, dirty, ImagingDefaultArena.block_size); + im, &ImagingDefaultArena, dirty, ImagingDefaultArena.block_size + ); MUTEX_UNLOCK(&ImagingDefaultArena.mutex); if (tmp) { return im;