From 0b575d9346020b365b01598a034275a12b2122d3 Mon Sep 17 00:00:00 2001 From: johncf Date: Thu, 10 Oct 2024 23:41:01 +0530 Subject: [PATCH 1/6] improve error handling in _pillow_heif.c --- pillow_heif/_pillow_heif.c | 54 ++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/pillow_heif/_pillow_heif.c b/pillow_heif/_pillow_heif.c index be80ef46..4f69e832 100644 --- a/pillow_heif/_pillow_heif.c +++ b/pillow_heif/_pillow_heif.c @@ -738,12 +738,13 @@ PyObject* _CtxDepthImage(struct heif_image_handle* main_handle, heif_item_id dep int remove_stride, int hdr_to_16bit, PyObject* file_bytes) { struct heif_image_handle* depth_handle; if (check_error(heif_image_handle_get_depth_image_handle(main_handle, depth_image_id, &depth_handle))) { - Py_RETURN_NONE; + return NULL; } CtxImageObject *ctx_image = PyObject_New(CtxImageObject, &CtxImage_Type); if (!ctx_image) { heif_image_handle_release(depth_handle); - Py_RETURN_NONE; + PyErr_SetString(PyExc_RuntimeError, "could not create CtxImage object"); + return NULL; } if (!heif_image_handle_get_depth_image_representation_info(main_handle, depth_image_id, &ctx_image->depth_metadata)) ctx_image->depth_metadata = NULL; @@ -803,7 +804,8 @@ PyObject* _CtxImage(struct heif_image_handle* handle, int hdr_to_8bit, CtxImageObject *ctx_image = PyObject_New(CtxImageObject, &CtxImage_Type); if (!ctx_image) { heif_image_handle_release(handle); - Py_RETURN_NONE; + PyErr_SetString(PyExc_RuntimeError, "could not create CtxImage object"); + return NULL; } ctx_image->depth_metadata = NULL; ctx_image->image_type = PhHeifImage; @@ -925,7 +927,7 @@ static PyObject* _CtxImage_color_profile(CtxImageObject* self, void* closure) { if (!data) { Py_DECREF(result); result = NULL; - PyErr_SetString(PyExc_OSError, "Out of Memory"); + PyErr_NoMemory(); } else { if (!check_error(heif_image_handle_get_raw_color_profile(self->handle, data))) @@ -954,15 +956,13 @@ static PyObject* _CtxImage_metadata(CtxImageObject* self, void* closure) { heif_item_id* meta_ids = (heif_item_id*)malloc(n_metas * sizeof(heif_item_id)); if (!meta_ids) { - PyErr_SetString(PyExc_OSError, "Out of Memory"); - return NULL; + return PyErr_NoMemory(); } n_metas = heif_image_handle_get_list_of_metadata_block_IDs(self->handle, NULL, meta_ids, n_metas); PyObject* meta_list = PyList_New(n_metas); if (!meta_list) { free(meta_ids); - PyErr_SetString(PyExc_OSError, "Out of Memory"); - return NULL; + return PyErr_NoMemory(); } for (int i = 0; i < n_metas; i++) { @@ -1163,21 +1163,20 @@ static PyObject* _CtxImage_depth_image_list(CtxImageObject* self, void* closure) return PyList_New(0); heif_item_id* images_ids = (heif_item_id*)malloc(n_images * sizeof(heif_item_id)); if (!images_ids) - return PyList_New(0); + return PyErr_NoMemory(); n_images = heif_image_handle_get_list_of_depth_image_IDs(self->handle, images_ids, n_images); PyObject* images_list = PyList_New(n_images); if (!images_list) { free(images_ids); - return PyList_New(0); + return PyErr_NoMemory(); } for (int i = 0; i < n_images; i++) { - PyList_SET_ITEM(images_list, - i, - _CtxDepthImage( - self->handle, images_ids[i], self->remove_stride, self->hdr_to_16bit, self->file_bytes - )); + PyObject* ctx_depth_image = _CtxDepthImage( + self->handle, images_ids[i], self->remove_stride, self->hdr_to_16bit, self->file_bytes); + // TODO how to handle the error if ctx_depth_image == NULL? + PyList_SET_ITEM(images_list, i, ctx_depth_image); } free(images_ids); return images_list; @@ -1193,7 +1192,7 @@ static PyObject* _CtxImage_camera_intrinsic_matrix(CtxImageObject* self, void* c Py_RETURN_NONE; } if (check_error(heif_image_handle_get_camera_intrinsic_matrix(self->handle, &camera_intrinsic_matrix))) { - Py_RETURN_NONE; + return NULL; } return Py_BuildValue( "(ddddd)", @@ -1218,12 +1217,12 @@ static PyObject* _CtxImage_camera_extrinsic_matrix_rot(CtxImageObject* self, voi Py_RETURN_NONE; } if (check_error(heif_image_handle_get_camera_extrinsic_matrix(self->handle, &camera_extrinsic_matrix))) { - Py_RETURN_NONE; + return NULL; } error = heif_camera_extrinsic_matrix_get_rotation_matrix(camera_extrinsic_matrix, rot); heif_camera_extrinsic_matrix_release(camera_extrinsic_matrix); if (check_error(error)) { - Py_RETURN_NONE; + return NULL; } return Py_BuildValue("(ddddddddd)", rot[0], rot[1], rot[2], rot[3], rot[4], rot[5], rot[6], rot[7], rot[8]); #else @@ -1338,16 +1337,14 @@ static PyObject* _load_file(PyObject* self, PyObject* args) { heif_item_id* images_ids = (heif_item_id*)malloc(n_images * sizeof(heif_item_id)); if (!images_ids) { heif_context_free(heif_ctx); - PyErr_SetString(PyExc_OSError, "Out of Memory"); - return NULL; + return PyErr_NoMemory(); } n_images = heif_context_get_list_of_top_level_image_IDs(heif_ctx, images_ids, n_images); PyObject* images_list = PyList_New(n_images); if (!images_list) { free(images_ids); heif_context_free(heif_ctx); - PyErr_SetString(PyExc_OSError, "Out of Memory"); - return NULL; + return PyErr_NoMemory(); } enum heif_colorspace colorspace; @@ -1365,11 +1362,11 @@ static PyObject* _load_file(PyObject* self, PyObject* args) { if (error.code == heif_error_Ok) { error = heif_image_handle_get_preferred_decoding_colorspace(handle, &colorspace, &chroma); if (error.code == heif_error_Ok) { - PyList_SET_ITEM(images_list, - i, - _CtxImage(handle, hdr_to_8bit, - bgr_mode, remove_stride, hdr_to_16bit, reload_size, primary, heif_bytes, - decoder_id, colorspace, chroma)); + PyObject* ctx_image = _CtxImage( + handle, hdr_to_8bit, bgr_mode, remove_stride, hdr_to_16bit, reload_size, primary, heif_bytes, + decoder_id, colorspace, chroma); + // TODO how to handle the error if ctx_image == NULL? + PyList_SET_ITEM(images_list, i, ctx_image); } else { heif_image_handle_release(handle); } @@ -1389,8 +1386,7 @@ static PyObject* _get_lib_info(PyObject* self) { PyObject* encoders_dict = PyDict_New(); PyObject* decoders_dict = PyDict_New(); if ((!lib_info_dict) || (!encoders_dict) || (!decoders_dict)) { - PyErr_SetString(PyExc_OSError, "Out of Memory"); - return NULL; + return PyErr_NoMemory(); } __PyDict_SetItemString(lib_info_dict, "libheif", PyUnicode_FromString(heif_get_version())); From 106dc5de9757338613d5470cd9debd312683d157 Mon Sep 17 00:00:00 2001 From: John C Date: Fri, 11 Oct 2024 11:19:02 +0530 Subject: [PATCH 2/6] make the behavior more consistent --- pillow_heif/_pillow_heif.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pillow_heif/_pillow_heif.c b/pillow_heif/_pillow_heif.c index 4f69e832..c4cb7756 100644 --- a/pillow_heif/_pillow_heif.c +++ b/pillow_heif/_pillow_heif.c @@ -804,8 +804,8 @@ PyObject* _CtxImage(struct heif_image_handle* handle, int hdr_to_8bit, CtxImageObject *ctx_image = PyObject_New(CtxImageObject, &CtxImage_Type); if (!ctx_image) { heif_image_handle_release(handle); - PyErr_SetString(PyExc_RuntimeError, "could not create CtxImage object"); - return NULL; + // note: silently ignoring the error + Py_RETURN_NONE; } ctx_image->depth_metadata = NULL; ctx_image->image_type = PhHeifImage; @@ -1175,7 +1175,11 @@ static PyObject* _CtxImage_depth_image_list(CtxImageObject* self, void* closure) for (int i = 0; i < n_images; i++) { PyObject* ctx_depth_image = _CtxDepthImage( self->handle, images_ids[i], self->remove_stride, self->hdr_to_16bit, self->file_bytes); - // TODO how to handle the error if ctx_depth_image == NULL? + if (!ctx_depth_image) { + Py_DECREF(images_list); + free(images_ids); + return NULL; + } PyList_SET_ITEM(images_list, i, ctx_depth_image); } free(images_ids); @@ -1365,7 +1369,7 @@ static PyObject* _load_file(PyObject* self, PyObject* args) { PyObject* ctx_image = _CtxImage( handle, hdr_to_8bit, bgr_mode, remove_stride, hdr_to_16bit, reload_size, primary, heif_bytes, decoder_id, colorspace, chroma); - // TODO how to handle the error if ctx_image == NULL? + // note: ctx_image could be None here PyList_SET_ITEM(images_list, i, ctx_image); } else { heif_image_handle_release(handle); From 34291bfb81ca17f8cd34853656f085cdfeb71021 Mon Sep 17 00:00:00 2001 From: John C Date: Fri, 11 Oct 2024 11:40:54 +0530 Subject: [PATCH 3/6] handle PyObject_New failures correctly A MemoryError is already set when PyObject_New fails: https://github.com/python/cpython/blob/2f8301cbf/Objects/object.c#L459-L468 --- pillow_heif/_pillow_heif.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pillow_heif/_pillow_heif.c b/pillow_heif/_pillow_heif.c index c4cb7756..d13f1367 100644 --- a/pillow_heif/_pillow_heif.c +++ b/pillow_heif/_pillow_heif.c @@ -704,7 +704,6 @@ static PyObject* _CtxWriteImage_create(CtxWriteObject* self, PyObject* args) { CtxWriteImageObject* ctx_write_image = PyObject_New(CtxWriteImageObject, &CtxWriteImage_Type); if (!ctx_write_image) { heif_image_release(image); - PyErr_SetString(PyExc_RuntimeError, "could not create CtxWriteImage object"); return NULL; } ctx_write_image->chroma = chroma; @@ -743,7 +742,6 @@ PyObject* _CtxDepthImage(struct heif_image_handle* main_handle, heif_item_id dep CtxImageObject *ctx_image = PyObject_New(CtxImageObject, &CtxImage_Type); if (!ctx_image) { heif_image_handle_release(depth_handle); - PyErr_SetString(PyExc_RuntimeError, "could not create CtxImage object"); return NULL; } if (!heif_image_handle_get_depth_image_representation_info(main_handle, depth_image_id, &ctx_image->depth_metadata)) @@ -804,8 +802,7 @@ PyObject* _CtxImage(struct heif_image_handle* handle, int hdr_to_8bit, CtxImageObject *ctx_image = PyObject_New(CtxImageObject, &CtxImage_Type); if (!ctx_image) { heif_image_handle_release(handle); - // note: silently ignoring the error - Py_RETURN_NONE; + return NULL; } ctx_image->depth_metadata = NULL; ctx_image->image_type = PhHeifImage; @@ -1295,7 +1292,6 @@ static PyObject* _CtxWrite(PyObject* self, PyObject* args) { if (!ctx_write) { heif_encoder_release(encoder); heif_context_free(ctx); - PyErr_SetString(PyExc_RuntimeError, "could not create CtxWrite object"); return NULL; } ctx_write->ctx = ctx; @@ -1369,7 +1365,13 @@ static PyObject* _load_file(PyObject* self, PyObject* args) { PyObject* ctx_image = _CtxImage( handle, hdr_to_8bit, bgr_mode, remove_stride, hdr_to_16bit, reload_size, primary, heif_bytes, decoder_id, colorspace, chroma); - // note: ctx_image could be None here + if (!ctx_image) { + Py_DECREF(images_list); + heif_image_handle_release(handle); + free(images_ids); + heif_context_free(heif_ctx); + return NULL; + } PyList_SET_ITEM(images_list, i, ctx_image); } else { heif_image_handle_release(handle); From 7822e844d0cbe1fa028794bfdb06adf7a7052160 Mon Sep 17 00:00:00 2001 From: John C Date: Fri, 11 Oct 2024 11:57:24 +0530 Subject: [PATCH 4/6] handle PyList_New failures correctly A MemoryError is already set when PyList_New fails: https://github.com/python/cpython/blob/2f8301cbf/Objects/listobject.c#L209-L248 https://github.com/python/cpython/blob/2f8301cbf/Python/gc.c#L2105-L2122 https://github.com/python/cpython/blob/2f8301cbf/Python/gc.c#L2085-L2102 --- pillow_heif/_pillow_heif.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pillow_heif/_pillow_heif.c b/pillow_heif/_pillow_heif.c index d13f1367..fed2abb4 100644 --- a/pillow_heif/_pillow_heif.c +++ b/pillow_heif/_pillow_heif.c @@ -959,7 +959,7 @@ static PyObject* _CtxImage_metadata(CtxImageObject* self, void* closure) { PyObject* meta_list = PyList_New(n_metas); if (!meta_list) { free(meta_ids); - return PyErr_NoMemory(); + return NULL; } for (int i = 0; i < n_metas; i++) { @@ -1030,7 +1030,7 @@ static PyObject* _CtxImage_thumbnails(CtxImageObject* self, void* closure) { PyObject* images_list = PyList_New(n_images); if (!images_list) { free(images_ids); - return PyList_New(0); + return NULL; } struct heif_image_handle* handle; @@ -1166,7 +1166,7 @@ static PyObject* _CtxImage_depth_image_list(CtxImageObject* self, void* closure) PyObject* images_list = PyList_New(n_images); if (!images_list) { free(images_ids); - return PyErr_NoMemory(); + return NULL; } for (int i = 0; i < n_images; i++) { @@ -1344,7 +1344,7 @@ static PyObject* _load_file(PyObject* self, PyObject* args) { if (!images_list) { free(images_ids); heif_context_free(heif_ctx); - return PyErr_NoMemory(); + return NULL; } enum heif_colorspace colorspace; From f94bbbc63aeb3b3eedb86d75c73f348f29ca2c65 Mon Sep 17 00:00:00 2001 From: John C Date: Fri, 11 Oct 2024 12:15:25 +0530 Subject: [PATCH 5/6] handle PyDict_New failures correctly A MemoryError is set when PyDict_New fails: https://github.com/python/cpython/blob/2f8301cbf/Objects/dictobject.c#L964-L970 https://github.com/python/cpython/blob/2f8301cbf/Objects/dictobject.c#L863-L887 --- pillow_heif/_pillow_heif.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/pillow_heif/_pillow_heif.c b/pillow_heif/_pillow_heif.c index fed2abb4..165f353f 100644 --- a/pillow_heif/_pillow_heif.c +++ b/pillow_heif/_pillow_heif.c @@ -894,8 +894,17 @@ static PyObject* _CtxImage_color_profile(CtxImageObject* self, void* closure) { return NULL; PyObject* result = PyDict_New(); + if (!result) { + heif_nclx_color_profile_free(nclx_profile); + return NULL; + } __PyDict_SetItemString(result, "type", PyUnicode_FromString("nclx")); PyObject* d = PyDict_New(); + if (!d) { + heif_nclx_color_profile_free(nclx_profile); + Py_DECREF(result); + return NULL; + } __PyDict_SetItemString(d, "color_primaries", PyLong_FromLong(nclx_profile->color_primaries)); __PyDict_SetItemString(d, "transfer_characteristics", PyLong_FromLong(nclx_profile->transfer_characteristics)); __PyDict_SetItemString(d, "matrix_coefficients", PyLong_FromLong(nclx_profile->matrix_coefficients)); @@ -914,6 +923,9 @@ static PyObject* _CtxImage_color_profile(CtxImageObject* self, void* closure) { } PyObject* result = PyDict_New(); + if (!result) { + return NULL; + } __PyDict_SetItemString( result, "type", PyUnicode_FromString(profile_type == heif_color_profile_type_rICC ? "rICC" : "prof")); size_t size = heif_image_handle_get_raw_color_profile_size(self->handle); @@ -972,6 +984,7 @@ static PyObject* _CtxImage_metadata(CtxImageObject* self, void* closure) { error = heif_image_handle_get_metadata(self->handle, meta_ids[i], data); if (error.code == heif_error_Ok) { meta_item_info = PyDict_New(); + // TODO handle meta_item_info == NULL __PyDict_SetItemString(meta_item_info, "type", PyUnicode_FromString(type)); __PyDict_SetItemString(meta_item_info, "content_type", PyUnicode_FromString(content_type)); __PyDict_SetItemString(meta_item_info, "data", PyBytes_FromStringAndSize((char*)data, size)); @@ -990,7 +1003,7 @@ static PyObject* _CtxImage_metadata(CtxImageObject* self, void* closure) { else if (self->image_type == PhHeifDepthImage) { PyObject* meta = PyDict_New(); if (!meta) { - Py_RETURN_NONE; + return NULL; } if (!self->depth_metadata) return meta; @@ -1389,10 +1402,19 @@ static PyObject* _load_file(PyObject* self, PyObject* args) { static PyObject* _get_lib_info(PyObject* self) { PyObject* lib_info_dict = PyDict_New(); + if (!lib_info_dict) { + return NULL; + } PyObject* encoders_dict = PyDict_New(); + if (!encoders_dict) { + Py_DECREF(lib_info_dict); + return NULL; + } PyObject* decoders_dict = PyDict_New(); - if ((!lib_info_dict) || (!encoders_dict) || (!decoders_dict)) { - return PyErr_NoMemory(); + if (!decoders_dict) { + Py_DECREF(encoders_dict); + Py_DECREF(lib_info_dict); + return NULL; } __PyDict_SetItemString(lib_info_dict, "libheif", PyUnicode_FromString(heif_get_version())); From 309997971ac628da3dbdc841a16c5e3f0498fb96 Mon Sep 17 00:00:00 2001 From: johncf Date: Sat, 12 Oct 2024 12:22:39 +0530 Subject: [PATCH 6/6] better handling of no-memory errors in _CtxImage_metadata --- pillow_heif/_pillow_heif.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/pillow_heif/_pillow_heif.c b/pillow_heif/_pillow_heif.c index 165f353f..82067a24 100644 --- a/pillow_heif/_pillow_heif.c +++ b/pillow_heif/_pillow_heif.c @@ -964,9 +964,9 @@ static PyObject* _CtxImage_metadata(CtxImageObject* self, void* closure) { return PyList_New(0); heif_item_id* meta_ids = (heif_item_id*)malloc(n_metas * sizeof(heif_item_id)); - if (!meta_ids) { + if (!meta_ids) return PyErr_NoMemory(); - } + n_metas = heif_image_handle_get_list_of_metadata_block_IDs(self->handle, NULL, meta_ids, n_metas); PyObject* meta_list = PyList_New(n_metas); if (!meta_list) { @@ -980,17 +980,25 @@ static PyObject* _CtxImage_metadata(CtxImageObject* self, void* closure) { content_type = heif_image_handle_get_metadata_content_type(self->handle, meta_ids[i]); size = heif_image_handle_get_metadata_size(self->handle, meta_ids[i]); data = malloc(size); - if (data) { - error = heif_image_handle_get_metadata(self->handle, meta_ids[i], data); - if (error.code == heif_error_Ok) { - meta_item_info = PyDict_New(); - // TODO handle meta_item_info == NULL - __PyDict_SetItemString(meta_item_info, "type", PyUnicode_FromString(type)); - __PyDict_SetItemString(meta_item_info, "content_type", PyUnicode_FromString(content_type)); - __PyDict_SetItemString(meta_item_info, "data", PyBytes_FromStringAndSize((char*)data, size)); + if (!data) { + Py_DECREF(meta_list); + free(meta_ids); + return PyErr_NoMemory(); + } + error = heif_image_handle_get_metadata(self->handle, meta_ids[i], data); + if (error.code == heif_error_Ok) { + meta_item_info = PyDict_New(); + if (!meta_item_info) { + free(data); + Py_DECREF(meta_list); + free(meta_ids); + return NULL; } - free(data); + __PyDict_SetItemString(meta_item_info, "type", PyUnicode_FromString(type)); + __PyDict_SetItemString(meta_item_info, "content_type", PyUnicode_FromString(content_type)); + __PyDict_SetItemString(meta_item_info, "data", PyBytes_FromStringAndSize((char*)data, size)); } + free(data); if (!meta_item_info) { meta_item_info = Py_None; Py_INCREF(meta_item_info); @@ -1002,9 +1010,8 @@ static PyObject* _CtxImage_metadata(CtxImageObject* self, void* closure) { } else if (self->image_type == PhHeifDepthImage) { PyObject* meta = PyDict_New(); - if (!meta) { + if (!meta) return NULL; - } if (!self->depth_metadata) return meta;