oss-fuzz 35265: graphicsmagick:coder_PGX_fuzzer: Out-of-memory in coder_PGX_fuzzer

[bobfriesenhahn]

It seems that the PGX decoder has an issue which allows a 197 byte PGX file to consume all of the memory on the system and then fail to allocate more memory. The oss-fuzz test case is attached.

oss-fuzz-35265.tar.gz

[mdadams]

I cannot reproduce this problem. I simply get a decode failure when I try to decode with imginfo. Also, this image file does not appear to be a PGX file, as far as I can tell. It is recognized by both "file" and JasPer as a JP2/JPC code stream. Please note that "out of memory" is not an error, unless JasPer is failing to honor the upper bound on memory specified for the library (which is controlled by JAS_DEC_DEFAULT_MAX_SAMPLES as well as codec options). Is JasPer failing to honor the specified memory limit?

[bobfriesenhahn]

On Sun, 20 Jun 2021, Michael Adams wrote:

I cannot reproduce this problem. I simply get a decode failure when I try to decode with imginfo. Also, this image file does not appear to be a PGX file, as far as I can tell. It is recognized by both "file" and JasPer as a JP2/JPC code stream. Please note that "out of memory" is not an error, unless JasPer is failing to honor the upper bound on memory specified for the library (which is controlled by JAS_DEC_DEFAULT_MAX_SAMPLES as well as codec options). Is JasPer failing to honor the specified memory limit?

Does Jasper now provide a working memory limit facility via its API?

It appears that jpc_dec_decodepkts is consuming 1,540,313,921 bytes of memory:

% LD_PRELOAD=/lib/x86_64-linux-gnu/libmemusage.so gm convert pgx:clusterfuzz-testcase-minimized-coder_PGX_fuzzer-4787980765102080 info:- warning: trailing garbage in marker segment (1 bytes) warning: trailing garbage in marker segment (2 bytes) jpc_dec_decodepkts failed gm convert: Unable to decode image file (clusterfuzz-testcase-minimized-coder_PGX_fuzzer-4787980765102080).

Memory usage summary: heap total: 1540381704, heap peak: 1540313921, stack peak: 21600 total calls total memory failed calls malloc| 10850020 1540378632 0 realloc| 3 3072 0 (nomove:0, dec:0, free:0) calloc| 0 0 0 free| 16273252 1540377217 Histogram for block sizes: 0-15 5423295 49% ================================================== 16-31 1347 <1% 32-47 42 <1% 48-63 10 <1% 64-79 8 <1% 80-95 5423255 49% ================================================= 96-111 12 <1% 112-127 3 <1% 128-143 14 <1% 160-175 1 <1% 176-191 1 <1% 192-207 5 <1% 208-223 2 <1% 256-271 2 <1% 432-447 1 <1% 544-559 3 <1% 768-783 1 <1% 896-911 2 <1% 912-927 1 <1% 1024-1039 10 <1% 1264-1279 2 <1% 1328-1343 1 <1% 1456-1471 1 <1% 2512-2527 1 <1% 4096-4111 2 <1% 5312-5327 1 <1% 5760-5775 2 <1% 6720-6735 1 <1% 6864-6879 2 <1% 8192-8207 2 <1% 8208-8223 1 <1% 8528-8543 2 <1% 32816-32831 1 <1% 47808-47823 1 <1% large 1988 <1%

-- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

One can restrict the number of samples in an image to be decoded. This can be used to bound memory usage for decoding. A default value for the limit is set via JAS_DEC_DEFAULT_MAX_SAMPLES when the library is built. This value can also be overridden via an option when decoding. If my memory serves me well, this option is called max_samples for all of the decoders. If the value of JAS_DEC_DEFAULT_MAX_SAMPLES is set larger than what your application wants, you can use the preceding decoder option to set a more desirable limit.

[bobfriesenhahn]

The 'max_samples' option appears to be completely undocumented. Setting it in options passed to the decoder does appear to do something useful. However, it is unclear that a "sample" is. What is the meaning of a "sample"? Is a "sample" a pixel, or a pixel component? If it is a pixel component, how can the using software know how many components there would be prior to requesting that JasPer decode the image?

For example, a user might care about the number of "pixels" but not care about a monochrome vs color image.

[bobfriesenhahn]

After some testing I am still not sure that a "sample" is. Samples to not appear to be directly related to pixel dimensions.

[bobfriesenhahn]

It would be useful if the Jasper library API were to offer a way to register callbacks to replace the standard memory allocation functions (e.g. malloc, calloc, realloc, free) as some other libraries like libpng do. If the application using Jasper performs resource accounting in its memory allocator, then it can deny an allocation request based on the resource usage of the whole application. This is more useful than providing arbitrary limits to just one part of the decoding chain.

[mdadams]

@bobfriesenhahn I have added a tentative interface for allowing applications to specify a custom allocator for the JasPer library via the newly added function jas_init_custom. To specify a custom allocator, initialize the library with jas_init_custom instead of jas_init. In addition, I have tried to fix a related issue, which is allowing the default maximum number of samples (which is specified at build time) to be overridden at run time. Since a value of zero for this parameter means "no limit", you can provide an allocator of your own and set the default maximum number of samples to zero when you call jas_init_custom. Then, I think that you should be okay from there, if I have understood what you need/want. Can you please try out the most recent commit on the mdadams-callbacks branch and let me know if it would be usable for you. This is the branch where I have placed all of my changes. I have done some limited testing and the code seems to be working okay. If it meets your needs, I can merge this to the master branch. Also, if you have any feedback or suggestions for improvement for this interface, please let me know (since it is difficult to change after it is merged and released).

You can see some examples of using jas_init_custom in the example applications in the directory src/appl (such as imginfo, imgcmp, and jasper).

Note: If user-specified allocators are used in a multithreaded application, it is assumed that the allocators internally provide any necessary locking (since JasPer does not itself do any locking).

[mdadams]

Hi Bob,

On Sat, 26 Jun 2021, Bob Friesenhahn wrote:

The 'max_samples' option appears to be completely undocumented. Setting it in options passed to the decoder does appear to do something useful. However, it is unclear that a "sample" is. What is the meaning of a "sample"? Is a "sample" a pixel, or a pixel component? If it is a pixel component, how can the using software know how many components there would be prior to requesting that JasPer decode the image?

"Sample" simply means "sample value" in the signal processing sense (i.e., a value for a single component of an image at a single position).

The number of samples in an image is simply the number of sample values for all of the image components taken together. For example, if all N components of the image are sampled at the same resolution of width W and height H, the total number of samples is W H N.

--Michael

[bobfriesenhahn]

It seems that max_samples is not as useful as hoped for. The new jas_init_custom() interface sounds very useful and I hope to have tried it within a week.

GraphicsMagick Jasper-related code needs some more care since it is not currently assuring that the decoder used is for the format explicitly requested. For example, the user might have requested to decode a "JP2" file but the file has a PGX header and is thus decoded as a PGX file. This violates the normal expectations in GraphicsMagick so I will be fixing that too.

[bobfriesenhahn]

Today I cloned the mdadams-callbacks branch and tried it out. I updated GraphicsMagick sources to optionally use this feature if the jas_init_custom() function is available. Unfortunately, I discovered that GraphicsMagick only encounters memory corruption and core dumps while using this branch. It did not matter if the code used jas_init() or jas_init_custom().

I observed quite a few compilation warnings while compiling JasPer. There is even a warning due to a non-ANSI prototype.

When I did 'make test' I found that the tests failed unless I first did 'make install'. The reason for this appears to be that one or more of the installed libraries has an encoded run-path (so it is discovered without being in the default library search path) and the JasPer test suite is not assuring that the library it just built is the one used while testing.

[mdadams]

Hi Bob,

On Sat, 3 Jul 2021, Bob Friesenhahn wrote:

Today I cloned the mdadams-callbacks branch and tried it out. I updated GraphicsMagick sources to optionally use this feature if the jas_init_custom() function is available. Unfortunately, I discovered that GraphicsMagick only encounters memory corruption and core dumps while using this branch. It did not matter if the code used jas_init() or jas_init_custom().

I observed quite a few compilation warnings while compiling JasPer. There is even a warning due to a non-ANSI prototype.

When I did 'make test' I found that the tests failed unless I first did 'make install'. The reason for this appears to be that one or more of the installed libraries has an encoded run-path (so it is discovered without being in the default library search path) and the JasPer test suite is not assuring that the library it just built is the one used while testing.

