Test data here:

https://github.com/uclouvain/openjpeg-data/tree/master/baseline/nonregression

With this PR, we now load Bretagne2_4.j2k correctly.

Bretagne2_1.j2k is still crashing argh.

OK, this now seems to load all the openjpeg test images correctly.

Thanks again for reporting this, @mika-fischer

This will be a pain to review :( sorry.

I added two tricky jp2k images from the openjpeg set to our test suite.

Perhaps this should target the 8.15 branch instead? Of course, I can also backport it with a follow-up PR if that's easier.

FWIW, LLVM does this very neatly with a bot that automates the entire process via /cherry-pick <commit> <...>.
https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches
https://github.com/llvm/llvm-project/blob/main/.github/workflows/issue-release-workflow.yml

I was thinking about adopting a similar approach for libvips, which would avoid the need to retarget PRs and regularly merging release branches back into the master branch.

This will be a pain to review :( sorry.

Don't worry, reviewing this with the Hide whitespace changes option makes it a bit easier.

Thanks for working on this!

I can confirm the two files above are working now.

Unfortunately, I still get crashes with the following files :-/

.\openjpeg-data\input\conformance\file9.jp2
-1073740940 0xC0000374 STATUS_HEAP_CORRUPTION
.\openjpeg-data\input\nonregression\issue104_jpxstream.jp2
-1073741819 0xC0000005 STATUS_ACCESS_VIOLATION
.\openjpeg-data\input\nonregression\issue235.jp2
-1073740940 0xC0000374 STATUS_HEAP_CORRUPTION
.\openjpeg-data\input\nonregression\issue254.jp2
-1073740940 0xC0000374 STATUS_HEAP_CORRUPTION
.\openjpeg-data\input\nonregression\issue412.jp2
-1073740940 0xC0000374 STATUS_HEAP_CORRUPTION
.\openjpeg-data\input\nonregression\mem-b2b86b74-2753.jp2
-1073740940 0xC0000374 STATUS_HEAP_CORRUPTION

It feels like a relatively large change, so I was thinking 8.16 would be a better place for it.

I'll have a look at the LLVM doc, thanks!

Unfortunately, I still get crashes with the following files

Oh poop. I'll have a look.

I noticed imagemagick gets quite a few of the openjpeg test files wrong too, fwiw. Their API really is very hard to work with, but still somehow easier than kakadu hehe.

openjpeg-data\input\conformance\file9.jp2 has a wrapper that says it's monochrome, but when you decode it you get RGB pixels.

libvips won't be able to support jp2ks like this (you don't know what format they are until after you have decoded them), unfortunately. I'll have to add a lot of sanity checks on every decode operation.

(what a ridiculous format! why is this allowed?)

I feel your pain 😬

Just to be clear, I don't really care whether vips can read any of these files correctly. It just should not crash, since we use vips as a library...

I agree it shouldn't crash.

You could just disable the libvips openjpeg loader, of course, that would probably be a safer fix. If you set the VIPS_BLOCK_UNTRUSTED env var, libvips won't run any loaders we haven't fuzzed ourselves.

You could just disable the libvips openjpeg loader, of course, that would probably be a safer fix.

Sure, that's what we did for the moment (falling back to ffmpeg). But of course one of the reasons we use vips in the first place is to be able to read formats like JPEG2000.

If you set the VIPS_BLOCK_UNTRUSTED env var, libvips won't run any loaders we haven't fuzzed ourselves.

I see, but we need JPEG2000, JPEG-XL & PPM, which we would lose that way, so that's not really a good option...

It's now detecting when decoded pixels don't match the jpeg2000 header and flagging an error, which seems to fix that set of crashes.

we need JPEG2000, JPEG-XL & PPM,

Ah OK. As long as your users aren't uploading these strange jpeg2000 files, then it should work now.

Great, thanks! I can confirm that the crashes are all gone and there are no valgrind warnings for the whole openjpeg folder.

Great! Thanks for testing @mika-fischer, and thanks for reporting this.