Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document and fail fast if tmp directory is noexec #252

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

melissalinkert
Copy link
Member

Updates readme to reference the known noexec issue, and tries to detect and fail conversion quickly if that is the case. The check is very heavy-handed, as it should always fail if java.io.tmpdir is noexec - independent of the input format or conversion options. We could make that more lenient if any strong opinions.

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a system where /tmp partition is mounted with noexec options i.e.

omero@apps:/tmp$ mount | grep /tmp
/dev/sda on /tmp type ext4 (rw,noexec,relatime,discard)

running bioformats2raw e.g. against a NPDI file will fail with an exception looking like the following:

omero@apps:/data$ ~/apps/bioformats2raw-0.9.2/bin/bioformats2raw CMU-1.ndpi CMU-1.zarr
2024-06-28 08:50:16,044 [main] WARN  c.g.bioformats2raw.OpenCVTools - Could not load OpenCV libraries
java.lang.UnsatisfiedLinkError: /tmp/opencv_openpnp17542193582129005746/nu/pattern/opencv/linux/x86_64/libopencv_java470.so: /tmp/opencv_openpnp17542193582129005746/nu/pattern/opencv/linux/x86_64/libopencv_java470.so: failed to map segment from shared object
	at java.base/java.lang.ClassLoader$NativeLibrary.load0(Native Method)
	at java.base/java.lang.ClassLoader$NativeLibrary.load(ClassLoader.java:2450)
	at java.base/java.lang.ClassLoader$NativeLibrary.loadLibrary(ClassLoader.java:2506)
	at java.base/java.lang.ClassLoader.loadLibrary0(ClassLoader.java:2705)
	at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2635)
	at java.base/java.lang.Runtime.load0(Runtime.java:768)
	at java.base/java.lang.System.load(System.java:1850)
	at nu.pattern.OpenCV$LocalLoader.<init>(OpenCV.java:330)
	at nu.pattern.OpenCV$LocalLoader.<init>(OpenCV.java:326)
	at nu.pattern.OpenCV$LocalLoader$Holder.<clinit>(OpenCV.java:336)
	at nu.pattern.OpenCV$LocalLoader.getInstance(OpenCV.java:340)
	at nu.pattern.OpenCV.loadLocally(OpenCV.java:323)
	at com.glencoesoftware.bioformats2raw.OpenCVTools.loadOpenCV(OpenCVTools.java:34)
	at com.glencoesoftware.bioformats2raw.Converter.call(Converter.java:1151)
	at com.glencoesoftware.bioformats2raw.Converter.call(Converter.java:104)
	at picocli.CommandLine.executeUserObject(CommandLine.java:2041)
	at picocli.CommandLine.access$1500(CommandLine.java:148)
	at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2461)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2453)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2415)
	at picocli.CommandLine$AbstractParseResultHandler.handleParseResult(CommandLine.java:2264)
	at picocli.CommandLine.parseWithHandlers(CommandLine.java:2664)
	at picocli.CommandLine.parseWithHandler(CommandLine.java:2599)
	at picocli.CommandLine.call(CommandLine.java:2875)
	at com.glencoesoftware.bioformats2raw.Converter.main(Converter.java:2974)
2024-06-28 08:50:16,053 [main] WARN  c.g.bioformats2raw.OpenCVTools - Could not load native library opencv_java470
java.lang.UnsatisfiedLinkError: no opencv_java470 in java.library.path: [/usr/java/packages/lib, /usr/lib/x86_64-linux-gnu/jni, /lib/x86_64-linux-gnu, /usr/lib/x86_64-linux-gnu, /usr/lib/jni, /lib, /usr/lib]
	at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2678)
	at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:830)
	at java.base/java.lang.System.loadLibrary(System.java:1886)
	at com.glencoesoftware.bioformats2raw.OpenCVTools.loadOpenCV(OpenCVTools.java:41)
	at com.glencoesoftware.bioformats2raw.Converter.call(Converter.java:1151)
	at com.glencoesoftware.bioformats2raw.Converter.call(Converter.java:104)
	at picocli.CommandLine.executeUserObject(CommandLine.java:2041)
	at picocli.CommandLine.access$1500(CommandLine.java:148)
	at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2461)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2453)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2415)
	at picocli.CommandLine$AbstractParseResultHandler.handleParseResult(CommandLine.java:2264)
	at picocli.CommandLine.parseWithHandlers(CommandLine.java:2664)
	at picocli.CommandLine.parseWithHandler(CommandLine.java:2599)
	at picocli.CommandLine.call(CommandLine.java:2875)
	at com.glencoesoftware.bioformats2raw.Converter.main(Converter.java:2974)