I am sorry that I missed the compiler warnings. I have fixed them and one other problem that I noticed, which should have been benign if jas_init_custom is used correctly (and shouldn't cause a crash in any case). If you use the code in the most recent commit of the mdadams-callbacks branch, this should hopefully eliminate the compiler warnings. Please confirm that the warnings are now gone for you. At least, my version of GCC isn't giving any warnings anymore.

As for the crashes with the new code, this is strange. The all of the tests in the test suite pass okay for the environments in which I have tested the mdadams-callbacks branch. I tested using jas_init_custom as well as using jas_init. Both worked. Are you sure that you are using the mdadams-callbacks version of the JasPer shared library with your application program? If not things will die. (See note below.) Also, what settings/options are you using with CMake when building JasPer? What compiler version and OS version are you using when things fail?

IMPORTANT NOTE: Please note that version of JasPer on the mdadams-callbacks branch is most definitely not ABI compatible with the version on the master branch. So, if you try to swap in one JasPer shared library with another, this will fail very badly. I did not change the versioning information for the JasPer library because I cannot make an official release of code not on the master branch. So, you may well run into problems with the wrong shared library version being used if you already have JasPer installed (since library version numbering will likely lie and say that the two versions are ABI compatible). To be safe, you ought to bump the major version number on the library before you build it. Also, ensure that you are compiling and (dynamically) linking against the mdadams-callbacks version of the library. My guess is that this explains the problem that you are having. Can you confirm this?

--Michael

[mdadams]

@bobfriesenhahn I'm sorry, but I forgot to push the commit (that fixed the compiler warnings). I just pushed it now. The commit is bea5945bb59e09f26886e4d65cf8d136515c1fca.

[bobfriesenhahn]

On Sat, 3 Jul 2021, Michael Adams wrote:

I am sorry that I missed the compiler warnings. I have fixed them and one other problem that I noticed, which should have been benign if jas_init_custom is used correctly (and shouldn't cause a crash in any case). If you use the code in the most recent commit of the mdadams-callbacks branch, this should hopefully eliminate the compiler warnings. Please confirm that the warnings are now gone for you. At least, my version of GCC isn't giving any warnings anymore.

I still see lots of warnings of the form:

home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/base/jas_image.c: In function 'jas_image_readcmpt': /home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/base/jas_image.c:546:47: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if (jas_stream_read(stream, buffer, width) != width)

As for the crashes with the new code, this is strange. The all of the tests in the test suite pass okay for the environments in which I have tested the mdadams-callbacks branch. I tested using jas_init_custom as well as using jas_init. Both worked. Are you sure that you are using the mdadams-callbacks version of the JasPer shared library with your application program? If not things will die. (See note below.) Also, what settings/options are you using with CMake when building JasPer? What compiler version and OS version are you using when things fail?

This is using GCC 7.5 under OmniOSce (an Illumos Solaris derivative).

I ran cmake like:

cmake -G "Unix Makefiles" -H/home/bfriesen/src/graphics/jasper-callbacks.git -B. -DCMAKE_INSTALL_PREFIX=/opt/local -DJAS_ENABLE_OPENGL=false

The above feels exceedingly clumsy.

likely lie and say that the two versions are ABI compatible). To be safe, you ought to bump the major version number on the library before you build it. Also, ensure that you are compiling and (dynamically) linking against the mdadams-callbacks version of the library. My guess is that this explains the problem that you are having. Can you confirm this?

I just re-did everything with your latest changes plus the SO bump and the GraphicsMagick tests pertaining to JasPer still all fail. It fails if jas_init() is used, and if jas_init_custom() is used.

% ldd utilities/gm ... libjasper.so.5 => /opt/local/lib/libjasper.so.5

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Hi Bob,

On Sat, 3 Jul 2021, Bob Friesenhahn wrote:

I still see lots of warnings of the form: home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/base/jas_imag e.c: In function 'jas_image_readcmpt': /home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/base/jas_ima ge.c:546:47: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if (jas_stream_read(stream, buffer, width) != width)

I did not change anything that should cause any new warnings in this code. So, I suspect that these warnings will also be present for you if you use the master branch as well (and should be benign). That is, I don't think that the warnings are due to my callback-related changes.

This is using GCC 7.5 under OmniOSce (an Illumos Solaris derivative).

I am at a loss to explain why things are crashing for you and not for me. The only things that comes to mind are either a shared library problem or platform-specific bug (or the master branch never worked in the first place)? Incidentally, what processor type does this machine have and is it 32-bit or 64-bit?

I ran cmake like: cmake -G "Unix Makefiles" -H/home/bfriesen/src/graphics/jasper-callbacks.git -B. -DCMAKE_INSTALL_PREFIX=/opt/local -DJAS_ENABLE_OPENGL=false

This looks reasonable. If you are on a Unix system, you can probably omit the -G option, since I think that using make as the native build tool is the default on such Unix systems. I also tried with the same -DJAS_ENABLE_OPENGL=false option and all of the test cases still pass. So, it is not the particular option that you are using. (I presume that you did not change the CMakeLists.txt file.) It would probably also be quite helpful to add: -DJAS_ENABLE_ASAN=true -DJAS_ENABLE_USAN=true (I am assuming that Address Sanitizer is available on your system.)

the GraphicsMagick tests pertaining to JasPer still all fail. It fails if jas_init() is used, and if jas_init_custom() is used.

This is very strange. There are two things that you can try:

1) Try building JasPer with a static library instead of a shared library and see what happens. This will allow us to determine whether the problem is related to shared libraries. (Use -DJAS_ENABLE_SHARED=false when you run cmake.)

2) If item 1 does not work, take your new code that uses jas_init with the mdadams-callback branch code (which crashes), and (without modification) try your code with the most recent commit on the master branch. This is basically a sanity check just to ensure that the bug is not something inherited from the master branch.

Let me know what happens. Thanks.

--Michael

[bobfriesenhahn]

On Sat, 3 Jul 2021, Michael Adams wrote:

This is using GCC 7.5 under OmniOSce (an Illumos Solaris derivative).

I am at a loss to explain why things are crashing for you and not for me. The only things that comes to mind are either a shared library problem or platform-specific bug (or the master branch never worked in the first place)? Incidentally, what processor type does this machine have and is it 32-bit or 64-bit?

Is is on Intel 64-bit Xeon hardware (20 core, 40 threads). In this particular case, the compiler produces 64-bit binaries by default.

While the Solaris ancestry may possibly provide some different definitions than Linux, in actual practice behavior has been the same.

So, it is not the particular option that you are using. (I presume that you did not change the CMakeLists.txt file.) It would probably also be quite helpful to add: -DJAS_ENABLE_ASAN=true -DJAS_ENABLE_USAN=true (I am assuming that Address Sanitizer is available on your system.)

Unfortunately, ASAN and UBSAN are not available on this system. :-(

I will try the same build on Ubuntu 18 to see if it occurs there as well. I have lots more tools there.

2) If item 1 does not work, take your new code that uses jas_init with the mdadams-callback branch code (which crashes), and (without modification) try your code with the most recent commit on the master branch. This is basically a sanity check just to ensure that the bug is not something inherited from the master branch.

Using the head of the master branch there is no problem with the new code.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[bobfriesenhahn]

Is this the backtrace I get from gdb under Linux. This build should be only using the jas_init() interface. Please note that GraphicsMagick uses the jas_stream_t stream manager interface:

(gdb) bt

0 __GI_raise @.***=6) at ../sysdeps/unix/sysv/linux/raise.c:51

1 0x00007f1ef874e9fb in __GI_abort () at abort.c:100

2 0x000055c09474354c in MagickPanicSignalHandler (signo=6) at /home/bfriesen/src/graphics/GM/magick/magick.c:873

3

4 __GI_raise @.***=6) at ../sysdeps/unix/sysv/linux/raise.c:51

5 0x00007f1ef874e921 in __GI_abort () at abort.c:79

6 0x00007f1ef8797967 in __libc_message @.=do_abort, @.=0x7f1ef88c4b0d "%s\n") at ../sysdeps/posix/libc_fatal.c:181

7 0x00007f1ef879e9da in malloc_printerr @.***=0x7f1ef88c2d08 "free(): invalid pointer") at malloc.c:5342

8 0x00007f1ef87a5f0c in _int_free (have_lock=0, p=0x55c09583c060, av=0x7f1ef8af9c40 ) at malloc.c:4167

9 __GI___libc_free (mem=0x55c09583c070) at malloc.c:3134

10 0x00007f1efa4e43fc in jas_bma_free (ptr=0x55c09583c090) at /home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/base/jas_malloc.c:299

11 0x00007f1efa4e3e9b in jas_free (ptr=0x55c09583c090) at /home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/base/jas_malloc.c:145

12 0x00007f1efa4e628f in jas_stream_destroy (stream=0x55c09583c090) at /home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/base/jas_stream.c:607

13 0x00007f1efa4e62f1 in jas_stream_close (stream=0x55c09583c090) at /home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/base/jas_stream.c:620

14 0x000055c09493ad67 in ReadJP2Image (image_info=0x55c095837cd0, exception=0x7fff24a89450) at /home/bfriesen/src/graphics/GM/coders/jp2.c:929

15 0x000055c094711870 in ReadImage (image_info=0x55c095833b50, exception=0x7fff24a89450) at /home/bfriesen/src/graphics/GM/magick/constitute.c:1630

16 0x000055c0946d9718 in ConvertImageCommand (image_info=0x55c095833b50, argc=3, argv=0x55c095835cb0, metadata=0x0, exception=0x7fff24a89450)

 at /home/bfriesen/src/graphics/GM/magick/command.c:4416

17 0x000055c0946e891a in MagickCommand (image_info=0x55c095833b50, argc=3, argv=0x7fff24a89de0, metadata=0x7fff24a89438, exception=0x7fff24a89450)

 at /home/bfriesen/src/graphics/GM/magick/command.c:8916

18 0x000055c09470345f in GMCommandSingle (argc=3, argv=0x7fff24a89de0) at /home/bfriesen/src/graphics/GM/magick/command.c:17454

19 0x000055c09470358a in GMCommand (argc=4, argv=0x7fff24a89dd8) at /home/bfriesen/src/graphics/GM/magick/command.c:17507

20 0x000055c0946cd08a in main (argc=4, argv=0x7fff24a89dd8) at /home/bfriesen/src/graphics/GM/utilities/gm.c:61

This is what valgrind shows. It appears that the pointer passed to free is "wild" data:

==3615== Invalid read of size 8 ==3615== at 0x5C6A363: jas_bma_free (jas_malloc.c:291) ==3615== by 0x5C69E9A: jas_free (jas_malloc.c:145) ==3615== by 0x5C6C28E: jas_stream_destroy (jas_stream.c:607) ==3615== by 0x5C6C2F0: jas_stream_close (jas_stream.c:620) ==3615== by 0x385D66: ReadJP2Image (jp2.c:929) ==3615== by 0x15C86F: ReadImage (constitute.c:1630) ==3615== by 0x124717: ConvertImageCommand (command.c:4416) ==3615== by 0x133919: MagickCommand (command.c:8916) ==3615== by 0x14E45E: GMCommandSingle (command.c:17454) ==3615== by 0x14E589: GMCommand (command.c:17507) ==3615== by 0x118089: main (gm.c:61) ==3615== Address 0x8d303a0 is 32 bytes before a block of size 112 in arena "client" ==3615== ==3615== Invalid free() / delete / delete[] / realloc() ==3615== at 0x4C32D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==3615== by 0x5C6A3FB: jas_bma_free (jas_malloc.c:299) ==3615== by 0x5C69E9A: jas_free (jas_malloc.c:145) ==3615== by 0x5C6C28E: jas_stream_destroy (jas_stream.c:607) ==3615== by 0x5C6C2F0: jas_stream_close (jas_stream.c:620) ==3615== by 0x385D66: ReadJP2Image (jp2.c:929) ==3615== by 0x15C86F: ReadImage (constitute.c:1630) ==3615== by 0x124717: ConvertImageCommand (command.c:4416) ==3615== by 0x133919: MagickCommand (command.c:8916) ==3615== by 0x14E45E: GMCommandSingle (command.c:17454) ==3615== by 0x14E589: GMCommand (command.c:17507) ==3615== by 0x118089: main (gm.c:61) ==3615== Address 0x8d303a0 is 32 bytes before a block of size 112 in arena "client" ==3615== input.jp2 JPC 70x46+0+0 DirectClass 8-bit 0.940u 0m:0.938370s (3.4Ki pixels/s) heap corruption detected

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[bobfriesenhahn]

