Guard against segfaults due to missing Vips.init() call (fixes #175)

[jbaiter]

This adds a check to VipsInvoker that verifies that Vips.init was called before calling into libvips. This prevents users from running into segmentation faults (which is what happens at the moment if users forget to call Vips.init) and provides a better developer experience.

[lopcode]

Let me have a think about this one - I'm not sure it's unreasonable for users to use the init method in libvips itself (via VipsHelper for example).

There might still be a way to improve the developer experience by asking libvips itself if it's initialised, in VipsInvoker, and then set an AtomicBoolean similar to how you've done already to skip the check next time.

[lopcode]

@jbaiter Do you remember which function specifically caused the segfault, when init is omitted? I'd like to reproduce it.

[jbaiter]

It was in vips_foreign_load. I don't remember the specifics, but the list of modules was a nullpointer, iirc, and that caused the segmentation fault. There was a gobject warning as well. I'll give you a reproduction when the kids are asleep 😅

[lopcode]

No rush! I was more wondering what you did in vips-ffm that triggered the segfault, so I can write a sample for it.

[jbaiter]

It's pretty easy to reproduce, just call VImage.newFromFile without having called Vips.init before, and it will reliably segfault. Using a foreign loader directly does not cause the crash.

import app.photofox.vipsffm.VImage;
import app.photofox.vipsffm.Vips;

public class VipsDebug {
  public static void main(String[] args) {
    if (args.length != 1) {
      System.err.println("usage: java -jar vips-dbg.jar <filename>");
    }
    Vips.run(arena ->
        VImage.newFromFile(arena, args[0]).writeToFile("out-ffm.png")
    );
  }
}

$ java -Djextract.trace.downcalls=true --enable-native-access=ALL-UNNAMED -jar ./build/libs/vips-dbg-all.jar fox.jpg
vips_filename_get_filename(MemorySegment{ address: 0x7fd1b8194a20, byteSize: 8 })
vips_filename_get_options(MemorySegment{ address: 0x7fd1b82ebae0, byteSize: 8 })
vips_foreign_find_load(MemorySegment{ address: 0x7fd1b82f69c0, byteSize: 8 })

(process:1985772): GLib-GObject-CRITICAL **: 20:40:48.879: cannot retrieve class for invalid (unclassed) type '<invalid>'
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00007fd13da67dd4, pid=1985772, tid=1985773
#
# JRE version: OpenJDK Runtime Environment (24.0.2+12) (build 24.0.2+12-Debian-1)
# Java VM: OpenJDK 64-Bit Server VM (24.0.2+12-Debian-1, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# C  [libvips.so+0x67dd4]
#
# Core dump will be written. Default location: /tmp/core-1985772
#
# An error report file with more information is saved as:
# /tmp/vips-dbg/hs_err_pid1985772.log
[0.352s][warning][os] Loading hsdis library failed
#
# If you would like to submit a bug report, please visit:
#   https://bugs.debian.org/openjdk-24
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#
zsh: IOT instruction (core dumped)  LD_LIBARY_PATH= java -Djextract.trace.downcalls=true  -jar  fox.jpg

hs_err_pid1985772.log

[lopcode]

I wanted to double check this behaviour upstream, so asked a question of the team: libvips/libvips#4642

[jbaiter]

So I checked the libvips API, and what vips_init does internally, I don't think there's a way to reliably programmatically check if vips_init was called before.

But how about:

• A new internal state AtomicBoolean wasInitialized, as you suggested
• A new public API Vips.setInitialized() for advanced users who called vips_init themselves and want to let the bindings know about that
• If VipsInvoker#invokeOperation is called with wasInitialized unset, we can throw an IllegalStateOperation (or maybe follow the libvips pattern and call Vips.init() on behalf of the user and just log a warning?)

[lopcode]

I got some advice from the upstream maintainers - other bindings call init on the callers behalf when they first load the module.

That's not possible (or desirable, I think) with Java, but vips-ffm does have Vips.run which could be a good place for the approach you just shared.

So Vips.run first tries to initialise, if that hasn't already been done, and skips if it has.

[jbaiter]

👍🏾 so:

• auto-initialize in Vips.run if not already done by user.
• keep the check in VipsInvoker#invokeOperation?
• add the Vips#setInitialized() API

[lopcode]

I think that we may not need the setInitialized API - I don't believe there's any downside to initialising twice in libvips, it should just skip the second time itself. The check in Vips.run is just preventing an unnecessary FFM call.

Unsure about the check in VipsInvoker - the vips-ffm README clearly says you should be calling any operation using Vips.run, so maybe it's fine to just have it in there? What do you think, as a user?

[jbaiter]

Hm, the README currently only suggests Vips.run as a convenience for users who don't want to manage the FFM Arenas on their own. In my case, I was managing the Arena myself, so having Vips.run call Vips.init on my behalf wouldn't have prevented me from running into the segfault.

But I think, if the documentation stated more clearly that users either need to use Vips.run or call Vips.init themselves if they don't, this should be enough.

I think my problem was that the need for Vips.init was not explictly stated in the README, it's only included in the sample and it's not highlighted how critical it is. Combining the auto-initialization in Vips.run and a more explicit documentation would probably have prevented me from running into the segfault.

But then again, simply forgetting to call an initialization method shouldn't result in a Java library crashing the JVM if you ask me 😅

Two ways out of that that come to mind:

• Implement checks in Vips.invokeOperation as initially suggested
• Mandate the use of Vips.run via the API, i.e. don't make the lambda take a Arena, but force a final Wrapper VipsArena with a private constructor that can only be created by the library itself. That way users are forced to use the Vips.run pattern and can't shoot themselves in the foot anymore. This would be a very sweeping API change however, since all vips-ffm methods that now take an Arena would have to be changed to take a VipsArena.

[lopcode]

I've been contemplating whether having a static initialiser kick off a Vips.init call would be a good idea or not - that's pretty much the behaviour for bindings in other languages, and it could include a system property / env var check to disable it.