Exception in thread "main" picocli.CommandLine$ExecutionException: Error while calling command (com.glencoesoftware.bioformats2raw.Converter@68e5eea7): java.io.IOException: JPEG service failed to load Turbo JPEG library
	at picocli.CommandLine.executeUserObject(CommandLine.java:2050)
	at picocli.CommandLine.access$1500(CommandLine.java:148)
	at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2461)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2453)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2415)
	at picocli.CommandLine$AbstractParseResultHandler.handleParseResult(CommandLine.java:2264)
	at picocli.CommandLine.parseWithHandlers(CommandLine.java:2664)
	at picocli.CommandLine.parseWithHandler(CommandLine.java:2599)
	at picocli.CommandLine.call(CommandLine.java:2875)
	at com.glencoesoftware.bioformats2raw.Converter.main(Converter.java:2974)
Caused by: java.io.IOException: JPEG service failed to load Turbo JPEG library
	at loci.formats.in.NDPIReader.initFile(NDPIReader.java:332)
	at loci.formats.FormatReader.setId(FormatReader.java:1480)
	at loci.formats.ImageReader.setId(ImageReader.java:865)
	at com.glencoesoftware.bioformats2raw.Converter.getBaseReaderClass(Converter.java:2791)
	at com.glencoesoftware.bioformats2raw.Converter.convert(Converter.java:1228)
	at com.glencoesoftware.bioformats2raw.Converter.call(Converter.java:1204)
	at com.glencoesoftware.bioformats2raw.Converter.call(Converter.java:104)
	at picocli.CommandLine.executeUserObject(CommandLine.java:2041)
	... 9 more

With this change the same command results in the following error

omero@apps:/data$ ~/apps/bioformats2raw-0.10.0-SNAPSHOT/bin/bioformats2raw CMU-1.ndpi CMU-1.zarr
Exception in thread "main" picocli.CommandLine$ExecutionException: Error while calling command (com.glencoesoftware.bioformats2raw.Converter@68e5eea7): java.lang.RuntimeException: /tmp is noexec; fix it or specify a different java.io.tmpdir
	at picocli.CommandLine.executeUserObject(CommandLine.java:2050)
	at picocli.CommandLine.access$1500(CommandLine.java:148)
	at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2461)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2453)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2415)
	at picocli.CommandLine$AbstractParseResultHandler.handleParseResult(CommandLine.java:2264)
	at picocli.CommandLine.parseWithHandlers(CommandLine.java:2664)
	at picocli.CommandLine.parseWithHandler(CommandLine.java:2599)
	at picocli.CommandLine.call(CommandLine.java:2875)
	at com.glencoesoftware.bioformats2raw.Converter.main(Converter.java:3017)
Caused by: java.lang.RuntimeException: /tmp is noexec; fix it or specify a different java.io.tmpdir
	at com.glencoesoftware.bioformats2raw.Converter.call(Converter.java:1190)
	at com.glencoesoftware.bioformats2raw.Converter.call(Converter.java:104)
	at picocli.CommandLine.executeUserObject(CommandLine.java:2041)
	... 9 more

Following the recommendations

