From d6c16aa01536165bf82747819ebcecb1bc248d45 Mon Sep 17 00:00:00 2001 From: wiredfool Date: Wed, 24 Sep 2014 15:37:28 -0700 Subject: [PATCH 1/4] Tests for jpeg memory leaks --- Tests/check_jpeg_leaks.py | 212 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 Tests/check_jpeg_leaks.py diff --git a/Tests/check_jpeg_leaks.py b/Tests/check_jpeg_leaks.py new file mode 100644 index 000000000..adf7652cf --- /dev/null +++ b/Tests/check_jpeg_leaks.py @@ -0,0 +1,212 @@ +from helper import unittest, PillowTestCase, hopper +from PIL import Image +from io import BytesIO +import sys + +iterations = 5000 + + +""" +When run on a system without the jpeg leak fixes, the valgrind runs look like this. + +NOSE_PROCESSES=0 NOSE_TIMEOUT=600 valgrind --tool=massif \ + python test-installed.py -s -v Tests/check_jpeg_leaks.py + +""" + + +@unittest.skipIf(sys.platform.startswith('win32'), "requires Unix or MacOS") +class TestJpegLeaks(PillowTestCase): + + """ +pre patch: + + MB +31.62^ : + | @:@:@:@#:: + | @:@:@@:@:@:@:@:@#:: + | ::::::::@:@:@@:@:@:@:@:@#:: + | :::::@::::::: ::::@:@:@@:@:@:@:@:@#:: + | @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | ::::::@::::@:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | ::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | :::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | ::::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | ::::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | ::::::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | : ::::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | @: ::::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | @@: ::::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | :@@: ::::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | :@@: ::::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | :@:@@: ::::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | :@:@@: ::::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + | :@:@@: ::::::::: : :@: : @:::::::::::::@: : ::: ::::@:@:@@:@:@:@:@:@#:: + 0 +----------------------------------------------------------------------->Gi + 0 8.535 + + +post-patch: + + MB +21.03^ :::@@:::@::::@@:::::::@@::::::::@::::::::::::@:::@:::::::@:::: + | #:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | #:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | :::#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | : :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | : :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | @: :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | @@: :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | @@: :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | @@: :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | :@@: :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | :@@: :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | :@:@@: :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | :@:@@: :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | :@:@@: :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | :@:@@: :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + | :@:@@: :#:::@ :::@::::@ : :: : @ :::::: :@:: ::: :::: @:: @:::::::@:::: + 0 +----------------------------------------------------------------------->Gi + 0 8.421 + +""" + + + def test_qtables_leak(self): + im = hopper('RGB') + + standard_l_qtable = [int(s) for s in """ + 16 11 10 16 24 40 51 61 + 12 12 14 19 26 58 60 55 + 14 13 16 24 40 57 69 56 + 14 17 22 29 51 87 80 62 + 18 22 37 56 68 109 103 77 + 24 35 55 64 81 104 113 92 + 49 64 78 87 103 121 120 101 + 72 92 95 98 112 100 103 99 + """.split(None)] + + standard_chrominance_qtable = [int(s) for s in """ + 17 18 24 47 99 99 99 99 + 18 21 26 66 99 99 99 99 + 24 26 56 99 99 99 99 99 + 47 66 99 99 99 99 99 99 + 99 99 99 99 99 99 99 99 + 99 99 99 99 99 99 99 99 + 99 99 99 99 99 99 99 99 + 99 99 99 99 99 99 99 99 + """.split(None)] + + qtables = [standard_l_qtable, + standard_chrominance_qtable] + + + for count in range(iterations): + test_output = BytesIO() + im.save(test_output, "JPEG", qtables=qtables) + + """ +pre patch: + + MB +177.1^ # + | @@@# + | :@@@@@@# + | ::::@@@@@@# + | ::::::::@@@@@@# + | @@::::: ::::@@@@@@# + | @@@@ ::::: ::::@@@@@@# + | @@@@@@@ ::::: ::::@@@@@@# + | @@::@@@@@@@ ::::: ::::@@@@@@# + | @@@@ : @@@@@@@ ::::: ::::@@@@@@# + | @@@@@@ @@ : @@@@@@@ ::::: ::::@@@@@@# + | @@@@ @@ @ @@ : @@@@@@@ ::::: ::::@@@@@@# + | @::@@ @@ @@ @ @@ : @@@@@@@ ::::: ::::@@@@@@# + | ::::@: @@ @@ @@ @ @@ : @@@@@@@ ::::: ::::@@@@@@# + | :@@: : @: @@ @@ @@ @ @@ : @@@@@@@ ::::: ::::@@@@@@# + | ::@@::@@: : @: @@ @@ @@ @ @@ : @@@@@@@ ::::: ::::@@@@@@# + | @@::: @ ::@@: : @: @@ @@ @@ @ @@ : @@@@@@@ ::::: ::::@@@@@@# + | @::@ : : @ ::@@: : @: @@ @@ @@ @ @@ : @@@@@@@ ::::: ::::@@@@@@# + | :::@: @ : : @ ::@@: : @: @@ @@ @@ @ @@ : @@@@@@@ ::::: ::::@@@@@@# + | @@@:: @: @ : : @ ::@@: : @: @@ @@ @@ @ @@ : @@@@@@@ ::::: ::::@@@@@@# + 0 +----------------------------------------------------------------------->Gi + 0 11.37 + + +post patch: + + MB +21.06^ ::::::::::::::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | ##::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | # ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | # ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | # ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @@# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @@@# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @@@# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @@@# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @@@# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @@@# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @@@@@# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @ @@@# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @ @@@# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @ @@@# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + | @ @@@# ::: ::::: : ::::::::::@::::@::::@::::@::::@::::@:::::::::@:::::: + 0 +----------------------------------------------------------------------->Gi + 0 11.33 + +""" + + def test_exif_leak(self): + im = hopper('RGB') + exif = b'12345678'*4096 + + for count in range(iterations): + test_output = BytesIO() + im.save(test_output, "JPEG", exif=exif) + + + """ +base case: + MB +20.99^ ::::: :::::::::::::::::::::::::::::::::::::::::::@::: + | ##: : ::::::@::::::: :::: :::: : : : : : : :::::::::::: :::@::: + | # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | @@# : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | @@@ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | @ @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | @@ @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | @@ @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | @@ @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | @@ @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | @@ @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | :@@@@ @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | :@ @@ @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | :@ @@ @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | :@ @@ @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + | :@ @@ @ # : : :: :: @:: :::: :::: :::: : : : : : : :::::::::::: :::@::: + 0 +----------------------------------------------------------------------->Gi + 0 7.882 +""" + + def test_base_save(self): + im = hopper('RGB') + + for count in range(iterations): + test_output = BytesIO() + im.save(test_output, "JPEG") + + +if __name__ == '__main__': + unittest.main() From 306ad7432413c1d102e76b32a8780994cecc3a7d Mon Sep 17 00:00:00 2001 From: wiredfool Date: Wed, 24 Sep 2014 14:15:17 -0700 Subject: [PATCH 2/4] qtables error handling tests --- Tests/test_file_jpeg.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Tests/test_file_jpeg.py b/Tests/test_file_jpeg.py index e46b656bb..a11a2200e 100644 --- a/Tests/test_file_jpeg.py +++ b/Tests/test_file_jpeg.py @@ -295,6 +295,21 @@ class TestFileJpeg(PillowTestCase): 1: standard_chrominance_qtable}), 30) + # not a sequence + self.assertRaises(Exception, lambda: self.roundtrip(im, qtables='a')) + # sequence wrong length + self.assertRaises(Exception, lambda: self.roundtrip(im, qtables=[])) + # sequence wrong length + self.assertRaises(Exception, lambda: self.roundtrip(im, qtables=[1,2,3,4,5])) + + # qtable entry not a sequence + self.assertRaises(Exception, lambda: self.roundtrip(im, qtables=[1])) + # qtable entry has wrong number of items + self.assertRaises(Exception, lambda: self.roundtrip(im, qtables=[[1,2,3,4]])) + + + + @unittest.skipUnless(djpeg_available(), "djpeg not available") def test_load_djpeg(self): img = Image.open(TEST_FILE) From d64b2376bc8cd38c3ba24f13de943d9af916c03c Mon Sep 17 00:00:00 2001 From: wiredfool Date: Mon, 22 Sep 2014 22:30:00 -0700 Subject: [PATCH 3/4] Memory Leak: Freeing malloc'd memory in Jpeg Encode --- encode.c | 39 +++++++++++++++++---------------------- libImaging/Jpeg.h | 4 +++- libImaging/JpegEncode.c | 15 +++++++++++++-- 3 files changed, 33 insertions(+), 25 deletions(-) diff --git a/encode.c b/encode.c index c26b7dfb7..d7a074720 100644 --- a/encode.c +++ b/encode.c @@ -535,12 +535,12 @@ PyImaging_ZipEncoderNew(PyObject* self, PyObject* args) #include "Jpeg.h" -static unsigned int** get_qtables_arrays(PyObject* qtables, int* qtablesLen) { +static unsigned int* get_qtables_arrays(PyObject* qtables, int* qtablesLen) { PyObject* tables; PyObject* table; PyObject* table_data; int i, j, num_tables; - unsigned int **qarrays; + unsigned int *qarrays; if ((qtables == NULL) || (qtables == Py_None)) { return NULL; @@ -554,10 +554,12 @@ static unsigned int** get_qtables_arrays(PyObject* qtables, int* qtablesLen) { tables = PySequence_Fast(qtables, "expected a sequence"); num_tables = PySequence_Size(qtables); if (num_tables < 1 || num_tables > NUM_QUANT_TBLS) { - PyErr_SetString(PyExc_ValueError, "Not a valid numbers of quantization tables. Should be between 1 and 4."); + PyErr_SetString(PyExc_ValueError, + "Not a valid number of quantization tables. Should be between 1 and 4."); + Py_DECREF(tables); return NULL; } - qarrays = (unsigned int**) PyMem_Malloc(num_tables * sizeof(unsigned int*)); + qarrays = (unsigned int*) malloc(num_tables * DCTSIZE2 * sizeof(unsigned int)); if (!qarrays) { Py_DECREF(tables); PyErr_NoMemory(); @@ -566,39 +568,32 @@ static unsigned int** get_qtables_arrays(PyObject* qtables, int* qtablesLen) { for (i = 0; i < num_tables; i++) { table = PySequence_Fast_GET_ITEM(tables, i); if (!PySequence_Check(table)) { - Py_DECREF(tables); PyErr_SetString(PyExc_ValueError, "Invalid quantization tables"); - return NULL; + goto JPEG_QTABLES_ERR; } if (PySequence_Size(table) != DCTSIZE2) { - Py_DECREF(tables); - PyErr_SetString(PyExc_ValueError, "Invalid quantization tables"); - return NULL; + PyErr_SetString(PyExc_ValueError, "Invalid quantization table size"); + goto JPEG_QTABLES_ERR; } table_data = PySequence_Fast(table, "expected a sequence"); - qarrays[i] = (unsigned int*) PyMem_Malloc(DCTSIZE2 * sizeof(unsigned int)); - if (!qarrays[i]) { - Py_DECREF(tables); - PyErr_NoMemory(); - return NULL; - } for (j = 0; j < DCTSIZE2; j++) { - qarrays[i][j] = PyInt_AS_LONG(PySequence_Fast_GET_ITEM(table_data, j)); + qarrays[i * DCTSIZE2 + j] = PyInt_AS_LONG(PySequence_Fast_GET_ITEM(table_data, j)); } } - Py_DECREF(tables); *qtablesLen = num_tables; +JPEG_QTABLES_ERR: + Py_DECREF(tables); // Run on both error and not error if (PyErr_Occurred()) { - PyMem_Free(qarrays); + free(qarrays); qarrays = NULL; + return NULL; } return qarrays; } - PyObject* PyImaging_JpegEncoderNew(PyObject* self, PyObject* args) { @@ -614,7 +609,7 @@ PyImaging_JpegEncoderNew(PyObject* self, PyObject* args) int xdpi = 0, ydpi = 0; int subsampling = -1; /* -1=default, 0=none, 1=medium, 2=high */ PyObject* qtables=NULL; - unsigned int **qarrays = NULL; + unsigned int *qarrays = NULL; int qtablesLen = 0; char* extra = NULL; int extra_size; @@ -638,7 +633,7 @@ PyImaging_JpegEncoderNew(PyObject* self, PyObject* args) qarrays = get_qtables_arrays(qtables, &qtablesLen); if (extra && extra_size > 0) { - char* p = malloc(extra_size); + char* p = malloc(extra_size); // Freed in JpegEncode, Case 5 if (!p) return PyErr_NoMemory(); memcpy(p, extra, extra_size); @@ -647,7 +642,7 @@ PyImaging_JpegEncoderNew(PyObject* self, PyObject* args) extra = NULL; if (rawExif && rawExifLen > 0) { - char* pp = malloc(rawExifLen); + char* pp = malloc(rawExifLen); // Freed in JpegEncode, Case 5 if (!pp) return PyErr_NoMemory(); memcpy(pp, rawExif, rawExifLen); diff --git a/libImaging/Jpeg.h b/libImaging/Jpeg.h index acd2da676..9536cad79 100644 --- a/libImaging/Jpeg.h +++ b/libImaging/Jpeg.h @@ -89,7 +89,9 @@ typedef struct { int subsampling; /* Custom quantization tables () */ - unsigned int **qtables; + unsigned int *qtables; + + /* in factors of DCTSIZE2 */ int qtablesLen; /* Extra data (to be injected after header) */ diff --git a/libImaging/JpegEncode.c b/libImaging/JpegEncode.c index 54876589d..3fe5d753f 100644 --- a/libImaging/JpegEncode.c +++ b/libImaging/JpegEncode.c @@ -153,7 +153,7 @@ ImagingJpegEncode(Imaging im, ImagingCodecState state, UINT8* buf, int bytes) } for (i = 0; i < context->qtablesLen; i++) { // TODO: Should add support for none baseline - jpeg_add_quant_table(&context->cinfo, i, context->qtables[i], + jpeg_add_quant_table(&context->cinfo, i, &context->qtables[i * DCTSIZE2], quality, TRUE); } } else if (context->quality > 0) { @@ -289,8 +289,19 @@ ImagingJpegEncode(Imaging im, ImagingCodecState state, UINT8* buf, int bytes) jpeg_finish_compress(&context->cinfo); /* Clean up */ - if (context->extra) + if (context->extra) { free(context->extra); + context->extra = NULL; + } + if (context->rawExif) { + free(context->rawExif); + context->rawExif = NULL; + } + if (context->qtables) { + free(context->qtables); + context->qtables = NULL; + } + jpeg_destroy_compress(&context->cinfo); /* if (jerr.pub.num_warnings) return BROKEN; */ state->errcode = IMAGING_CODEC_END; From c154fd05fd4072f4299f6e8bf28a444b259919bb Mon Sep 17 00:00:00 2001 From: wiredfool Date: Wed, 24 Sep 2014 15:27:43 -0700 Subject: [PATCH 4/4] Memory Leak, missing Py_DECREF --- encode.c | 1 + 1 file changed, 1 insertion(+) diff --git a/encode.c b/encode.c index d7a074720..fef102176 100644 --- a/encode.c +++ b/encode.c @@ -579,6 +579,7 @@ static unsigned int* get_qtables_arrays(PyObject* qtables, int* qtablesLen) { for (j = 0; j < DCTSIZE2; j++) { qarrays[i * DCTSIZE2 + j] = PyInt_AS_LONG(PySequence_Fast_GET_ITEM(table_data, j)); } + Py_DECREF(table_data); } *qtablesLen = num_tables;