If I use the new jas_init_custom() interface then the info from valgrind changes (truncated at the point my allocator detects a problem and asserts). I get the impression that JasPer has added a free or changed the order that it does frees when a stream is used. If JasPer was to free the object at jas_streamt "obj" then the errors would look similar to this:

==4171== Invalid read of size 8 ==4171== at 0x19105D: _MagickReallocateResourceLimitedMemory (memory.c:702) ==4171== by 0x1913C9: _MagickFreeResourceLimitedMemory (memory.c:831) ==4171== by 0x5C69E9A: jas_free (jas_malloc.c:145) ==4171== by 0x5C6C28E: jas_stream_destroy (jas_stream.c:607) ==4171== by 0x5C6C2F0: jas_stream_close (jas_stream.c:620) ==4171== by 0x385E3B: ReadJP2Image (jp2.c:929) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== Address 0x8d30700 is 16 bytes after a block of size 8,192 free'd ==4171== at 0x4C32D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==4171== by 0x79183BA: _IO_setb (genops.c:349) ==4171== by 0x791633B: _IO_file_close_it@@GLIBC_2.2.5 (fileops.c:154) ==4171== by 0x7908466: fclose@@GLIBC_2.2.5 (iofclose.c:53) ==4171== by 0x3B220C: CloseBlob (blob.c:1037) ==4171== by 0x3836BB: BlobClose (jp2.c:358) ==4171== by 0x5C6C2E4: jas_stream_close (jas_stream.c:618) ==4171== by 0x385E3B: ReadJP2Image (jp2.c:929) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== Block was alloc'd at ==4171== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==4171== by 0x790826B: _IO_file_doallocate (filedoalloc.c:101) ==4171== by 0x790B58A: setvbuf (iosetvbuf.c:60) ==4171== by 0x3B5840: OpenBlob (blob.c:2881) ==4171== by 0x383AB7: ReadJP2Image (jp2.c:527) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== ==4171== Invalid read of size 8 ==4171== at 0x191060: _MagickReallocateResourceLimitedMemory (memory.c:702) ==4171== by 0x1913C9: _MagickFreeResourceLimitedMemory (memory.c:831) ==4171== by 0x5C69E9A: jas_free (jas_malloc.c:145) ==4171== by 0x5C6C28E: jas_stream_destroy (jas_stream.c:607) ==4171== by 0x5C6C2F0: jas_stream_close (jas_stream.c:620) ==4171== by 0x385E3B: ReadJP2Image (jp2.c:929) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== Address 0x8d30708 is 24 bytes after a block of size 8,192 in arena "client" ==4171== ==4171== Invalid read of size 8 ==4171== at 0x19106C: _MagickReallocateResourceLimitedMemory (memory.c:702) ==4171== by 0x1913C9: _MagickFreeResourceLimitedMemory (memory.c:831) ==4171== by 0x5C69E9A: jas_free (jas_malloc.c:145) ==4171== by 0x5C6C28E: jas_stream_destroy (jas_stream.c:607) ==4171== by 0x5C6C2F0: jas_stream_close (jas_stream.c:620) ==4171== by 0x385E3B: ReadJP2Image (jp2.c:929) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== Address 0x8d30710 is 32 bytes before a block of size 112 in arena "client" ==4171== ==4171== Invalid read of size 8 ==4171== at 0x191070: _MagickReallocateResourceLimitedMemory (memory.c:702) ==4171== by 0x1913C9: _MagickFreeResourceLimitedMemory (memory.c:831) ==4171== by 0x5C69E9A: jas_free (jas_malloc.c:145) ==4171== by 0x5C6C28E: jas_stream_destroy (jas_stream.c:607) ==4171== by 0x5C6C2F0: jas_stream_close (jas_stream.c:620) ==4171== by 0x385E3B: ReadJP2Image (jp2.c:929) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== Address 0x8d30718 is 24 bytes before a block of size 104 alloc'd ==4171== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==4171== by 0x190BB5: MagickMalloc (memory.c:175) ==4171== by 0x3836F8: JP2StreamManager (jp2.c:372) ==4171== by 0x38410C: ReadJP2Image (jp2.c:607) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== ==4171== Invalid read of size 8 ==4171== at 0x19107C: _MagickReallocateResourceLimitedMemory (memory.c:702) ==4171== by 0x1913C9: _MagickFreeResourceLimitedMemory (memory.c:831) ==4171== by 0x5C69E9A: jas_free (jas_malloc.c:145) ==4171== by 0x5C6C28E: jas_stream_destroy (jas_stream.c:607) ==4171== by 0x5C6C2F0: jas_stream_close (jas_stream.c:620) ==4171== by 0x385E3B: ReadJP2Image (jp2.c:929) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== Address 0x8d30720 is 16 bytes before a block of size 104 alloc'd ==4171== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==4171== by 0x190BB5: MagickMalloc (memory.c:175) ==4171== by 0x3836F8: JP2StreamManager (jp2.c:372) ==4171== by 0x38410C: ReadJP2Image (jp2.c:607) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== ==4171== Invalid read of size 8 ==4171== at 0x191080: _MagickReallocateResourceLimitedMemory (memory.c:702) ==4171== by 0x1913C9: _MagickFreeResourceLimitedMemory (memory.c:831) ==4171== by 0x5C69E9A: jas_free (jas_malloc.c:145) ==4171== by 0x5C6C28E: jas_stream_destroy (jas_stream.c:607) ==4171== by 0x5C6C2F0: jas_stream_close (jas_stream.c:620) ==4171== by 0x385E3B: ReadJP2Image (jp2.c:929) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== Address 0x8d30728 is 8 bytes before a block of size 104 alloc'd ==4171== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==4171== by 0x190BB5: MagickMalloc (memory.c:175) ==4171== by 0x3836F8: JP2StreamManager (jp2.c:372) ==4171== by 0x38410C: ReadJP2Image (jp2.c:607) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== gm: /home/bfriesen/src/graphics/GM/magick/memory.c:702: _MagickReallocateResourceLimitedMemory: Assertion `(&memory_resource)->signature == MagickSignature' failed. /home/bfriesen/build/GM-16-static-debug-noopenmp/utilities/gm convert: abort due to signal 6 (SIGABRT) "Abort"...

-- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Bob,

On Sun, 4 Jul 2021, Bob Friesenhahn wrote:

If I use the new jas_init_custom() interface then the info from valgrind changes (truncated at the point my allocator detects a problem and asserts). I get the impression that JasPer has added a free or changed the order that it does frees when a stream is used. If JasPer was to free the object at jas_streamt "obj" then the errors would look similar to this:

==4171== Invalid read of size 8 ==4171== at 0x19105D: _MagickReallocateResourceLimitedMemory (memory.c:702) ==4171== by 0x1913C9: _MagickFreeResourceLimitedMemory (memory.c:831) ==4171== by 0x5C69E9A: jas_free (jas_malloc.c:145) ==4171== by 0x5C6C28E: jas_stream_destroy (jas_stream.c:607) ==4171== by 0x5C6C2F0: jas_stream_close (jas_stream.c:620) ==4171== by 0x385E3B: ReadJP2Image (jp2.c:929) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== Address 0x8d30700 is 16 bytes after a block of size 8,192 free'd ==4171== at 0x4C32D3B: free (in

I am confused by the above traceback. Why is the non-JasPer function ReadJP2Image calling jas_stream_close? I also don't understand why the JasPer decoder functions (presumably for the JP2 decoder) are not appearing in the backtrace. Can you shed any light on this? When is this sequence of function calls happening?

--Michael

[bobfriesenhahn]

On Sun, 4 Jul 2021, Michael Adams wrote:

==4171== Invalid read of size 8 ==4171== at 0x19105D: _MagickReallocateResourceLimitedMemory (memory.c:702) ==4171== by 0x1913C9: _MagickFreeResourceLimitedMemory (memory.c:831) ==4171== by 0x5C69E9A: jas_free (jas_malloc.c:145) ==4171== by 0x5C6C28E: jas_stream_destroy (jas_stream.c:607) ==4171== by 0x5C6C2F0: jas_stream_close (jas_stream.c:620) ==4171== by 0x385E3B: ReadJP2Image (jp2.c:929) ==4171== by 0x15C86F: ReadImage (constitute.c:1630) ==4171== by 0x124717: ConvertImageCommand (command.c:4416) ==4171== by 0x133919: MagickCommand (command.c:8916) ==4171== by 0x14E45E: GMCommandSingle (command.c:17454) ==4171== by 0x14E589: GMCommand (command.c:17507) ==4171== by 0x118089: main (gm.c:61) ==4171== Address 0x8d30700 is 16 bytes after a block of size 8,192 free'd ==4171== at 0x4C32D3B: free (in

I am confused by the above traceback. Why is the non-JasPer function ReadJP2Image calling jas_stream_close? I also don't understand why the JasPer decoder functions (presumably for the JP2 decoder) are not appearing in the backtrace. Can you shed any light on this? When is this sequence of function calls happening?

Since the dawn of time (since 2001) ImageMagick and then GraphicsMagick (since 2002) has performed this sequence at the end of decoding, and when returning from reported errors as well:

jas_matrix_destroy(pixels); (void) jas_stream_close(jp2_stream); jas_image_destroy(jp2_image);

So, that is why it is happening.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Hi Bob,

On Sun, 4 Jul 2021, Bob Friesenhahn wrote:

Since the dawn of time (since 2001) ImageMagick and then GraphicsMagick (since 2002) has performed this sequence at the end of decoding, and when returning from reported errors as well:

jas_matrix_destroy(pixels); (void) jas_stream_close(jp2_stream); jas_image_destroy(jp2_image);

So, that is why it is happening.

Thank you for the clarification. Is this happening near program termination? If you are having problems at/near program termination maybe the atexit problem that I mention in part of my response below is the cause.

In an effort to help pinpoint the cause of the problems that you are having, I have made some additional changes to the mdadams-callbacks branch. All of my comments below relate to this branch (not master).

The memory allocator used by default in JasPer (e.g., when jas_init is used instead of jas_init_custom) is now controlled by the JAS_USE_STD_ALLOCATOR_BY_DEFAULT option in the CMakeLists.txt file. The default settings in the main CMakeLists.txt file enable JasPer's basic memory allocator (BMA) allocator, whose associated functions have "bma" in their names. The BMA allocator tracks (and optionally limits) memory usage. I have added a signature (i.e., magic value) to the memory blocks used by the allocator to give an extra level of checking for bad pointers. I might have found a bug in this memory allocator. I am not sure if the code was wrong, but the suspicious lines of code were replaced in the process of making my other changes. So, I might have fixed a bug. This allocator passes all of the test cases in the JasPer test suite on my test machines.

Will your custom memory allocator be toasted if it is invoked after main returns? The issue here is that jas_init_custom was calling atexit(jas_cleanup), which would be problematic for memory allocators that lose their ability to operate correctly by the time atexit invokes callbacks upon program termination. So, I have changed the behavior of jas_init_custom so that it does not call atexit and it is now the library user's responsibility to call jas_cleanup at an appropriate time. I think that this change will be necessary in order to accommodate some custom allocators. You might want to try your code again with my changes on the off chance that this was the problem. (I'm not feeling lucky, but it is worth a try, I guess.)

If my code changes did not fix things for you, please proceed continue reading. In order to help me to more precisely pinpoint the source of the problems that you are having, could you please try the following:

1) Try building JasPer exactly as is on the mdadams-callbacks branch with the default build options and check if it passes all of it test cases. For example: cmake -H. -Btmp_cmake cmake --build tmp_cmake cmake --build tmp_cmake --target test

2) If step 1 fails, try rebuilding JasPer with the standard library allocator malloc by setting JAS_USE_STD_ALLOCATOR_BY_DEFAULT and JAS_USE_JAS_INIT to true. For example: cmake -H. -Btmp_cmake -DJAS_USE_STD_ALLOCATOR_BY_DEFAULT=1 -DJAS_USE_JAS_INIT=1 cmake --build tmp_cmake cmake --build tmp_cmake --target test

The above will help me to determine whether the problems that you are having are somehow due to the BMA allocator. I cannot see anything wrong with BMA allocator code, and it used to be enabled by default in earlier versions of JasPer (and worked reliably then) but it got disabled at some point and maybe got broken in some strange way by other code changes. The code does, however, work for me. I am hoping that it will work for you, in which case it seems likely that the problem is due to the particular way in which are you using JasPer, which causes a bug in JasPer to be hit that is not normally hit by the application programs that are part of JasPer.

--Michael

[mdadams]

On Sun, 4 Jul 2021, Bob Friesenhahn wrote:

If I use the new jas_init_custom() interface then the info from valgrind changes (truncated at the point my allocator detects a problem and asserts). I get the impression that JasPer has added a free or changed the order that it does frees when a stream is used. If JasPer was to free the object at jas_streamt "obj" then the errors would look similar to this:

I have not changed the stream code in the mdadams-callbacks branch. The only things that I have changed are essentially the memory allocator code and the initialization/cleanup code. So, I don't think that things are going to be freed/allocated in different orders from before.

[bobfriesenhahn]

On Sun, 4 Jul 2021, Michael Adams wrote:

Thank you for the clarification. Is this happening near program termination? If you are having problems at/near program termination maybe the atexit problem that I mention in part of my response below is the cause.

The call stack does not show it is happening at/near program termination. It has not gotten to that point yet.

Will your custom memory allocator be toasted if it is invoked after main returns? The issue here is that jas_init_custom was calling atexit(jas_cleanup), which would be problematic for memory allocators that lose their ability to operate correctly by the time atexit invokes callbacks upon program termination. So, I have changed the behavior of

Yes, it would be toasted (although I could likely avoid the proble). Use of atexit() for cleanup in a library is pox to be avoided.

Some C++ programs may have major issues with atexit().

jas_init_custom so that it does not call atexit and it is now the library user's responsibility to call jas_cleanup at an appropriate time.

Making the user responsible is a good plan. GraphicsMagick already does the right thing here.

If my code changes did not fix things for you, please proceed continue reading. In order to help me to more precisely pinpoint the source of the problems that you are having, could you please try the following:

I will check on this later.

1) Try building JasPer exactly as is on the mdadams-callbacks branch with the default build options and check if it passes all of it test cases. For example: cmake -H. -Btmp_cmake cmake --build tmp_cmake cmake --build tmp_cmake --target test

I do run the test suite before I install the library. I will do what you request later.

The above will help me to determine whether the problems that you are having are somehow due to the BMA allocator. I cannot see anything wrong with BMA allocator code, and it used to be enabled by default in earlier versions of JasPer (and worked reliably then) but it got disabled at some point and maybe got broken in some strange way by other code changes.

I recall that I used this feature before and it failed (crashed). I reported this to you at the time.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Yes, it would be toasted (although I could likely avoid the proble). Use of atexit() for cleanup in a library is pox to be avoided.

I agree with your comment on use of atexit for cleanup, but changing the behavior of jas_init to fix this might break some applications (e.g., due to memory leaks). At least, we can do better with jas_init_custom since it is a new function and, as such, we can define the behavior of this function however we like when it is first officially introduced in the library. I'm glad that you think that having the user call jas_cleanup is a good plan.

The above will help me to determine whether the problems that you are having are somehow due to the BMA allocator. I cannot see anything wrong with BMA allocator code, and it used to be enabled by default in earlier versions of JasPer (and worked reliably then) but it got disabled at some point and maybe got broken in some strange way by other code changes.

I recall that I used this feature before and it failed (crashed). I reported this to you at the time.

If one of the problems that you are experiencing is due to a bug in the BMA allocator, I would like to fix this because I would like to make this allocator the one used by default (i.e., by jas_init) since it tracks and limits memory usage. This is a good safety measure to have for applications that do not use their own custom allocator (i.e., applications that use jas_init). Although it is better that applications use their own allocator (like you do with your application), the unfortunate reality is that many applications do not, and I want to prevent huge allocations from happening when decoding malicious coded images in applications that use the library's default allocator. I hope that makes sense.

[bobfriesenhahn]

On Sun, 4 Jul 2021, Michael Adams wrote:

The above will help me to determine whether the problems that you are having are somehow due to the BMA allocator. I cannot see anything wrong with BMA allocator code, and it used to be enabled by default in earlier versions of JasPer (and worked reliably then) but it got disabled at some point and maybe got broken in some strange way by other code changes.

I recall that I used this feature before and it failed (crashed). I reported this to you at the time.

If one of the problems that you are experiencing is due to a bug in the BMA allocator, I would like to fix this because I would like to make this allocator the one used by default (i.e., by jas_init) since

Using a debugger (starting with jas_stream_close), I see that the stream passed into the close callback is totally corrupted. The _obj definition is based on

typedef struct _StreamManager { jas_stream_t *stream;

Image *image; } StreamManager;

From debugging, I see that the 'image' data is still ok but 'stream' is now garbage. To me this suggest that StreamManager itself is not corrupted, but something has corrupted the jas_stream_t stream.