omero@apps:/data$ JAVA_OPTS="-Djava.io.tmpdir=/opt/omero/tmp" ~/apps/bioformats2raw-0.10.0-SNAPSHOT/bin/bioformats2raw CMU-1.ndpi CMU-1.zarr
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.esotericsoftware.reflectasm.AccessClassLoader (file:/opt/omero/apps/bioformats2raw-0.10.0-SNAPSHOT/lib/reflectasm-1.11.9.jar) to method java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain)
WARNING: Please consider reporting this to the maintainers of com.esotericsoftware.reflectasm.AccessClassLoader
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
omero@apps:/data$ ls CMU-1.zarr/
0  1  OME

The requirement to mount /tmp as noexec is a guideline from the CIS security benchmarks. This means we should expect increasingly more systems to start enforcing this recommendation.
These mount options are at odds with the current logic of the native-lib-loader library used by Bio-Formats which extracts native libraries bundled in a JAR under java.io.tmpdir, usually /tmp, with executable permissions. This exact issue has already been reported in https://forum.image.sc/t/82646 and https://forum.image.sc/t/81868 and will affect file formats using libraries like libjpegturbo or libjxr.

Discussing potential upstream resolutions with @melissalinkert, one of the problem is that there is no authoritative way to identify an alternative temporary directory with write & executable permissions in all scenarios. As discussed recently with @stick, an application like OMERO.server might be able to create its own internal temporary directory like var/tmp and set java.io.tmpdir. On the other side, a library bioformats2raw might be used in variety of environments including containers, HPC.

Functionally, this PR is definitely an improvement, documents a known issue and provides the end-user with a more informative error on how to resolve it. As indicated in the description, the biggest concern is that the proposed implementation will always fail if /tmp is mounted as noexec even if no native library is required. It would be useful to get additional feedback from people working directly with end-users to report on the potential negative effect before spending additional time in investigating more lenient ways to send the error message.

@joshmoore
Copy link
Contributor

Two quick thoughts:

  • if possible, making this a flag might be worth the visibility and usability if this is going to become a more frequent issue
  • (possibly for upstream) I could also see this as a (personal) cache-dir scenario rather than temp-dir. If so, one could conceivably keep the files around until the next upgrade to speed up start up.

@melissalinkert
Copy link
Member Author

As discussed earlier today, the next steps here are:

  • keep the exception by default, but add an option to warn instead
  • make the readme changes a separate section (so we can link to it), and explain the issue a little more
  • delete the "check" file right away instead of on exit
  • include in a 0.10.0 (minor, not patch) release and make sure release notes flag this as a potentially breaking change

@melissalinkert
Copy link
Member Author

  • keep the exception by default, but add an option to warn instead
  • make the readme changes a separate section (so we can link to it), and explain the issue a little more
  • delete the "check" file right away instead of on exit
  • include in a 0.10.0 (minor, not patch) release and make sure release notes flag this as a potentially breaking change

All except the actual release should be covered by 9dca8a9, cc2ec93, and 862e4ac.

@melissalinkert melissalinkert added this to the 0.10.0 milestone Jul 15, 2024
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in a system with /tmp mounted as noexec.

bioformats2raw test.fake test.zarr fails but bioformats2raw test.fake test.zarr --warn-no-exec executes the conversion with a warning as expected.

Only issue that arose from the testing is the cleanup of the temporary test files when the command fails with the exception

throw new RuntimeException(msg);
}
}
tmpdirCheck.delete();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the scenario where /tmp is mounted as noxec and --warn-no-exec is not passed to the command, this deletion will never be executed as per the throw call a few lines earlier. Would it make sense to wrap this in a try/finally block to ensure temporary test files are always cleaned up?

README.md Outdated Show resolved Hide resolved
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retested with the last commit and the temporary file created for checking the executable state of java.io.tmpdir is now cleaned up on every command invocation independently of whether --warn-no-exec is passed.

boolean success = tmpdirCheck.setExecutable(true);
if (!success || !tmpdirCheck.canExecute()) {
String msg = System.getProperty("java.io.tmpdir") +
" is noexec; fix it or specify a different java.io.tmpdir";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See c772b46.

Copy link
Member

@stick stick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fine with the behavior and documentation from my end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants