jline / jline3

JLine is a Java library for handling console input.
Other
1.49k stars 218 forks source link

Remove execute permission on native .so file #1081

Closed ddanielr closed 1 month ago

ddanielr commented 1 month ago

Removes the execute permission on the .so native library file created in the java tmp directory. This permission is not required for java to load the library file successfully.

JLine creates two files in a temp directory location when being used in an interactive application.

-rwx------. jlinenative-3.27.0-862111cc4ad0a980-libjlinenative.so
-rw-------.  jlinenative-3.27.0-862111cc4ad0a980-libjlinenative.so.lck

The .so file has read, write, and execute permissions set on it.

The temp directory path set by java.io.tmpdir is typically /tmp or /var/tmp. However these directories are commonly set with the noexec flag to conform with security best practices. example: https://www.stigviewer.com/stig/red_hat_enterprise_linux_8/2023-12-01/finding/V-230513

When the noexec flag is set, JLine is prevented from creating the native lib file and functioning correctly. However after removing the execute permission from file creation, the native library file was able to be loaded successfully without issue.

gnodet commented 1 month ago

When the noexec flag is set, JLine is prevented from creating the native lib file and functioning correctly. However after removing the execute permission from file creation, the native library file was able to be loaded successfully without issue.

What happens exactly ? My understanding is that the noexec flag simply forbids the execution of the file. It should not affect the fact that the JVM reads the library to load it...

ddanielr commented 1 month ago

When the noexec flag is set, JLine is prevented from creating the native lib file and functioning correctly. However after removing the execute permission from file creation, the native library file was able to be loaded successfully without issue.

What happens exactly ? My understanding is that the noexec flag simply forbids the execution of the file. It should not affect the fact that the JVM reads the library to load it...

So I reviewed my test code and my test conditions weren't correct. The file creation and setting of the execute permissions does work.

With a tmp directory set with noexec permissions, I can see the .so file get created with execute permissions. So my previous statement about that part failing is incorrect.

The code returns a log message:

org.jline.nativ.JLineNativeLoader log
WARNING: Failed to load native library: jlinenative-3.27.0-862111cc4ad0a980-libjlinenative.so. osinfo: Linux/x86_64 (caused by: java.lang.UnsatisfiedLinkError: /tmp/jlinenative-3.27.0-862111cc4ad0a980-libjlinenative.so: /tmp/jlinenative-3.27.0-862111cc4ad0a980-libjlinenative.so: failed to map segment from shared object, enable debug logging for stacktrace)

Which seems to be generated here: https://github.com/jline/jline3/blob/1f56fba1c57edd2838401a3b3d554f4531252517/native/src/main/java/org/jline/nativ/JLineNativeLoader.java#L258-L263

My original concern was that our application would fall over after jline failed to load the native library. However that failure seems to have been unrelated. I'm not certain if that's because I'm not triggering a condition that would use the native lib contents, but in my current testing I cannot replicate the failure. We see the warning log message but the application continues to function.

Given that @ctubbsii was able to replicate this with an unrelated library file I agree that the issue is coming up from the underlying system loader so the change in this PR doesn't seem warranted.

gnodet commented 1 month ago

Another option is to provide a separate system property like jline.native.tmpdir or a similarly named property, which defaults to the value of java.io.tmpdir, but can be configured independently if the user wishes.

JLine uses the jline.tmpdir which takes precedence over the java.io.tmpdir when set.

gnodet commented 1 month ago

@ddanielr @ctubbsii thx a lot for taking the time to investigate this issue. I'll close this one and opened a new one do better document the loading process.

ctubbsii commented 1 month ago

@gnodet I didn't realize a different property already existed! That helps as a workaround. Thanks!

I'm still concerned about the unchecked RuntimeException that gets thrown at https://github.com/jline/jline3/blob/b2749c4525e658c326d9978d802ac8678c954e35/native/src/main/java/org/jline/nativ/JLineNativeLoader.java#L80

There are several classes that call JLineNativeLoader.initialize() in static initializer blocks. If that throws an exception, then the class fails to be initialized by the classloader and won't be available for use. There is also the ExecTerminalProvider and AbstractPty classes that will rely on the native library, depending on the value of the system properties, org.jline.terminal.pty.fileDescriptorCreationMode and org.jline.terminal.exec.redirectPipeCreationMode. I believe both of these default to reflection,native, and try reflection first. So, that might be why @ddanielr describes only an error message, but things proceeding as expected after that, without an obvious failure. Chances are, one of the classes failed to load from the classloader because the native library could not be loaded in one of the static initializer blocks, but the code path never actually tried to use any of those classes, because everything defaulted to the reflection implementations. At this point, though I'm really just speculating. However, I do think that the load should probably not throw a RuntimeException that falls through the static initializer blocks and prevents those classes from being loaded.