mm2 / Little-CMS

A free, open source, CMM engine. It provides fast transforms between ICC profiles.
https://www.littlecms.com
MIT License
571 stars 176 forks source link

lcms2-2.16 crashes transforming sRGB float to planar uint16 #420

Closed cgohlke closed 11 months ago

cgohlke commented 11 months ago

Hello. First of all, thank you for developing and maintaining this library!

After upgrading to lcms2-2.16 from 2.15, I get reproducible crashes (Windows fatal exception: access violation) during the tests of the imagecodecs Python library.

lcms2-2.16 was built from source using the VS2022 solutions on Windows 11. Visual Studio 2022 is at version 17.8.3.

The crash occurs when transforming float32 or float64 sRGB images to uint16 planar sRGB at https://github.com/mm2/Little-CMS/blob/453bafeb85b4ef96498866b7a8eadcc74dff9223/src/cmspack.c#L3038

The call stack is:

[0x0]   _cms_cp311_win_amd64!PackWordsFromFloat+0x3c0   0xd31abeb530   0x7ff8c044aeb7   
[0x1]   _cms_cp311_win_amd64!FloatXFORM+0x177   0xd31abeb5c0   0x7ff8c044c416   
[0x2]   _cms_cp311_win_amd64!cmsDoTransform+0x56   0xd31abeb700   0x7ff8c04b4c5c   
[0x3]   _cms_cp311_win_amd64!__pyx_pf_11imagecodecs_4_cms_4cms_transform+0x2d4c   0xd31abeb760   0x7ff8c04b5e53   

This is related to commit https://github.com/mm2/Little-CMS/commit/4c0c66e7109a903c470159ecd3d753e8a4d56c79.

lcms2-2.15 or other transforms (for example, to non-planar or uint8) do not crash.

If necessary, I can try to create a standalone example or debug build.

Unrelated: the CPython headers define a conflicting T_FLOAT macro at https://github.com/python/cpython/blob/v3.12.1/Include/structmember.h#L27. Hence, the lcms2 T_FLOAT macro cannot be used in Python bindings.

mm2 commented 11 months ago

Thanks for reporting. I'm taking a look and eventually could create a 2.16.1 to fix the issue

cgohlke commented 11 months ago

The stride needs to be corrected like in the other, similar functions: Stride /= PixelSize(info->OutputFormat); .

I see it's already fixed in master. Thank you.

mm2 commented 11 months ago

Should I create a 2.16.1? I realize that could be a stopper for your package. Just let me know.

cgohlke commented 11 months ago

Not really a showstopper for me. I can build against a patched version of lcms2 if necessary. On the other hand, known access violations might pose a security risk so it's probably a good idea to release the fix soonish.

cgohlke commented 11 months ago

Are there any known limitations using half floats (float16) in lcms? I get some crashes during the imagecodecs tests when enabling half floats. Is that worth investigating and opening another issue?

cgohlke commented 11 months ago

This fixes the segfaults with float16 formats for me:

diff --git a/src/cmspack.c b/src/cmspack.c
index c191896..a594a3d 100644
--- a/src/cmspack.c
+++ b/src/cmspack.c
@@ -3426,7 +3428,7 @@ cmsUInt8Number* UnrollHalfToFloat(_cmsTRANSFORM* info,
     cmsUInt32Number i, start = 0;
     cmsFloat32Number maximum = IsInkSpace(info ->InputFormat) ? 100.0F : 1.0F;

-    Stride /= PixelSize(info->OutputFormat);
+    Stride /= PixelSize(info->InputFormat);

     if (ExtraFirst)
             start = Extra;
cgohlke commented 11 months ago

Are there any known limitations using half floats (float16) in lcms?

All transforms seem to work now from/to half float, except output to planar half float, which fails with "Unsupported raster format".

mm2 commented 11 months ago

This fixes the segfaults with float16 formats for me:

That is a bug, thanks for finding it. I'm fixing it in e88056ef81e42cd1fe06945da9d44ac95c80fa48

mm2 commented 11 months ago

All transforms seem to work now from/to half float, except output to planar half float, which fails with "Unsupported raster format".

yes, that's not implemented.