From 79bd456db009419abe4fe6dc05296f039a33e27d Mon Sep 17 00:00:00 2001 From: Alexander Date: Sat, 5 Aug 2017 18:54:27 +0300 Subject: [PATCH 1/9] no reasons to release GIL for one calloc --- libImaging/Storage.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/libImaging/Storage.c b/libImaging/Storage.c index fb27572d3..ac9c6b2de 100644 --- a/libImaging/Storage.c +++ b/libImaging/Storage.c @@ -46,11 +46,9 @@ int ImagingNewCount = 0; */ Imaging -ImagingNewPrologueSubtype(const char *mode, int xsize, int ysize, - int size) +ImagingNewPrologueSubtype(const char *mode, int xsize, int ysize, int size) { Imaging im; - ImagingSectionCookie cookie; im = (Imaging) calloc(1, size); if (!im) @@ -212,14 +210,10 @@ ImagingNewPrologueSubtype(const char *mode, int xsize, int ysize, /* Setup image descriptor */ strcpy(im->mode, mode); - ImagingSectionEnter(&cookie); - /* Pointer array (allocate at least one line, to avoid MemoryError exceptions on platforms where calloc(0, x) returns NULL) */ im->image = (char **) calloc((ysize > 0) ? ysize : 1, sizeof(void *)); - ImagingSectionLeave(&cookie); - if (!im->image) { free(im); return (Imaging) ImagingError_MemoryError(); From 7c67911ea48b4cdaab3fda171bd40e99606b53a5 Mon Sep 17 00:00:00 2001 From: Alexander Date: Sat, 5 Aug 2017 18:59:16 +0300 Subject: [PATCH 2/9] check args before allocate memory --- libImaging/Storage.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/libImaging/Storage.c b/libImaging/Storage.c index ac9c6b2de..e5f620363 100644 --- a/libImaging/Storage.c +++ b/libImaging/Storage.c @@ -50,15 +50,16 @@ ImagingNewPrologueSubtype(const char *mode, int xsize, int ysize, int size) { Imaging im; - im = (Imaging) calloc(1, size); - if (!im) - return (Imaging) ImagingError_MemoryError(); - /* linesize overflow check, roughly the current largest space req'd */ if (xsize > (INT_MAX / 4) - 1) { return (Imaging) ImagingError_MemoryError(); } + im = (Imaging) calloc(1, size); + if (!im) { + return (Imaging) ImagingError_MemoryError(); + } + /* Setup image descriptor */ im->xsize = xsize; im->ysize = ysize; @@ -228,8 +229,7 @@ Imaging ImagingNewPrologue(const char *mode, int xsize, int ysize) { return ImagingNewPrologueSubtype( - mode, xsize, ysize, sizeof(struct ImagingMemoryInstance) - ); + mode, xsize, ysize, sizeof(struct ImagingMemoryInstance)); } Imaging @@ -371,7 +371,6 @@ ImagingNewBlock(const char *mode, int xsize, int ysize) } im->destroy = ImagingDestroyBlock; - } return ImagingNewEpilogue(im); From 7da62d5a972455cd85de9cc6bcca9a1e3876a2ab Mon Sep 17 00:00:00 2001 From: Alexander Date: Sat, 5 Aug 2017 20:25:57 +0300 Subject: [PATCH 3/9] FIX memory leak ImagingNewEpilogue now is always success The Imaging object itself is freed through ImagingDelete in case when memory is not allocated in ImagingNewBlock or ImagingNewArray --- libImaging/Imaging.h | 2 +- libImaging/Storage.c | 41 +++++++++++++++++++++-------------------- map.c | 6 ++---- 3 files changed, 24 insertions(+), 25 deletions(-) diff --git a/libImaging/Imaging.h b/libImaging/Imaging.h index 99fff7f67..086a7dc6d 100644 --- a/libImaging/Imaging.h +++ b/libImaging/Imaging.h @@ -172,7 +172,7 @@ extern Imaging ImagingNewPrologue(const char *mode, extern Imaging ImagingNewPrologueSubtype(const char *mode, int xsize, int ysize, int structure_size); -extern Imaging ImagingNewEpilogue(Imaging im); +extern void ImagingNewEpilogue(Imaging im); extern void ImagingCopyInfo(Imaging destination, Imaging source); diff --git a/libImaging/Storage.c b/libImaging/Storage.c index e5f620363..640211e33 100644 --- a/libImaging/Storage.c +++ b/libImaging/Storage.c @@ -232,15 +232,9 @@ ImagingNewPrologue(const char *mode, int xsize, int ysize) mode, xsize, ysize, sizeof(struct ImagingMemoryInstance)); } -Imaging +void ImagingNewEpilogue(Imaging im) { - /* If the raster data allocator didn't setup a destructor, - assume that it couldn't allocate the required amount of - memory. */ - if (!im->destroy) - return (Imaging) ImagingError_MemoryError(); - /* Initialize alias pointers to pixel data. */ switch (im->pixelsize) { case 1: case 2: case 3: @@ -250,8 +244,6 @@ ImagingNewEpilogue(Imaging im) im->image32 = (INT32 **) im->image; break; } - - return im; } void @@ -316,10 +308,15 @@ ImagingNewArray(const char *mode, int xsize, int ysize) ImagingSectionLeave(&cookie); - if (y == im->ysize) - im->destroy = ImagingDestroyArray; + if (y != im->ysize) { + ImagingDelete(im); + return (Imaging) ImagingError_MemoryError(); + } + + im->destroy = ImagingDestroyArray; + ImagingNewEpilogue(im); - return ImagingNewEpilogue(im); + return im; } @@ -364,16 +361,20 @@ ImagingNewBlock(const char *mode, int xsize, int ysize) im->block = (char *) calloc(im->ysize, im->linesize); } - if (im->block) { - for (y = i = 0; y < im->ysize; y++) { - im->image[y] = im->block + i; - i += im->linesize; - } - - im->destroy = ImagingDestroyBlock; + if ( ! im->block) { + ImagingDelete(im); + return (Imaging) ImagingError_MemoryError(); } - return ImagingNewEpilogue(im); + for (y = i = 0; y < im->ysize; y++) { + im->image[y] = im->block + i; + i += im->linesize; + } + + im->destroy = ImagingDestroyBlock; + ImagingNewEpilogue(im); + + return im; } /* -------------------------------------------------------------------- diff --git a/map.c b/map.c index 9d4751e31..8c36660d0 100644 --- a/map.c +++ b/map.c @@ -229,8 +229,7 @@ mapping_readimage(ImagingMapperObject* mapper, PyObject* args) im->destroy = ImagingDestroyMap; - if (!ImagingNewEpilogue(im)) - return NULL; + ImagingNewEpilogue(im); mapper->offset += size; @@ -387,8 +386,7 @@ PyImaging_MapBuffer(PyObject* self, PyObject* args) ((ImagingBufferInstance*) im)->target = target; ((ImagingBufferInstance*) im)->view = view; - if (!ImagingNewEpilogue(im)) - return NULL; + ImagingNewEpilogue(im); return PyImagingNew(im); } From 40676cb1fcdbbe568560770a487193a62504a995 Mon Sep 17 00:00:00 2001 From: Alexander Date: Sat, 5 Aug 2017 20:39:14 +0300 Subject: [PATCH 4/9] move ImagingNewEpilogue functionality to ImagingNewPrologueSubtype doublechecked: no im->image or im->image8 or im->image32 access between ImagingNewPrologue and ImagingNewEpilogue anywhere --- libImaging/Imaging.h | 1 - libImaging/Storage.c | 33 ++++++++++++++------------------- map.c | 7 +------ 3 files changed, 15 insertions(+), 26 deletions(-) diff --git a/libImaging/Imaging.h b/libImaging/Imaging.h index 086a7dc6d..ede35f74c 100644 --- a/libImaging/Imaging.h +++ b/libImaging/Imaging.h @@ -172,7 +172,6 @@ extern Imaging ImagingNewPrologue(const char *mode, extern Imaging ImagingNewPrologueSubtype(const char *mode, int xsize, int ysize, int structure_size); -extern void ImagingNewEpilogue(Imaging im); extern void ImagingCopyInfo(Imaging destination, Imaging source); diff --git a/libImaging/Storage.c b/libImaging/Storage.c index 640211e33..5cddfd66c 100644 --- a/libImaging/Storage.c +++ b/libImaging/Storage.c @@ -215,11 +215,21 @@ ImagingNewPrologueSubtype(const char *mode, int xsize, int ysize, int size) exceptions on platforms where calloc(0, x) returns NULL) */ im->image = (char **) calloc((ysize > 0) ? ysize : 1, sizeof(void *)); - if (!im->image) { + if ( ! im->image) { free(im); return (Imaging) ImagingError_MemoryError(); } + /* Initialize alias pointers to pixel data. */ + switch (im->pixelsize) { + case 1: case 2: case 3: + im->image8 = (UINT8 **) im->image; + break; + case 4: + im->image32 = (INT32 **) im->image; + break; + } + ImagingNewCount++; return im; @@ -232,20 +242,6 @@ ImagingNewPrologue(const char *mode, int xsize, int ysize) mode, xsize, ysize, sizeof(struct ImagingMemoryInstance)); } -void -ImagingNewEpilogue(Imaging im) -{ - /* Initialize alias pointers to pixel data. */ - switch (im->pixelsize) { - case 1: case 2: case 3: - im->image8 = (UINT8 **) im->image; - break; - case 4: - im->image32 = (INT32 **) im->image; - break; - } -} - void ImagingDelete(Imaging im) { @@ -312,9 +308,8 @@ ImagingNewArray(const char *mode, int xsize, int ysize) ImagingDelete(im); return (Imaging) ImagingError_MemoryError(); } - + im->destroy = ImagingDestroyArray; - ImagingNewEpilogue(im); return im; } @@ -338,8 +333,9 @@ ImagingNewBlock(const char *mode, int xsize, int ysize) Py_ssize_t y, i; im = ImagingNewPrologue(mode, xsize, ysize); - if (!im) + if ( ! im) { return NULL; + } /* We shouldn't overflow, since the threshold defined below says that we're only going to allocate max 4M @@ -372,7 +368,6 @@ ImagingNewBlock(const char *mode, int xsize, int ysize) } im->destroy = ImagingDestroyBlock; - ImagingNewEpilogue(im); return im; } diff --git a/map.c b/map.c index 8c36660d0..76b316012 100644 --- a/map.c +++ b/map.c @@ -229,8 +229,6 @@ mapping_readimage(ImagingMapperObject* mapper, PyObject* args) im->destroy = ImagingDestroyMap; - ImagingNewEpilogue(im); - mapper->offset += size; return PyImagingNew(im); @@ -367,8 +365,7 @@ PyImaging_MapBuffer(PyObject* self, PyObject* args) } im = ImagingNewPrologueSubtype( - mode, xsize, ysize, sizeof(ImagingBufferInstance) - ); + mode, xsize, ysize, sizeof(ImagingBufferInstance)); if (!im) return NULL; @@ -386,8 +383,6 @@ PyImaging_MapBuffer(PyObject* self, PyObject* args) ((ImagingBufferInstance*) im)->target = target; ((ImagingBufferInstance*) im)->view = view; - ImagingNewEpilogue(im); - return PyImagingNew(im); } From da453d085f1aef29f79a540294ad4c942cf129ba Mon Sep 17 00:00:00 2001 From: Alexander Date: Sat, 5 Aug 2017 21:58:31 +0300 Subject: [PATCH 5/9] add tests for Image.new modes --- Tests/test_image.py | 19 +++++++++++++++++++ libImaging/Storage.c | 7 ++----- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/Tests/test_image.py b/Tests/test_image.py index 1f9c4d798..a83b917a0 100644 --- a/Tests/test_image.py +++ b/Tests/test_image.py @@ -7,6 +7,25 @@ import sys class TestImage(PillowTestCase): + def test_image_modes_success(self): + for mode in [ + '1', 'P', 'PA', + 'L', 'LA', 'La', + 'F', 'I', 'I;16', 'I;16L', 'I;16B', 'I;16N', + 'RGB', 'RGBX', 'RGBA', 'RGBa', + 'CMYK', 'YCbCr', 'LAB', 'HSV', + ]: + Image.new(mode, (1, 1)) + + def test_image_modes_fail(self): + for mode in [ + '', 'bad', 'very very long', + 'BGR;15', 'BGR;16', 'BGR;24', 'BGR;32' + ]: + with self.assertRaises(ValueError) as e: + Image.new(mode, (1, 1)); + self.assertEqual(e.exception.message, 'unrecognized image mode') + def test_sanity(self): im = Image.new("L", (100, 100)) diff --git a/libImaging/Storage.c b/libImaging/Storage.c index 5cddfd66c..6acda2af0 100644 --- a/libImaging/Storage.c +++ b/libImaging/Storage.c @@ -205,7 +205,7 @@ ImagingNewPrologueSubtype(const char *mode, int xsize, int ysize, int size) } else { free(im); - return (Imaging) ImagingError_ValueError("unrecognized mode"); + return (Imaging) ImagingError_ValueError("unrecognized image mode"); } /* Setup image descriptor */ @@ -387,16 +387,13 @@ ImagingNew(const char* mode, int xsize, int ysize) int bytes; Imaging im; - if (strcmp(mode, "") == 0) - return (Imaging) ImagingError_ValueError("empty mode"); - if (strlen(mode) == 1) { if (mode[0] == 'F' || mode[0] == 'I') bytes = 4; else bytes = 1; } else - bytes = strlen(mode); /* close enough */ + bytes = strlen(mode) || 1; /* close enough */ if (xsize < 0 || ysize < 0) { return (Imaging) ImagingError_ValueError("bad image size"); From 2750d7bac34a2c96d41aca3f81179928d10a4885 Mon Sep 17 00:00:00 2001 From: Alexander Date: Sun, 6 Aug 2017 00:50:56 +0300 Subject: [PATCH 6/9] destroy image and set MemoryError on overflow check failure --- libImaging/Storage.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libImaging/Storage.c b/libImaging/Storage.c index 6acda2af0..7ab5ae1f5 100644 --- a/libImaging/Storage.c +++ b/libImaging/Storage.c @@ -344,7 +344,8 @@ ImagingNewBlock(const char *mode, int xsize, int ysize) if (im->linesize && im->ysize > INT_MAX / im->linesize) { /* punt if we're going to overflow */ - return NULL; + ImagingDelete(im); + return (Imaging) ImagingError_MemoryError(); } if (im->ysize * im->linesize <= 0) { From 0e70e6cb400b471d60801256a5565ee252b20d6c Mon Sep 17 00:00:00 2001 From: Alexander Date: Sun, 6 Aug 2017 01:20:00 +0300 Subject: [PATCH 7/9] =?UTF-8?q?rename=20ImagingNewBlock=20=E2=86=92=20Imag?= =?UTF-8?q?ingAllocateBlock=20rename=20ImagingNewArray=20=E2=86=92=20Imagi?= =?UTF-8?q?ngAllocateArray=20new=20utility=20function=20with=20old=20name?= =?UTF-8?q?=20ImagingNewBlock=20call=20ImagingNewPrologue=20outside=20of?= =?UTF-8?q?=20ImagingAllocateBlock=20and=20ImagingAllocateArray?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- libImaging/Imaging.h | 1 - libImaging/Storage.c | 50 +++++++++++++++++++++++++++----------------- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/libImaging/Imaging.h b/libImaging/Imaging.h index ede35f74c..35b563b1c 100644 --- a/libImaging/Imaging.h +++ b/libImaging/Imaging.h @@ -163,7 +163,6 @@ extern Imaging ImagingNew2(const char* mode, Imaging imOut, Imaging imIn); extern void ImagingDelete(Imaging im); extern Imaging ImagingNewBlock(const char* mode, int xsize, int ysize); -extern Imaging ImagingNewArray(const char* mode, int xsize, int ysize); extern Imaging ImagingNewMap(const char* filename, int readonly, const char* mode, int xsize, int ysize); diff --git a/libImaging/Storage.c b/libImaging/Storage.c index 7ab5ae1f5..155bac883 100644 --- a/libImaging/Storage.c +++ b/libImaging/Storage.c @@ -277,18 +277,13 @@ ImagingDestroyArray(Imaging im) } Imaging -ImagingNewArray(const char *mode, int xsize, int ysize) +ImagingAllocateArray(Imaging im) { - Imaging im; ImagingSectionCookie cookie; int y; char* p; - im = ImagingNewPrologue(mode, xsize, ysize); - if (!im) - return NULL; - ImagingSectionEnter(&cookie); /* Allocate image as an array of lines */ @@ -305,7 +300,6 @@ ImagingNewArray(const char *mode, int xsize, int ysize) ImagingSectionLeave(&cookie); if (y != im->ysize) { - ImagingDelete(im); return (Imaging) ImagingError_MemoryError(); } @@ -327,16 +321,10 @@ ImagingDestroyBlock(Imaging im) } Imaging -ImagingNewBlock(const char *mode, int xsize, int ysize) +ImagingAllocateBlock(Imaging im) { - Imaging im; Py_ssize_t y, i; - im = ImagingNewPrologue(mode, xsize, ysize); - if ( ! im) { - return NULL; - } - /* We shouldn't overflow, since the threshold defined below says that we're only going to allocate max 4M here before going to the array allocator. Check anyway. @@ -344,7 +332,6 @@ ImagingNewBlock(const char *mode, int xsize, int ysize) if (im->linesize && im->ysize > INT_MAX / im->linesize) { /* punt if we're going to overflow */ - ImagingDelete(im); return (Imaging) ImagingError_MemoryError(); } @@ -359,7 +346,6 @@ ImagingNewBlock(const char *mode, int xsize, int ysize) } if ( ! im->block) { - ImagingDelete(im); return (Imaging) ImagingError_MemoryError(); } @@ -400,15 +386,41 @@ ImagingNew(const char* mode, int xsize, int ysize) return (Imaging) ImagingError_ValueError("bad image size"); } + im = ImagingNewPrologue(mode, xsize, ysize); + if (!im) + return NULL; + if ((int64_t) xsize * (int64_t) ysize <= THRESHOLD / bytes) { - im = ImagingNewBlock(mode, xsize, ysize); - if (im) + if (ImagingAllocateBlock(im)) { return im; + } /* assume memory error; try allocating in array mode instead */ ImagingError_Clear(); } - return ImagingNewArray(mode, xsize, ysize); + if (ImagingAllocateArray(im)) { + return im; + } + + ImagingDelete(im); + return NULL; +} + +Imaging +ImagingNewBlock(const char* mode, int xsize, int ysize) +{ + Imaging im; + + im = ImagingNewPrologue(mode, xsize, ysize); + if ( ! im) + return NULL; + + if (ImagingAllocateBlock(im)) { + return im; + } + + ImagingDelete(im); + return NULL; } Imaging From 1ae089072bd53e53273fb1738acd0bfda1476eef Mon Sep 17 00:00:00 2001 From: Alexander Date: Sun, 6 Aug 2017 01:31:31 +0300 Subject: [PATCH 8/9] use accurate im->linesize instead of strlen(mode) approximation --- libImaging/Storage.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/libImaging/Storage.c b/libImaging/Storage.c index 155bac883..3c365207d 100644 --- a/libImaging/Storage.c +++ b/libImaging/Storage.c @@ -371,26 +371,17 @@ ImagingAllocateBlock(Imaging im) Imaging ImagingNew(const char* mode, int xsize, int ysize) { - int bytes; Imaging im; - if (strlen(mode) == 1) { - if (mode[0] == 'F' || mode[0] == 'I') - bytes = 4; - else - bytes = 1; - } else - bytes = strlen(mode) || 1; /* close enough */ - if (xsize < 0 || ysize < 0) { return (Imaging) ImagingError_ValueError("bad image size"); } im = ImagingNewPrologue(mode, xsize, ysize); - if (!im) + if ( ! im) return NULL; - if ((int64_t) xsize * (int64_t) ysize <= THRESHOLD / bytes) { + if (im->ysize && im->linesize <= THRESHOLD / im->ysize) { if (ImagingAllocateBlock(im)) { return im; } From 0f17356d46032d6417ddabab27de503c948bba60 Mon Sep 17 00:00:00 2001 From: Alexander Date: Sun, 6 Aug 2017 01:31:51 +0300 Subject: [PATCH 9/9] fix tests on python 3 --- Tests/test_image.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/test_image.py b/Tests/test_image.py index a83b917a0..dcfc28e4b 100644 --- a/Tests/test_image.py +++ b/Tests/test_image.py @@ -24,7 +24,7 @@ class TestImage(PillowTestCase): ]: with self.assertRaises(ValueError) as e: Image.new(mode, (1, 1)); - self.assertEqual(e.exception.message, 'unrecognized image mode') + self.assertEqual(str(e.exception), 'unrecognized image mode') def test_sanity(self):