From b9a7445296a650c7639efe1f71c737ccfa1630e6 Mon Sep 17 00:00:00 2001 From: Kevin Murray Date: Fri, 27 Jun 2014 21:26:58 +1000 Subject: [PATCH 1/5] [skimg.io._plugins.freeimage_plugin] fix segfault I have moved the freeimage error handler callback function to the module namespace to prevent it being garbage collected. See the following for more info on this quirk of ctypes: http://stackoverflow.com/questions/12995925/how-to-prevent-functype-from-being-collected https://github.com/JohannesBuchner/PyMultiNest/issues/5 This also changes the way FreeImage errors are handled. If an exception is raised in a callback, it will not propagate beyond ctypes internals. Now, we use a callback that sets a global variable to indicate error. We then check for error and reset the error string to NULL every time the C api is called. This is the only way we can both: a) Not segfault on freeimage error b) Pass the freeimage error to the user c) raise RuntimeError() --- skimage/io/_plugins/freeimage_plugin.py | 62 +++++++++++++++++++------ 1 file changed, 49 insertions(+), 13 deletions(-) diff --git a/skimage/io/_plugins/freeimage_plugin.py b/skimage/io/_plugins/freeimage_plugin.py index a26125fa..07e8d313 100644 --- a/skimage/io/_plugins/freeimage_plugin.py +++ b/skimage/io/_plugins/freeimage_plugin.py @@ -34,15 +34,29 @@ def _generate_candidate_libs(): return lib_dirs, lib_paths +if sys.platform == 'win32': + LOADER = ctypes.windll + FUNCTYPE = ctypes.WINFUNCTYPE +else: + LOADER = ctypes.cdll + FUNCTYPE = ctypes.CFUNCTYPE + +def handle_errors(): + global FT_ERROR_STR + if FT_ERROR_STR: + tmp = FT_ERROR_STR + FT_ERROR_STR = None + raise RuntimeError(tmp) + +FT_ERROR_STR = None +# This MUST happen in module scope, or the function pointer is garbage +# collected, leading to a segfault when error_handler is called. +@FUNCTYPE(None, ctypes.c_int, ctypes.c_char_p) +def c_error_handler(fif, message): + global FT_ERROR_STR + FT_ERROR_STR = 'FreeImage error: %s' % message def load_freeimage(): - if sys.platform == 'win32': - loader = ctypes.windll - functype = ctypes.WINFUNCTYPE - else: - loader = ctypes.cdll - functype = ctypes.CFUNCTYPE - freeimage = None errors = [] # First try a few bare library names that ctypes might be able to find @@ -54,7 +68,7 @@ def load_freeimage(): lib_paths = bare_libs + lib_paths for lib in lib_paths: try: - freeimage = loader.LoadLibrary(lib) + freeimage = LOADER.LoadLibrary(lib) break except Exception: if lib not in bare_libs: @@ -81,11 +95,7 @@ def load_freeimage(): '\n'.join(lib_dirs)) # FreeImage found - @functype(None, ctypes.c_int, ctypes.c_char_p) - def error_handler(fif, message): - raise RuntimeError('FreeImage error: %s' % message) - - freeimage.FreeImage_SetOutputMessage(error_handler) + freeimage.FreeImage_SetOutputMessage(c_error_handler) return freeimage _FI = load_freeimage() @@ -189,13 +199,17 @@ class FI_TYPES(object): @classmethod def get_type_and_shape(cls, bitmap): w = _FI.FreeImage_GetWidth(bitmap) + handle_errors() h = _FI.FreeImage_GetHeight(bitmap) + handle_errors() fi_type = _FI.FreeImage_GetImageType(bitmap) + handle_errors() if not fi_type: raise ValueError('Unknown image pixel type') dtype = cls.dtypes[fi_type] if fi_type == cls.FIT_BITMAP: bpp = _FI.FreeImage_GetBPP(bitmap) + handle_errors() if bpp == 8: extra_dims = [] elif bpp == 24: @@ -377,9 +391,11 @@ class METADATA_DATATYPE(object): def _process_bitmap(filename, flags, process_func): filename = asbytes(filename) ftype = _FI.FreeImage_GetFileType(filename, 0) + handle_errors() if ftype == -1: raise ValueError('Cannot determine type of file %s' % filename) bitmap = _FI.FreeImage_Load(ftype, filename, flags) + handle_errors() bitmap = ctypes.c_void_p(bitmap) if not bitmap: raise ValueError('Could not load file %s' % filename) @@ -387,6 +403,7 @@ def _process_bitmap(filename, flags, process_func): return process_func(bitmap) finally: _FI.FreeImage_Unload(bitmap) + handle_errors() def read(filename, flags=0): @@ -414,6 +431,7 @@ def read_metadata(filename): def _process_multipage(filename, flags, process_func): filename = asbytes(filename) ftype = _FI.FreeImage_GetFileType(filename, 0) + handle_errors() if ftype == -1: raise ValueError('Cannot determine type of file %s' % filename) create_new = False @@ -422,14 +440,17 @@ def _process_multipage(filename, flags, process_func): multibitmap = _FI.FreeImage_OpenMultiBitmap(ftype, filename, create_new, read_only, keep_cache_in_memory, flags) + handle_errors() multibitmap = ctypes.c_void_p(multibitmap) if not multibitmap: raise ValueError('Could not open %s as multi-page image.' % filename) try: pages = _FI.FreeImage_GetPageCount(multibitmap) + handle_errors() out = [] for i in range(pages): bitmap = _FI.FreeImage_LockPage(multibitmap, i) + handle_errors() bitmap = ctypes.c_void_p(bitmap) if not bitmap: raise ValueError('Could not open %s as a multi-page image.' @@ -438,9 +459,11 @@ def _process_multipage(filename, flags, process_func): out.append(process_func(bitmap)) finally: _FI.FreeImage_UnlockPage(multibitmap, bitmap, False) + handle_errors() return out finally: _FI.FreeImage_CloseMultiBitmap(multibitmap, 0) + handle_errors() def read_multipage(filename, flags=0): @@ -469,6 +492,7 @@ def _wrap_bitmap_bits_in_array(bitmap, shape, dtype): """ pitch = _FI.FreeImage_GetPitch(bitmap) + handle_errors() height = shape[-1] byte_size = height * pitch itemsize = dtype.itemsize @@ -478,6 +502,7 @@ def _wrap_bitmap_bits_in_array(bitmap, shape, dtype): else: strides = (itemsize, pitch) bits = _FI.FreeImage_GetBits(bitmap) + handle_errors() array = numpy.ndarray(shape, dtype=dtype, buffer=(ctypes.c_char * byte_size).from_address(bits), strides=strides) @@ -502,6 +527,7 @@ def _array_from_bitmap(bitmap): g = n(array[1]) r = n(array[2]) if shape[0] == 3: + handle_errors() return numpy.dstack((r, g, b)) elif shape[0] == 4: a = n(array[3]) @@ -523,6 +549,7 @@ def _read_metadata(bitmap): for model_name, number in models: mdhandle = _FI.FreeImage_FindFirstMetadata(number, bitmap, ctypes.byref(tag)) + handle_errors() mdhandle = ctypes.c_void_p(mdhandle) if mdhandle: more = True @@ -530,8 +557,10 @@ def _read_metadata(bitmap): tag_name = asstr(_FI.FreeImage_GetTagKey(tag)) tag_type = _FI.FreeImage_GetTagType(tag) byte_size = _FI.FreeImage_GetTagLength(tag) + handle_errors() char_ptr = ctypes.c_char * byte_size tag_str = char_ptr.from_address(_FI.FreeImage_GetTagValue(tag)) + handle_errors() if tag_type == METADATA_DATATYPE.FIDT_ASCII: tag_val = asstr(tag_str.value) else: @@ -541,7 +570,9 @@ def _read_metadata(bitmap): tag_val = tag_val[0] metadata[(model_name, tag_name)] = tag_val more = _FI.FreeImage_FindNextMetadata(mdhandle, ctypes.byref(tag)) + handle_errors() _FI.FreeImage_FindCloseMetadata(mdhandle) + handle_errors() return metadata @@ -556,6 +587,7 @@ def write(array, filename, flags=0): array = numpy.asarray(array) filename = asbytes(filename) ftype = _FI.FreeImage_GetFIFFromFilename(filename) + handle_errors() if ftype == -1: raise ValueError('Cannot determine type for %s' % filename) bitmap, fi_type = _array_to_bitmap(array) @@ -563,16 +595,20 @@ def write(array, filename, flags=0): if fi_type == FI_TYPES.FIT_BITMAP: can_write = _FI.FreeImage_FIFSupportsExportBPP(ftype, _FI.FreeImage_GetBPP(bitmap)) + handle_errors() else: can_write = _FI.FreeImage_FIFSupportsExportType(ftype, fi_type) + handle_errors() if not can_write: raise TypeError('Cannot save image of this format ' 'to this file type') res = _FI.FreeImage_Save(ftype, bitmap, filename, flags) + handle_errors() if not res: raise RuntimeError('Could not save image properly.') finally: _FI.FreeImage_Unload(bitmap) + handle_errors() def write_multipage(arrays, filename, flags=0): From d69cdb951bae4e6e6eee03c64f4851230e5ef4f2 Mon Sep 17 00:00:00 2001 From: Kevin Murray Date: Fri, 27 Jun 2014 21:32:18 +1000 Subject: [PATCH 2/5] [freeimage tests] Add a test w/ truncated image This tests tests the fix in the previous commit b9a7445, covering the issue noted in issue #1037. --- skimage/data/truncated.jpg | Bin 0 -> 400 bytes skimage/io/tests/test_freeimage.py | 7 +++++++ 2 files changed, 7 insertions(+) create mode 100644 skimage/data/truncated.jpg diff --git a/skimage/data/truncated.jpg b/skimage/data/truncated.jpg new file mode 100644 index 0000000000000000000000000000000000000000..11bb8140594261069f318623950704d8a075f9be GIT binary patch literal 400 zcmex=iF;N$`UAd82aiwDF383NJD#LCRf%Eivc4pu@E@&5pWAO}MVLkcsa5(ASU zBeNjm|04|YKzFi&odL?6mQqXwbzED#l4gO`Kd};u4Zls%q*Qnp!5NX66=_R?aT2 zZtfnQUcn)uVc`*xQOPN(Y3Ui6S;Zx#W#tu>Rn0A}ZS5VMU6UqHnL2IyjG40*Enc#8 z+42=DS8dw7W$U)>J9h3mboj{8W5-XNJay^vm8;jT-?(|};iJb-o<4j2;^nK4pFV&2 Q`tAFVpT9u Date: Fri, 27 Jun 2014 21:42:09 +1000 Subject: [PATCH 3/5] [test_freeimage.py] remove extraneous statements --- skimage/io/tests/test_freeimage.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/skimage/io/tests/test_freeimage.py b/skimage/io/tests/test_freeimage.py index f0d19ab3..38ba725f 100644 --- a/skimage/io/tests/test_freeimage.py +++ b/skimage/io/tests/test_freeimage.py @@ -39,8 +39,6 @@ def test_imread(): @skipif(not FI_available) def test_imread_truncated_jpg(): - raised = False - raised = True assert_raises(RuntimeError, sio.imread, os.path.join(si.data_dir, 'truncated.jpg')) From 83d0717986f66b47ecc323328befdcb50a67fe54 Mon Sep 17 00:00:00 2001 From: Kevin Murray Date: Mon, 30 Jun 2014 18:47:31 +1000 Subject: [PATCH 4/5] [test_freeimage] Allow ValueError to be raised With the truncated image, sometimes ValueError is raised. This allows either RuntimeError (i.e. libfreeimage error) or ValueError to be raised. --- skimage/io/tests/test_freeimage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skimage/io/tests/test_freeimage.py b/skimage/io/tests/test_freeimage.py index 38ba725f..4e64bd16 100644 --- a/skimage/io/tests/test_freeimage.py +++ b/skimage/io/tests/test_freeimage.py @@ -39,7 +39,7 @@ def test_imread(): @skipif(not FI_available) def test_imread_truncated_jpg(): - assert_raises(RuntimeError, + assert_raises((RuntimeError, ValueError), sio.imread, os.path.join(si.data_dir, 'truncated.jpg')) From 4160a67a42829d4c8322a108f4bf6d5ec77b7954 Mon Sep 17 00:00:00 2001 From: Kevin Murray Date: Mon, 30 Jun 2014 19:35:35 +1000 Subject: [PATCH 5/5] [skimage.io.tests] add truncated jpg imread tests --- skimage/io/tests/test_imread.py | 5 +++++ skimage/io/tests/test_pil.py | 4 ++++ skimage/io/tests/test_simpleitk.py | 6 ++++++ 3 files changed, 15 insertions(+) diff --git a/skimage/io/tests/test_imread.py b/skimage/io/tests/test_imread.py index afb9eac9..206b800d 100644 --- a/skimage/io/tests/test_imread.py +++ b/skimage/io/tests/test_imread.py @@ -37,6 +37,11 @@ def test_imread_palette(): img = imread(os.path.join(data_dir, 'palette_color.png')) assert img.ndim == 3 +@skipif(not imread_available) +def test_imread_truncated_jpg(): + assert_raises((RuntimeError, ValueError), + sio.imread, + os.path.join(si.data_dir, 'truncated.jpg')) @skipif(not imread_available) def test_bilevel(): diff --git a/skimage/io/tests/test_pil.py b/skimage/io/tests/test_pil.py index 243d2fd1..89147fd6 100644 --- a/skimage/io/tests/test_pil.py +++ b/skimage/io/tests/test_pil.py @@ -95,6 +95,10 @@ def test_repr_png(): assert np.all(original_img == round_trip) +@skipif(not PIL_available) +def test_imread_truncated_jpg(): + assert_raises((IOError, ValueError), imread, + os.path.join(data_dir, 'truncated.jpg')) # Big endian images not correctly loaded for PIL < 1.1.7 # Renable test when PIL 1.1.7 is more common. diff --git a/skimage/io/tests/test_simpleitk.py b/skimage/io/tests/test_simpleitk.py index bf3d3614..89f7416f 100644 --- a/skimage/io/tests/test_simpleitk.py +++ b/skimage/io/tests/test_simpleitk.py @@ -51,6 +51,12 @@ def test_bilevel(): np.testing.assert_array_equal(img, expected) +@skipif(not sitk_available) +def test_imread_truncated_jpg(): + assert_raises((RuntimeError, ValueError), + sio.imread, + os.path.join(si.data_dir, 'truncated.jpg')) + @skipif(not sitk_available) def test_imread_uint16(): expected = np.load(os.path.join(data_dir, 'chessboard_GRAY_U8.npy'))