Breakpoint 1, ReadJP2Image (image_info=0x555555b16cd0, exception=0x7fffffffddf0) at /home/bfriesen/src/graphics/GM/coders/jp2.c:929 929 (void) jas_stream_close(jp2_stream); / crashes in this function / (gdb) s jas_stream_close (stream=0x555555b1b150) at /home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/base/jas_stream.c:612 612 JAS_DBGLOG(100, ("jas_stream_close(%p)\n", stream)); (gdb) n 615 jas_streamflush(stream); (gdb) n 618 (*stream->ops->close)(stream->obj); (gdb) p stream $1 = (jas_streamt *) 0x555555b1b150 (gdb) p stream->ops $2 = (const jas_stream_opst *) 0x7fffffffb240 (gdb) p stream->obj $3 = (jas_stream_objt ) 0x555555b1c020 (gdb) s BlobClose (object=0x555555b1c020) at /home/bfriesen/src/graphics/GM/coders/jp2.c:356 356 source = (StreamManager ) object; (gdb) p source $4 = (StreamManager ) 0x0 (gdb) n 358 CloseBlob(source->image); (gdb) p source $5 = (StreamManager ) 0x555555b1c020 (gdb) p source $6 = {stream = 0x555555b11e20, image = 0x555555b19070} (gdb) p *source->stream $7 = {openmode = 1437668240, bufmode = 21845, flags = 1437663248, bufbase = 0x20626f6c62202020 <error: Cannot access memory at address 0x20626f6c62202020>, bufstart = 0x51 <error: Cannot access memory at address 0x51>, bufsize = -1414746709, ptr = 0x632f676e696e6570 <error: Cannot access memory at address 0x632f676e696e6570>, cnt = 1769172844, tinybuf = "ng/loading events", ops = 0x6361632020202020, obj = 0x2020202020206568, rwcnt = 7311709787893276704, rwlimit = 2334386757906407532} (gdb) p source->image $8 = {storage_class = DirectClass, colorspace = RGBColorspace, compression = UndefinedCompression, dither = 1, matte = 0, columns = 70, rows = 46, colors = 0, depth = 8, colormap = 0x0, background_color = {blue = 65535, green = 65535, red = 65535, opacity = 0}, border_color = {blue = 57311, green = 57311, red = 57311, opacity = 0}, matte_color = {blue = 48573, green = 48573, red = 48573, opacity = 0}, gamma_magick = 0, chromaticity = {red_primary = {x = 0, y = 0, z = 0}, green_primary = {x = 0, y = 0, z = 0}, blue_primary = {x = 0, y = 0, z = 0}, white_point = {x = 0, y = 0, z = 0}}, orientation = UndefinedOrientation, rendering_intent = UndefinedIntent, units = UndefinedResolution, montage = 0x0, directory = 0x0, geometry = 0x0, offset = 0, x_resolution = 0, y_resolution = 0, page = {width = 0, height = 0, x = 0, y = 0}, tile_info = {width = 0, height = 0, x = 0, y = 0}, blur = 1, fuzz = 0, filter = UndefinedFilter, interlace = UndefinedInterlace, endian = UndefinedEndian, gravity = ForgetGravity, compose = OverCompositeOp, dispose = UndefinedDispose, scene = 0, delay = 0, iterations = 0, total_colors = 0, start_loop = 0, error = {mean_error_per_pixel = 0, normalized_mean_error = 0, normalized_maximum_error = 0}, timer = {user = { start = 0.01, stop = 0, total = 0}, elapsed = {start = 1548117.410848994, stop = 0, total = 0}, state = RunningTimerState, signature = 2880220587}, client_data = 0x0, filename = "input.jp2", '\000' <repeats 2043 times>, magick_filename = "input.jp2", '\000' <repeats 2043 times>, magick = "JPC", '\000' <repeats 2049 times>, magick_columns = 0, magick_rows = 0, exception = {severity = UndefinedException, reason = 0x0, description = 0x0, error_number = 0, module = 0x0, function = 0x0, line = 0, signature = 2880220587}, previous = 0x0, next = 0x0, profiles = 0x0, is_monochrome = 0, is_grayscale = 0, taint = 1, extra = 0x555555b11fa0, ping = 0, cache = 0x555555b1c100, default_views = 0x555555b1bf60, attributes = 0x0, ascii85 = 0x0, blob = 0x555555b1bed0, reference_count = 1, semaphore = 0x555555b1b040, logging = 0, list = 0x0, signature = 2880220587} (gdb) p source->stream->ops_ Cannot access memory at address 0x6361632020202020

-- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Bob,

On Mon, 5 Jul 2021, Bob Friesenhahn wrote:

Using a debugger (starting with jas_stream_close), I see that the stream passed into the close callback is totally corrupted. The _obj definition is based on

typedef struct _StreamManager { jas_stream_t *stream;

Image *image; } StreamManager; ...

While I appreciate this information, the most important thing that I need to know at this point is what I indicated in my post yesterday. That is, I need to know whether the version of JasPer on the mdadams-callbacks branch without any modification works or not on your machine. That is, does it work or not in each of the following cases:

(a) the BMA allocator is used;

(b) the standard library allocator is used.

In each of these cases, I need for you to run the JasPer test suite to check whether all of the test cases pass, and also try decoding, with the (unmodified mdadams-callbacks version of the) imginfo application, the particular image that you are decoding when you are experiencing crashes (but still not using any of your code). This would help to provide an indication of whether the bug is in the BMA allocator, in other parts of the base JasPer code, or somewhere else.

Thanks, Michael

[bobfriesenhahn]

On Mon, 5 Jul 2021, Michael Adams wrote:

While I appreciate this information, the most important thing that I need to know at this point is what I indicated in my post yesterday. That is, I need to know whether the version of JasPer on the mdadams-callbacks branch without any modification works or not on your machine. That is, does it work or not in each of the following cases:

(a) the BMA allocator is used;

(b) the standard library allocator is used.

I am not sure how the above is accomplished without any modification.

In each of these cases, I need for you to run the JasPer test suite to check whether all of the test cases pass, and also try decoding, with the (unmodified mdadams-callbacks version of the) imginfo application, the particular image that you are decoding when you are experiencing crashes (but still not using any of your code). This would help to provide an indication of whether the bug is in the BMA allocator, in other parts of the base JasPer code, or somewhere else.

The cause of the problem has been discovered. It is related to use of an alternate memory allocator (BMA or user provided via the new API).

The problem is that the jas_stream_t stream needs to be allocated via jas_malloc() rather than malloc(). The reason for this is that jas_stream_destroy() uses jas_free() and jas_free() is based on JasPer's internal memory allocator.

If malloc()/free() are used to allocate the elements needed for a stream, then there will be problems when something other than the standard library is used.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Bob,

On Mon, 5 Jul 2021, Bob Friesenhahn wrote:

The cause of the problem has been discovered. It is related to use of an alternate memory allocator (BMA or user provided via the new API).

The problem is that the jas_stream_t stream needs to be allocated via jas_malloc() rather than malloc(). The reason for this is that jas_stream_destroy() uses jas_free() and jas_free() is based on JasPer's internal memory allocator.

If malloc()/free() are used to allocate the elements needed for a stream, then there will be problems when something other than the standard library is used.

Can you please clarify where the mismatch between the allocation and deallocation functions is happening? I looked at jas_stream.c and I cannot find any places where malloc or free are being called directly. Everything seems to be using jas_malloc and jas_free. For example, jas_stream_create uses jas_malloc and jas_stream_destroy uses jas_free.

Obviously, there is no way to stop malloc/free from being used by the C standard library, but this shouldn't affect JasPer. As long as allocation that is done directly by JasPer uses jas_malloc and jas_free consistently all should be well.

BTW, in an earlier commit, I did find a memory allocator mismatch in the TSFB code for the JP2/JPC codec, but this was fixed and therefore should not be the problem.

--Michael

[bobfriesenhahn]

On Mon, 5 Jul 2021, Michael Adams wrote:

If malloc()/free() are used to allocate the elements needed for a stream, then there will be problems when something other than the standard library is used.

Can you please clarify where the mismatch between the allocation and deallocation functions is happening? I looked at jas_stream.c and I cannot find any places where malloc or free are being called directly. Everything seems to be using jas_malloc and jas_free. For example, jas_stream_create uses jas_malloc and jas_stream_destroy uses jas_free.

To be clear, the user here which is creating the stream manager is GraphicsMagick code. The problem is not in JasPer itself.

If GraphicsMagick was using malloc/free then it is possible that other JasPer users would also be impacted by this issue.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Hi Bob,

On Mon, 5 Jul 2021, Bob Friesenhahn wrote:

Can you please clarify where the mismatch between the allocation and deallocation functions is happening? I looked at jas_stream.c and I cannot find any places where malloc or free are being called directly. Everything seems to be using jas_malloc and jas_free. For example, jas_stream_create uses jas_malloc and jas_stream_destroy uses jas_free.

To be clear, the user here which is creating the stream manager is GraphicsMagick code. The problem is not in JasPer itself.

I am still having difficulty understanding the particular usage of JasPer that is leading to problems. As long as you use the JasPer library API to create and destroy stream objects, I think that everything should be okay (i.e., mismatched allocation/deallocation functions should not arise). The only usage that I can think of that would be problematic would be if a user bypasses the JasPer API and starts creating/destroying JasPer stream objects with their own code. Then, the user would have to be very careful to use jas_malloc and jas_free and not other allocation/deallocation functions. Are you constructing your own stream objects in your code? Is this what was causing your problems? (If this is what you are doing, I would recommend against this, if it can be reasonably avoided.) I would like to be sure that I understand the problem fully in order to better determine the impact of this issue on other library users.

--Michael

[bobfriesenhahn]

On Mon, 5 Jul 2021, Michael Adams wrote:

I am still having difficulty understanding the particular usage of JasPer that is leading to problems. As long as you use the JasPer library API to create and destroy stream objects, I think that everything should be okay (i.e., mismatched allocation/deallocation functions should not arise). The only usage that I can think of that would be problematic would be if a user bypasses the JasPer API and starts creating/destroying JasPer stream objects with their own code. Then, the user would have to be very careful to use jas_malloc and jas_free and not other allocation/deallocation functions. Are you constructing your own stream objects in your code? Is this what was causing your problems? (If this is what you are doing, I would recommend against this, if it can be reasonably avoided.) I would like to be sure that I understand the problem fully in order to better determine the impact of this issue on other library users.

Yes, of course GraphicsMagick is constructing its own stream objects using the JasPer API. GraphicsMagick has its own I/O layer.

I did not create the original GraphicsMagick interface to JasPer, although I have improved it quite a lot. The stream interfacing was originally done by someone else (probably John Cristy and Bill Radcliffe) in the 2001/2002 time-frame.

GraphicsMagick is free and open software. You can view the current JPEG 2000 support at:

https://sourceforge.net/p/graphicsmagick/code/ci/default/tree/coders/jp2.c

You might even try building it yourself as a sample program which uses JasPer.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Hi Bob,

On Mon, 5 Jul 2021, Bob Friesenhahn wrote:

Yes, of course GraphicsMagick is constructing its own stream objects using the JasPer API. GraphicsMagick has its own I/O layer.

I guess that the problem is more-or-less caused by the function:

 static jas_stream_t *JP2StreamManager(jas_stream_ops_t *stream_ops,
 Image *image);

This function is using JasPer in a way that is not really supported, as it relies on JasPer internals. In any case, based on what you said, at least your code does work as long as you are careful to avoid mismatches in allocation/deallocation functions.

So, just to be sure... It is now my understanding that all of the problems with the mdadams-callbacks branch have been resolved. Is this correct? I'd like to be sure that things work for you before moving forward.

The only other thing that I am currently considering before merging is effectively making the code invoked by jas_eprintf configurable by the application at run time (in a similar way as the memory allocator). The jas_eprintf function basically performs formatted information/error logging for the JasPer library. I think that it would be helpful to allow library users to provide their own version of this function. For some applications, sending log messages to standard error (which is the current behavior) is likely not a good choice. Just out of curiosity, is control over jas_eprintf something that might be helpful to you?

--Michael

[bobfriesenhahn]

On Mon, 5 Jul 2021, Michael Adams wrote:

So, just to be sure... It is now my understanding that all of the problems with the mdadams-callbacks branch have been resolved. Is this correct? I'd like to be sure that things work for you before moving forward.

It is working in that the GraphicsMagick test suite is working with it. I have not done any re-testing with oss-fuzz test cases with this branch to see how effective it is at preventing excess use of memory.

Until yesterday's commit, oss-fuzz was free to declare the input format of its test case, but then the GM decoder would do its own format detection based on the header and dispatch to the "appropriate" decoder (e.g. JP2 --> PGX). Now the GM decoder enforces that the declared format matches the header. So once oss-fuzz runs its tests again, I am expecting a significant impact on the many open cases (most of the open cases are due to JasPer) since many of the existing cases would be outright rejected. The actual underyling issues still remain and eventually oss-fuzz will find them again by supplying a correct header.

The only other thing that I am currently considering before merging is effectively making the code invoked by jas_eprintf configurable by the application at run time (in a similar way as the memory allocator). The jas_eprintf function basically performs formatted information/error logging for the JasPer library. I think that it would be helpful to allow library users to provide their own version of this function. For some applications, sending log messages to standard error (which is the current behavior) is likely not a good choice. Just out of curiosity, is control over jas_eprintf something that might be helpful to you?

This would be very helpful. We have some similar support from other libraries such as the JPEG and TIFF libraries.

As it exists, this appears to be just tracing for the user, without any declaration of impact. So GraphicsMagick could only use it for tracing for the user (which is still very useful).

It is useful to have a distinction between terminal "error" reports and informational traces since "error" reports can be passed on as the final error reason to the user. If there were "error" reports, that would likely require a different/new call-back since existing uses are informational.

JasPer has no notion of calling thread context but its APIs use an image handle. When this image handle is allocated, an error context could be registered/allocated with it in order to support delivering an error report payload. Otherwise methods such as thread-specific data would need to be used.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[bobfriesenhahn]

In order for the new JasPer memory allocation callbacks to be effective (and in general!), JasPer needs to properly deal with every case where an allocation request may return NULL. This is clearly not currently the case as shown by this stack backtrace (notice jas_matrix_destroy (matrix=0x0)):

