diff --git a/.github/workflows/test-valgrind-memory.yml b/.github/workflows/test-valgrind-memory.yml new file mode 100644 index 000000000..e6a5f6e77 --- /dev/null +++ b/.github/workflows/test-valgrind-memory.yml @@ -0,0 +1,60 @@ +name: Test Valgrind Memory Leaks + +# like the Docker tests, but running valgrind only on *.c/*.h changes. + +# this is very expensive. Only run on the pull request. +on: + # push: + # branches: + # - "**" + # paths: + # - ".github/workflows/test-valgrind.yml" + # - "**.c" + # - "**.h" + pull_request: + paths: + - ".github/workflows/test-valgrind.yml" + - "**.c" + - "**.h" + - "depends/docker-test-valgrind-memory.sh" + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + build: + + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + docker: [ + ubuntu-22.04-jammy-amd64-valgrind, + ] + dockerTag: [main] + + name: ${{ matrix.docker }} + + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + + - name: Build system information + run: python3 .github/workflows/system-info.py + + - name: Docker pull + run: | + docker pull pythonpillow/${{ matrix.docker }}:${{ matrix.dockerTag }} + + - name: Build and Run Valgrind + run: | + # The Pillow user in the docker container is UID 1001 + sudo chown -R 1001 $GITHUB_WORKSPACE + docker run --name pillow_container -e "PILLOW_VALGRIND_TEST=true" -v $GITHUB_WORKSPACE:/Pillow pythonpillow/${{ matrix.docker }}:${{ matrix.dockerTag }} /Pillow/depends/docker-test-valgrind-memory.sh + sudo chown -R runner $GITHUB_WORKSPACE diff --git a/Makefile b/Makefile index 1f9d2ce13..ce780acbc 100644 --- a/Makefile +++ b/Makefile @@ -106,10 +106,18 @@ test-p: .PHONY: valgrind valgrind: python3 -c "import pytest_valgrind" > /dev/null 2>&1 || python3 -m pip install pytest-valgrind - PYTHONMALLOC=malloc valgrind --suppressions=Tests/oss-fuzz/python.supp --leak-check=no \ + PILLOW_VALGRIND_TEST=true PYTHONMALLOC=malloc valgrind --suppressions=Tests/oss-fuzz/python.supp --leak-check=no \ --log-file=/tmp/valgrind-output \ python3 -m pytest --no-memcheck -vv --valgrind --valgrind-log=/tmp/valgrind-output +.PHONY: valgrind-leak +valgrind-leak: + python3 -c "import pytest_valgrind" > /dev/null 2>&1 || python3 -m pip install pytest-valgrind + PILLOW_VALGRIND_TEST=true PYTHONMALLOC=malloc valgrind --suppressions=Tests/oss-fuzz/python.supp \ + --leak-check=full --show-leak-kinds=definite --errors-for-leak-kinds=definite \ + --log-file=/tmp/valgrind-output \ + python3 -m pytest -vv --valgrind --valgrind-log=/tmp/valgrind-output + .PHONY: readme readme: python3 -c "import markdown2" > /dev/null 2>&1 || python3 -m pip install markdown2 diff --git a/Tests/oss-fuzz/python.supp b/Tests/oss-fuzz/python.supp index 36385d672..4803497ad 100644 --- a/Tests/oss-fuzz/python.supp +++ b/Tests/oss-fuzz/python.supp @@ -14,3 +14,23 @@ fun:_TIFFReadEncodedTileAndAllocBuffer ... } + +{ + + Memcheck:Leak + match-leak-kinds: all + fun:malloc + fun:_PyMem_RawMalloc + fun:PyObject_Malloc + ... +} + +{ + + Memcheck:Leak + match-leak-kinds: all + fun:malloc + fun:_PyMem_RawRealloc + fun:PyMem_Realloc + ... +} diff --git a/depends/docker-test-valgrind-memory.sh b/depends/docker-test-valgrind-memory.sh new file mode 100755 index 000000000..f0d1d851d --- /dev/null +++ b/depends/docker-test-valgrind-memory.sh @@ -0,0 +1,11 @@ +#!/bin/bash + +## Run this as the test script in the Docker valgrind image. +## Note -- can be included directly into the Docker image, +## but requires the current python.supp. + +source /vpy3/bin/activate +cd /Pillow +make clean +make install +make valgrind-leak diff --git a/src/_imaging.c b/src/_imaging.c index 72f122143..79e0a2b23 100644 --- a/src/_imaging.c +++ b/src/_imaging.c @@ -2226,6 +2226,7 @@ _unsharp_mask(ImagingObject *self, PyObject *args) { } if (!ImagingUnsharpMask(imOut, imIn, radius, percent, threshold)) { + ImagingDelete(imOut); return NULL; } diff --git a/src/_imagingft.c b/src/_imagingft.c index 62dab73e5..0d70544a5 100644 --- a/src/_imagingft.c +++ b/src/_imagingft.c @@ -275,6 +275,7 @@ text_layout_raqm( if (!text || !size) { /* return 0 and clean up, no glyphs==no size, and raqm fails with empty strings */ + PyMem_Free(text); goto failed; } set_text = raqm_set_text(rq, text, size); @@ -425,6 +426,7 @@ text_layout_fallback( "setting text direction, language or font features is not supported " "without libraqm" ); + return 0; } if (PyUnicode_Check(string)) { diff --git a/src/_webp.c b/src/_webp.c index 3aa4c408b..a7809c40e 100644 --- a/src/_webp.c +++ b/src/_webp.c @@ -641,6 +641,10 @@ WebPEncode_wrapper(PyObject *self, PyObject *args) { ImagingSectionLeave(&cookie); WebPPictureFree(&pic); + + output = writer.mem; + ret_size = writer.size; + if (!ok) { int error_code = (&pic)->error_code; char message[50] = ""; @@ -652,10 +656,9 @@ WebPEncode_wrapper(PyObject *self, PyObject *args) { ); } PyErr_Format(PyExc_ValueError, "encoding error %d%s", error_code, message); + free(output); return NULL; } - output = writer.mem; - ret_size = writer.size; { /* I want to truncate the *_size items that get passed into WebP diff --git a/src/encode.c b/src/encode.c index 7c365a74f..e56494036 100644 --- a/src/encode.c +++ b/src/encode.c @@ -703,6 +703,8 @@ PyImaging_LibTiffEncoderNew(PyObject *self, PyObject *args) { return NULL; } + encoder->cleanup = ImagingLibTiffEncodeCleanup; + num_core_tags = sizeof(core_tags) / sizeof(int); for (pos = 0; pos < tags_size; pos++) { item = PyList_GetItemRef(tags, pos); diff --git a/src/libImaging/Arrow.c b/src/libImaging/Arrow.c index 33ff2ce77..0b8c89a07 100644 --- a/src/libImaging/Arrow.c +++ b/src/libImaging/Arrow.c @@ -36,7 +36,10 @@ ReleaseExportedSchema(struct ArrowSchema *array) { child->release(child); child->release = NULL; } - // UNDONE -- should I be releasing the children? + free(array->children[i]); + } + if (array->children) { + free(array->children); } // Release dictionary @@ -117,6 +120,7 @@ export_imaging_schema(Imaging im, struct ArrowSchema *schema) { retval = export_named_type(schema->children[0], im->arrow_band_format, "pixel"); if (retval != 0) { free(schema->children[0]); + free(schema->children); schema->release(schema); return retval; } @@ -127,9 +131,7 @@ static void release_const_array(struct ArrowArray *array) { Imaging im = (Imaging)array->private_data; - if (array->n_children == 0) { - ImagingDelete(im); - } + ImagingDelete(im); // Free the buffers and the buffers array if (array->buffers) { diff --git a/src/libImaging/JpegEncode.c b/src/libImaging/JpegEncode.c index 3c11eac22..79a38e12f 100644 --- a/src/libImaging/JpegEncode.c +++ b/src/libImaging/JpegEncode.c @@ -131,6 +131,7 @@ ImagingJpegEncode(Imaging im, ImagingCodecState state, UINT8 *buf, int bytes) { break; default: state->errcode = IMAGING_CODEC_CONFIG; + jpeg_destroy_compress(&context->cinfo); return -1; } @@ -161,6 +162,7 @@ ImagingJpegEncode(Imaging im, ImagingCodecState state, UINT8 *buf, int bytes) { /* Would subsample the green and blue channels, which doesn't make sense */ state->errcode = IMAGING_CODEC_CONFIG; + jpeg_destroy_compress(&context->cinfo); return -1; } jpeg_set_colorspace(&context->cinfo, JCS_RGB); diff --git a/src/libImaging/TiffDecode.c b/src/libImaging/TiffDecode.c index 9a2db95b4..173eca160 100644 --- a/src/libImaging/TiffDecode.c +++ b/src/libImaging/TiffDecode.c @@ -929,6 +929,27 @@ ImagingLibTiffSetField(ImagingCodecState state, ttag_t tag, ...) { return status; } +int +ImagingLibTiffEncodeCleanup(ImagingCodecState state) { + TIFFSTATE *clientstate = (TIFFSTATE *)state->context; + TIFF *tiff = clientstate->tiff; + + if (!tiff) { + return 0; + } + // TIFFClose in libtiff calls tif_closeproc and TIFFCleanup + if (clientstate->fp) { + // Python will manage the closing of the file rather than libtiff + // So only call TIFFCleanup + TIFFCleanup(tiff); + } else { + // When tif_closeproc refers to our custom _tiffCloseProc though, + // that is fine, as it does not close the file + TIFFClose(tiff); + } + return 0; +} + int ImagingLibTiffEncode(Imaging im, ImagingCodecState state, UINT8 *buffer, int bytes) { /* One shot encoder. Encode everything to the tiff in the clientstate. @@ -1010,16 +1031,6 @@ ImagingLibTiffEncode(Imaging im, ImagingCodecState state, UINT8 *buffer, int byt TRACE(("Encode Error, row %d\n", state->y)); state->errcode = IMAGING_CODEC_BROKEN; - // TIFFClose in libtiff calls tif_closeproc and TIFFCleanup - if (clientstate->fp) { - // Python will manage the closing of the file rather than libtiff - // So only call TIFFCleanup - TIFFCleanup(tiff); - } else { - // When tif_closeproc refers to our custom _tiffCloseProc though, - // that is fine, as it does not close the file - TIFFClose(tiff); - } if (!clientstate->fp) { free(clientstate->data); } @@ -1036,22 +1047,11 @@ ImagingLibTiffEncode(Imaging im, ImagingCodecState state, UINT8 *buffer, int byt TRACE(("Error flushing the tiff")); // likely reason is memory. state->errcode = IMAGING_CODEC_MEMORY; - if (clientstate->fp) { - TIFFCleanup(tiff); - } else { - TIFFClose(tiff); - } if (!clientstate->fp) { free(clientstate->data); } return -1; } - TRACE(("Closing \n")); - if (clientstate->fp) { - TIFFCleanup(tiff); - } else { - TIFFClose(tiff); - } // reset the clientstate metadata to use it to read out the buffer. clientstate->loc = 0; clientstate->size = clientstate->eof; // redundant? diff --git a/src/libImaging/TiffDecode.h b/src/libImaging/TiffDecode.h index 22361210d..77808b543 100644 --- a/src/libImaging/TiffDecode.h +++ b/src/libImaging/TiffDecode.h @@ -40,6 +40,8 @@ ImagingLibTiffInit(ImagingCodecState state, int fp, uint32_t offset); extern int ImagingLibTiffEncodeInit(ImagingCodecState state, char *filename, int fp); extern int +ImagingLibTiffEncodeCleanup(ImagingCodecState state); +extern int ImagingLibTiffMergeFieldInfo( ImagingCodecState state, TIFFDataType field_type, int key, int is_var_length );