Core was generated by `/home/bfriesen/gm16 convert -limit memory 1GB clusterfuzz-testcase-minimized-co'. Program terminated with signal SIGABRT, Aborted.

0 __GI_raise @.***=6) at ../sysdeps/unix/sysv/linux/raise.c:51

51 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. (gdb) bt

0 __GI_raise @.***=6) at ../sysdeps/unix/sysv/linux/raise.c:51

1 0x00007f05308fc9fb in __GI_abort () at abort.c:100

2 0x00005590bad4f193 in MagickPanicSignalHandler (signo=) at /home/bfriesen/src/graphics/GM/magick/magick.c:873

3 MagickPanicSignalHandler (signo=) at /home/bfriesen/src/graphics/GM/magick/magick.c:849

4

5 0x00007f0532af4b1e in jas_matrix_destroy (matrix=0x0) at /home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/base/jas_seq.c:163

6 0x00007f0532b05940 in jpc_dec_tilefini (dec=0x5590bccdd6f0, tile=0x5590bccde410)

 at /home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/jpc/jpc_dec.c:1073

7 0x00007f0532b086ee in jpc_dec_destroy (dec=0x5590bccdd6f0) at /home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/jpc/jpc_dec.c:2171

8 0x00007f0532b034f3 in jpc_decode (in=0x5590bccdd660, optstr=0x7ffeb4a23e70 "max_samples=1073741824")

 at /home/bfriesen/src/graphics/jasper-callbacks.git/src/libjasper/jpc/jpc_dec.c:297

9 0x00005590baea0be6 in ReadJP2Image (image_info=0x5590bccd3e50, exception=0x7ffeb4a26940) at /home/bfriesen/src/graphics/GM/coders/jp2.c:639

10 0x00005590bad2a288 in ReadImage @.=0x5590bcccfcb0, @.=0x7ffeb4a26940)

 at /home/bfriesen/src/graphics/GM/magick/constitute.c:1630

11 0x00005590bad0d5f3 in ConvertImageCommand (image_info=0x5590bcccfcb0, argc=, argv=, metadata=0x0,

 exception=0x7ffeb4a26940) at /home/bfriesen/src/graphics/GM/magick/command.c:4416

12 0x00005590bacff545 in MagickCommand @.=0x5590bcccfcb0, @.=6, @.***=0x7ffeb4a272d0,

 ***@***.***=0x7ffeb4a26938, ***@***.***=0x7ffeb4a26940) at /home/bfriesen/src/graphics/GM/magick/command.c:8916

13 0x00005590bad005fa in GMCommandSingle (argc=6, @.=7, argv=0x7ffeb4a272d0, @.=0x7ffeb4a272c8)

 at /home/bfriesen/src/graphics/GM/magick/command.c:17454

14 0x00005590bad1edcf in GMCommand (argc=7, argv=0x7ffeb4a272c8) at /home/bfriesen/src/graphics/GM/magick/command.c:17507

15 0x00007f05308ddbf7 in __libc_start_main (main=0x5590bacf7ba0

, argc=7, argv=0x7ffeb4a272c8, init=, fini=,

 rtld_fini=<optimized out>, stack_end=0x7ffeb4a272b8) at ../csu/libc-start.c:310

16 0x00005590bacf7e6a in _start ()

-- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Hi Bob,

On Sat, 10 Jul 2021, Bob Friesenhahn wrote:

In order for the new JasPer memory allocation callbacks to be effective (and in general!), JasPer needs to properly deal with every case where an allocation request may return NULL. This is clearly not currently the case as shown by this stack backtrace (notice jas_matrix_destroy (matrix=0x0)):

Given that the last bug report that I dealt with from you (which I spent a substantial amount of time on), was not due to a bug in JasPer but rather due to allocator mismatch in your application code, I hope that you can appreciate why I might be a little skeptical about this new bug report. For example, have you actually found any place in JasPer where the return value of jas_malloc (or a similar function) is ignored? I am not saying that that this cannot be the case, but I am saying that it is not a foregone conclusion (and it does seem likely that if such a bug did exist in JasPer it would have been hit and reported a long time ago). Please understand that the particular failure that you are seeing could conceivably be caused by bugs in the application using JasPer, especially if the application is using a custom allocator (which is the case here, as I understand it). In any case, even if the bug is in JasPer, it is impossible for me to fix such a problem unless I can reproduce it. If this bug is in JasPer (and not the application), it is quite likely that the bug should be reproducible in JasPer without your application code if you use the same coded bitstream that causes the failure in your application. If you can reproduce the problem in this way, I can investigate further.

--Michael

[bobfriesenhahn]

On Sat, 10 Jul 2021, Michael Adams wrote:

(which is the case here, as I understand it). In any case, even if the bug is in JasPer, it is impossible for me to fix such a problem unless I can reproduce it. If this bug is in JasPer (and not the application), it is quite likely that the bug should be reproducible in JasPer without your application code if you use the same coded bitstream that causes the failure in your application. If you can reproduce the problem in this way, I can investigate further.

I fix many problems without reproducing them. You can do so as well!

When I run the program this message is printed by JasPer code:

"maximum memory limit (1073741824) would be exceeded"

This means that the issue was detected by JasPer rather than my own code.

What I see is that the implementation of jpc_dec_destroy() is not fully robust. There is a loop which destroys tiles based on the claimed number of tiles. The jpc_dec_tilefini() function is called. If one looks at jas_matrix_destroy(), it is readily apparent that it is not robust against null pointers since it does not check for them. The null pointer in the provided backtrace is as plain as day.

This is not a case of the original memory allocation failure not being detected. Instead it is a simple case of calling a generalized clean-up function which is not fully prepared to handle null pointers.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[bobfriesenhahn]

I opened a new issue for the jpc_dec_destroy() bug, using only the 'jasper' program.

[mdadams]

Hi Bob,

On Sat, 10 Jul 2021, Bob Friesenhahn wrote:

What I see is that the implementation of jpc_dec_destroy() is not fully robust. There is a loop which destroys tiles based on the claimed number of tiles. The jpc_dec_tilefini() function is called. If one looks at jas_matrix_destroy(), it is readily apparent that it is not robust against null pointers since it does not check for them. The null pointer in the provided backtrace is as plain as day.

This is not a case of the original memory allocation failure not being detected. Instead it is a simple case of calling a generalized clean-up function which is not fully prepared to handle null pointers.

The problem is not in jpc_dec_destroy. The problem (or at least, a problem) is in the decoder initialization. That is, the decoder does not always initialize all of its state properly. This only leads to bad behavior in certain particular cases. The BMA allocator (or custom allocators) increase the probability of hitting this bad behavior, however. I guess that this is why no one encountered this problem before, but you did. Lucky you, eh?

--Michael

[mdadams]

Bob,

On Tue, 6 Jul 2021, Bob Friesenhahn wrote:

Until yesterday's commit, oss-fuzz was free to declare the input format of its test case, but then the GM decoder would do its own format detection based on the header and dispatch to the "appropriate" decoder (e.g. JP2 --> PGX). Now the GM decoder enforces that the declared format matches the header. So once oss-fuzz runs its tests again, I am expecting a significant impact on the many open cases (most of the open cases are due to JasPer) since many of the existing cases would be outright rejected. The actual underyling issues still remain and eventually oss-fuzz will find them again by supplying a correct header.

The JPC decoder initialization bug #293 will cause MANY problems. So, I would imagine at least some of your fuzz testing errors will go away with this bug fix.

JasPer has no notion of calling thread context but its APIs use an image handle. When this image handle is allocated, an error context could be registered/allocated with it in order to support delivering an error report payload. Otherwise methods such as thread-specific data would need to be used.

It is not feasible to use the approach that you have indicated (unless I have misunderstood it) because this would require passing additional information as an explicit parameter to almost every function in JasPer (since almost all nontrivial functions use the logging/error capability). I might be able to do something to distinguish errors from informational messages, but I'll have to think more about this.

--Michael

[bobfriesenhahn]

On Sat, 10 Jul 2021, Michael Adams wrote:

The problem is not in jpc_dec_destroy. The problem (or at least, a problem) is in the decoder initialization. That is, the decoder does not always initialize all of its state properly. This only leads to bad behavior in certain particular cases. The BMA allocator (or custom allocators) increase the probability of hitting this bad behavior, however. I guess that this is why no one encountered this problem before, but you did. Lucky you, eh?

After pulling latest changes, the situation is improved with the test-case (no crash), but I see that valgrind is reporting memory leaks. I don't know if these claimed leaks are unexpected since I have never run valgrind on the jasper utility before:

maximum memory limit (1073741824) would be exceeded error: cannot load image data ==8527== 64 bytes in 1 blocks are possibly lost in loss record 1 of 7 ==8527== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8527== by 0x4E5609E: jas_bma_alloc (jas_malloc.c:244) ==8527== by 0x4E55D83: jas_malloc (jas_malloc.c:132) ==8527== by 0x4E57DB0: jas_stream_fopen (jas_stream.c:341) ==8527== by 0x109D61: main (jasper.c:214) ==8527== ==8527== 64 bytes in 1 blocks are possibly lost in loss record 2 of 7 ==8527== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8527== by 0x4E5609E: jas_bma_alloc (jas_malloc.c:244) ==8527== by 0x4E55D83: jas_malloc (jas_malloc.c:132) ==8527== by 0x4E57DB0: jas_stream_fopen (jas_stream.c:341) ==8527== by 0x109E1D: main (jasper.c:230) ==8527== ==8527== 136 bytes in 1 blocks are possibly lost in loss record 3 of 7 ==8527== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8527== by 0x4E5609E: jas_bma_alloc (jas_malloc.c:244) ==8527== by 0x4E55D83: jas_malloc (jas_malloc.c:132) ==8527== by 0x4E57965: jas_stream_create (jas_stream.c:165) ==8527== by 0x4E57CED: jas_stream_fopen (jas_stream.c:310) ==8527== by 0x109D61: main (jasper.c:214) ==8527== ==8527== 136 bytes in 1 blocks are possibly lost in loss record 4 of 7 ==8527== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8527== by 0x4E5609E: jas_bma_alloc (jas_malloc.c:244) ==8527== by 0x4E55D83: jas_malloc (jas_malloc.c:132) ==8527== by 0x4E57965: jas_stream_create (jas_stream.c:165) ==8527== by 0x4E57CED: jas_stream_fopen (jas_stream.c:310) ==8527== by 0x109E1D: main (jasper.c:230) ==8527== ==8527== 8,240 bytes in 1 blocks are possibly lost in loss record 5 of 7 ==8527== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8527== by 0x4E5609E: jas_bma_alloc (jas_malloc.c:244) ==8527== by 0x4E55D83: jas_malloc (jas_malloc.c:132) ==8527== by 0x4E58EEA: jas_stream_initbuf (jas_stream.c:965) ==8527== by 0x4E57E70: jas_stream_fopen (jas_stream.c:363) ==8527== by 0x109D61: main (jasper.c:214) ==8527== ==8527== 8,240 bytes in 1 blocks are possibly lost in loss record 6 of 7 ==8527== at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8527== by 0x4E5609E: jas_bma_alloc (jas_malloc.c:244) ==8527== by 0x4E55D83: jas_malloc (jas_malloc.c:132) ==8527== by 0x4E58EEA: jas_stream_initbuf (jas_stream.c:965) ==8527== by 0x4E57E70: jas_stream_fopen (jas_stream.c:363) ==8527== by 0x109E1D: main (jasper.c:230) ==8527==

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Hi Bob,

On Sun, 11 Jul 2021, Bob Friesenhahn wrote:

After pulling latest changes, the situation is improved with the test-case (no crash), but I see that valgrind is reporting memory leaks.

It's good to hear that the more serious problem (with incorrect initialization) seems to be fixed.

As for the memory leak problem, this is not really a bug per se, as far as I can tell. Your debug information seems to be showing the case of leaked memory for JasPer I/O streams when the application program terminates (with an error) without first closing all open streams. The issue is that when the application programs that are part of JasPer terminate with an error, they do not close their open (JasPer) file streams which have some memory allocated against them. If the code was written in C++, the solution could be handled relatively easily by using RAII and destructors. In C, however, it is not always practical to do all cleanup on every error path in an application. Since the application is terminating, the memory will be released anyways, so not freeing the memory is not a big deal (in relative terms) aside from valgrind/LSAN warnings. The only practical way to avoid the memory leak would be to close all JasPer I/O streams via atexit. Doing cleanup that frees memory in an atexit handler, however, would interact poorly with (i.e., probably break) some/many custom allocators. Note that it is not really the library that is at fault here. It is the fault of the JasPer application (e.g., jasper, imginfo, etc.) for not closing all open JasPer streams before exiting.

--Michael

[mdadams]

@bobfriesenhahn I have just pushed a new commit to the mdadams-callbacks branch. See commit b48c3660037069b21af275931a43082fea40c7ad. This commit makes some significant changes to the interfaces that relate to initialization. I'm sorry for making some breaking changes on you, but this sort of thing is inevitable from time to time, since the code on this branch is experimental. (Despite the code being experimental, it is still very helpful to have you try it and see how well it meets your needs so that I have a better idea whether this work is worth merging back into master.) I have added a file called notes.txt in the top-level directory of the source tree to highlight how initialization works (as of this new commit). The main change is to make more configuration parameters controllable via jas_init_custom and the merge the two parameters for jas_init_custom into one. The code appears to be working okay, based on my testing. It passes all of the test cases in the test suite as well. You can now control which codecs built into the library are enabled via jas_init_custom.

Changing the topic slightly: Currently, the JasPer library is initialized as a single library instance for all threads in a process. How important do you think it is to allow each thread to have their own separate independent instance of JasPer? Or do you even think that this is a good idea at all? I'm not sure if it is possible to add this functionality without breaking backward compatibility (and it would probably require some use of thread local storage), but if it is possible how useful would it be, or would it even be a good idea at all?

[bobfriesenhahn]

On Sun, 11 Jul 2021, Michael Adams wrote:

significant changes to the interfaces that relate to initialization. I'm sorry for making some breaking changes on you, but this sort of thing is inevitable from time to time, since the code on this branch is experimental. (Despite the code being experimental, it is still very helpful to have you try it and see how well it meets your needs so that I have a better idea whether this work is worth merging back into master.) I have added a file called notes.txt in the top-level

I will take a look.

Changing the topic slightly: Currently, the JasPer library is initialized as a single library instance for all threads in a process. How important do you think it is to allow each thread to have their own separate independent instance of JasPer? Or do you even think that this is a good idea at all? I'm not sure if it is possible to add this functionality without breaking backward compatibility (and it would probably require some use of thread local storage), but if it is possible how useful would it be, or would it even be a good idea at all?

This is something that I wish GraphicsMagick could do. It would be nice to have an initialized context 'handle' and then that handle is passed everywhere. Unfortunately, this can not be done without breaking the existing API since it would be a whole new set of APIs.

If one imagines a multi-threaded server process which accepts requests from multiple clients, then one can imagine that the clients desire different properties (e.g. their own memory limits).

In order to make the ABI the most robust, one would want to allocate an opaque handle, and then use discrete function calls to update settings in that opaque handle. As long as the API user never sees the definition of the opaque handle, you have tremendously more freedom in the future to add features without impact to the ABI since it is just a matter of adding more accessor functions rather than adding structure members.

GraphicsMagick does use TSD for some things. Under Linux and other Unix like systems, there is virtually no execution cost, but it seems that there is some execution cost under Windows. See https://sourceforge.net/p/graphicsmagick/code/ci/default/tree/magick/tsd.c for a TSD wrapper I created which works for single-thread, POSIX threads, Windows threads, and OpenMP.

When one uses TSD one makes an assumption regarding the threading model and how the program uses threads.

Languages like Go use a "tasklet" green-threads type model where the invocation context is not necessarily the same as a POSIX/Windows thread and so the OS thread id could change while the program is running. TSD could break down severely in such cases unless there is a special TSD designed for the threading model.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[bobfriesenhahn]

On Sun, 11 Jul 2021, Michael Adams wrote:

@bobfriesenhahn I have just pushed a new commit to the mdadams-callbacks branch. See commit b48c3660037069b21af275931a43082fea40c7ad. This commit makes some significant changes to the interfaces that relate to initialization. I'm sorry for making some breaking changes on you, but this sort of thing is inevitable from time to time, since the code on this branch is experimental. (Despite the code being experimental, it is still

My working copy of GraphicsMagick is working as well as before with the interface updates.

The new interface design is rigid and fragile due to requiring that the user allocate the structure. The allocation lifetime of the structure is not clear, but it seems that JasPer must copy the configuration values since otherwise there would be a crash when jas_init_custom() has been called but the allocation context for the structure is gone.

It would be better if the library would at least provide functions for allocating and freeing the structure so that the using app does not bake in the structure size. For full defense, there should be accessor functions for updating each property and the structure should be only forward declared so that the using app does not see the structure members.

If the library is constrained to a single thread-unsafe initialization, then it could allocate the structure internally as a static allocation rather than on the heap. The using app would be unaware as to how the structure is allocated.

OpenSSL is an example of a library which has learned the hard way and forced a breaking API change in that its interface structures were changed to only being forward-declared. Libpng did the same.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Hi Bob,

On Mon, 12 Jul 2021, Bob Friesenhahn wrote:

The new interface design is rigid and fragile due to requiring that the user allocate the structure. The allocation lifetime of the structure is not clear, but it seems that JasPer must copy the configuration values since otherwise there would be a crash when jas_init_custom() has been called but the allocation context for the structure is gone.

I think that you have misunderstood the jas_init_custom function. No allocation needs to be performed by the user of the library. The jas_conf_t* parameter to jas_init_custom is used to initialize other internal data structures in the JasPer library (by copying information). The jas_conf_t object only needs to live until jas_init_custom returns.

In order to make this clearer, I have updated the documentation for jas_init_custom to read as follows (in commit f3cd939b58addb9ddc68c480d7250ea460c9c568):

/*! @brief Initialize the JasPer library with custom values for various configuration settings.

@param conf A pointer to the data structure that specifies the configuration settings to be used for the library.

@details This function initializes the JasPer library, setting the values of various configuration parameters for the library to those specified by @c conf. Any information needed by the library that is specified in the structure pointed to by @c conf is copied before the function returns.

@returns If successful, zero is returned; otherwise, a nonzero value is returned. / JAS_DLLEXPORT int jas_init_custom(const jas_conf_t conf);

Does the above clarification address your concerns? As far as I can tell, there is no memory allocation or lifetime issues here at all, since the jas_conf_t data structure does not need to live longer than the point at which the jas_init_custom function returns to the caller.

--Michael

[bobfriesenhahn]

On Mon, 12 Jul 2021, Michael Adams wrote:

Hi Bob,

On Mon, 12 Jul 2021, Bob Friesenhahn wrote:

The new interface design is rigid and fragile due to requiring that the user allocate the structure. The allocation lifetime of the structure is not clear, but it seems that JasPer must copy the configuration values since otherwise there would be a crash when jas_init_custom() has been called but the allocation context for the structure is gone.

I think that you have misunderstood the jas_init_custom function. No allocation needs to be performed by the user of the library. The jas_conf_t* parameter to jas_init_custom is used to initialize other internal data structures in the JasPer library (by copying information). The jas_conf_t object only needs to live until jas_init_custom returns.

The above is what I assumed.

There would typically be an automatic allocation of a structure on the stack.

My concern is that you would not be able to modify the jas_conf_t definition in the future without changing the ABI. If you were to add members to the structure then they would need to be added to the end.

If the structure is enlarged, then the JasPer library has no idea what structure variant is being used. The jas_init_custom function likely expects the structure definition which was current when that version of the library was released. For example, the structure may be old (thereby likely smaller) and then the JasPer library will access outside of the bounds of the structure and cause a crash.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Hi Bob,

On Sun, 11 Jul 2021, Bob Friesenhahn wrote:

Changing the topic slightly: Currently, the JasPer library is initialized as a single library instance for all threads in a process. How important do you think it is to allow each thread to have their own separate independent instance of JasPer? Or do you even think that this is a good idea at all? ...

This is something that I wish GraphicsMagick could do. It would be nice to have an initialized context 'handle' and then that handle is passed everywhere. Unfortunately, this can not be done without breaking the existing API since it would be a whole new set of APIs.

Because most functions in the JasPer library have a dependence (either directly or indirectly) on the configuration parameters specified via the jas_init_custom function, this means that almost all functions in JasPer need access to this state information. As far as I can see, there are really only two options in this regard (unless one relies on functionality not guaranteed by the C standard):

1) pass an extra explicit "context" parameter to every single function
in the JasPer library (including all internal functions); or

2) use thread-local storage (for thread-specific state);
I believe that this is essentially what you mean when you are
referring to thread-specific data (TSD).  Maybe the only distinction
is that thread-local storage is specified in the C standard, whereas
TSD could be referring to a more general mechanism for thread-specific
data than thread-local storage (as specified in C11).

Of the above options, option 1 is just not practical as it would lead to very gross and convoluted code. Option 2 is practical and is required to be supported by the C language as of the C11 standard. So, option 2 may be doable, but would need further investigation. The first question (with option 2) is how many users of JasPer are still using pre-C11 compilers that lack support for thread-local storage. The second question is whether multiple contexts can be added in a way that does not break compatibility with older versions of the library.

The need for a user to see a context handle may be avoidable by using thread-local storage. In this case, the language implementation effectively takes care of the context handle implicitly. As you noted, there could be performance issues as well, with the addition of support for multiple contexts. There are definitely many considerations, which is why I have not resolved this issue to date. This said, however, with the changes that I am making with the addition of jas_init_custom, it makes sense to re-consider the multithreading multiple-context issue now, since it would probably make sense to change both things at once.

--Michael

[mdadams]

Hi Bob,

My concern is that you would not be able to modify the jas_conf_t definition in the future without changing the ABI. If you were to add members to the structure then they would need to be added to the end.

This is one reason why I added the "reserved" member in the jas_conf_t type (which is for future expansion to add new members later). This could be adjusted to ensure the size of the structure does not change. A little more elegance is required, perhaps using a union and alignof, but I think that such an approach is technically feasible. I think that as long as the size of jas_conf_t is fixed and there is a function like jas_get_default_conf which is required to be used to initialize the jas_conf_t object, this should help to reduce the need to break ABI every time a new member is added. In what I wrote above, I am assuming that new members are added at the end of the structure (in order to maintain offsets of previously-existing members). Does that make sense? Incidentally, I don't plan to add many new configuration parameters in the future. I only plan to address the list of things that have been identified as problematic/suboptimal over the years since JasPer was first written.

--Michael

[bobfriesenhahn]

On Mon, 12 Jul 2021, Michael Adams wrote:

2) use thread-local storage (for thread-specific state); I believe that this is essentially what you mean when you are referring to thread-specific data (TSD). Maybe the only distinction is that thread-local storage is specified in the C standard, whereas TSD could be referring to a more general mechanism for thread-specific data than thread-local storage (as specified in C11).

My use of "TSD" is from the pthreads APIs, which call it thread-specific data:

SYNOPSIS

include

    int pthread_key_create(pthread_key_t *key, void (*destr_function) (void *));

    int pthread_key_delete(pthread_key_t key);

    int pthread_setspecific(pthread_key_t key, const void *pointer);

    void * pthread_getspecific(pthread_key_t key);

I have not looked at C11 thread-local storage, which might be something different. This might only be useful when C11 threads are used, and not when some other form of threads are used. Under Linux, OpenMP threads happen to be discrete POSIX threads (at least a discrete POSIX thread id is consistently returned) and perhaps C11 threads are also POSIX threads with similar properties.

In Linux (and likely other modern Unix systems), each process has a special reserved area (similar to the process stack) where an array of TSD pointers may be stored. Calling pthread_key_create() returns a unique key, which is just an index into the TSD array. The pthread_setspecific()/pthread_getspecific() functions use this index value so it is very fast. A valuable feature of this facility is that pthread_key_create() accepts a destructor function for automatic clean-up on delete.

Take care about relying on exotic C11 features since not all evironments have been speedy with supporting new things.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt

[mdadams]

Hi Bob,

On Mon, 12 Jul 2021, Bob Friesenhahn wrote:

My use of "TSD" is from the pthreads APIs, which call it thread-specific data:

Thank you for the clarification as to what you meant by TSD. Currently, the JasPer library only requires a system that is compliant with the C standard, not POSIX. Portability has always been an important consideration in the development of JasPer. I know that some people/projects/organizations use the JasPer library on systems that are not POSIX compliant and have no POSIX Threads library. This precludes the use of this library in the case of these users. This is why I feel quite strongly that it would be better to rely on the C standard for threading support instead of relying on another library that is known not to be supported on some platforms of interest with C compilers. The threading support in C is implemented using whatever operating system primitives are provided natively. The main benefit of using the C language support for threading is that it is completely platform independent. Any code that requires the POSIX Thread library is, by definition, not completely platform independent, as such code requires a system with the POSIX Threads library. On POSIX systems, I strongly suspect that the C language threading support uses POSIX Threads underneath. So, I don't think that POSIX Threads are likely to be any more efficient than C threads.

Take care about relying on exotic C11 features since not all evironments have been speedy with supporting new things.

Yet another reason that I didn't go down the path of multiple-context support for JasPer earlier was that C11 was too new and I was therefore concerned that many C compilers might not yet have C11 threading support. I think that this is likely less of an issue today, since C11 is now about 10 years old. In any case, regardless of what threading support is used (e.g., C standard or POSIX Threads), there is the potential for problems due to some users not having the chosen threading support on their platform. So, if I were to include support for multiple contexts in JasPer, I think that I would probably need to allow this functionality to be a build-time configuration option that could be disabled (i.e., all of the multiple-context support could conditionally commented out by a build configuration parameter at build time).

--Michael

[bobfriesenhahn]

On Mon, 12 Jul 2021, Michael Adams wrote:

Thank you for the clarification as to what you meant by TSD. Currently, the JasPer library only requires a system that is compliant with the C standard, not POSIX. Portability has always been an important consideration in the development of JasPer. I know that some people/projects/organizations use the JasPer library on systems that are not POSIX compliant and have no POSIX Threads library. This precludes the use of this library in the case of these users. This is why I feel quite strongly that it would be better to rely on the C standard for threading support instead of relying on another library that is known not to be supported on some platforms of interest with C compilers. The threading support in C is implemented using whatever operating system primitives are provided natively. The main benefit of using the C language support for threading is that it is completely platform independent. Any code that requires the POSIX Thread library is, by definition, not completely platform independent, as such code requires a system with the POSIX Threads library. On POSIX systems, I strongly suspect that the C language threading support uses POSIX Threads underneath. So, I don't think that POSIX Threads are likely to be any more efficient than C threads.

I have not heard of any portability issues with GraphicsMagick's TSD portability wrapper, which was introduced in 2005.

Now that I have taken a look at C11 threads, I see that they are a thin veneer over the features/definitions which already existed for POSIX threads. POSIX threads have been available for use since 1996 and are supported on all Unix-type systems since that time, and are available under Cygwin and MinGW for Windows. I see that a C compiler can be C11 compliant yet not support C11 threads. If a C compiler supports C11 but does not support C11 threads, it is supposed to define STDC_NO_THREADS.

Regardless, given the existence of any reliable form of locking, it is easy to create TSD APIs (which I have also done) since locking is only required upon creation/destruction and not simple access.

It would be useful to see if Microsoft's Visual Studio compiler (MSVC) supports C11 threads.

Bob -- Bob Friesenhahn @.***, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer, http://www.GraphicsMagick.org/ Public Key, http://www.simplesystems.org/users/bfriesen/public-key.txt