TurboVNC / turbovnc

Main TurboVNC repository
https://TurboVNC.org
GNU General Public License v2.0
809 stars 143 forks source link

libawt search path improvement #169

Closed kisol-pw closed 5 years ago

kisol-pw commented 5 years ago

Hi, it's me again.

I found a little issue with the way the linux vncviewer script searches for libawt.so

In https://github.com/TurboVNC/turbovnc/blob/f570c5f2581c45a55636b09a7cee62ae1017ff03/unix/vncviewer/vncviewer.in#L26 JAWT_PATH is hard set to $JAVA_HOME/lib/$ARCH when JAVA_HOME is actually set. Well, turns out my dev-box has JAVA_HOME set to select one of many jdk. And not all of them have a $ARCH folder beneath lib: ./java-8-openjdk-amd64/lib/amd64/libjawt.so ./java-11-openjdk-amd64/lib/libjawt.so ./jdk1.5.0_22/jre/lib/amd64/libjawt.so ./jdk-9.0.4/lib/libjawt.so ./jdk-12/lib/libjawt.so ./jdk1.7.0_51/jre/lib/amd64/libjawt.so ./jdk-10.0.1/lib/libjawt.so ./jdk1.6.0_31/jre/lib/amd64/libjawt.so ./jdk1.8.0_172/lib/amd64/libjawt.so ./jdk-11.0.2/lib/libjawt.so

The following test in Line 33 actually works, but is skipped since JAWT_PATH gets set without testing whether the folder actually exists. Maybe something like

_TMP=$JAVA_HOME/lib
if [ -f "$_TMP/libjawt.so" ]; then
    JAWT_PATH=$_TMP
elif  [ -f "$_TMP/$ARCH/libjawt.so" ]; then
    JAWT_PATH=$_TMP/$ARCH
fi
dcommander commented 5 years ago

The simplest solution for TurboVNC 2.2.2 would probably be:

--- a/unix/vncviewer/vncviewer.in
+++ b/unix/vncviewer/vncviewer.in
@@ -19,7 +19,11 @@ if [ "$ARCH" = "x86_64" ]; then
 fi

 if [ "$JAVA_HOME" != "" ]; then
-       JAWT_PATH=$JAVA_HOME/lib/$ARCH
+       if [ -f "$JAVA_HOME/lib/$ARCH/libjawt.so" ]; then
+               JAWT_PATH=$JAVA_HOME/lib/$ARCH
+       elif [ -f "$JAVA_HOME/lib/libjawt.so" ]; then
+               JAWT_PATH=$JAVA_HOME/lib
+       fi
 fi

The evolving TurboVNC 3.0 code, since it no longer has to support Java 6 and earlier, can handle this a bit more elegantly:

--- a/unix/vncviewer/vncviewer.in
+++ b/unix/vncviewer/vncviewer.in
@@ -22,23 +22,15 @@ if [ "$ARCH" = "x86_64" ]; then
        ARCH=amd64
 fi

-if [ "$JAVA_HOME" != "" ]; then
-       JAWT_PATH=$JAVA_HOME/lib/$ARCH
-elif [ -d $BINDIR/../java/jre ]; then
-       JAWT_PATH=$BINDIR/../java/jre/lib/$ARCH
-fi
-
-if [ "$JAWT_PATH" = "" ]; then
-       # This should work with OpenJDK or Oracle Java 7 and later.
-       _TMP=`java -XshowSettings:properties -version 2>&1 | grep sun.boot.library.path | sed s/.*=\ //g`
-       if [ -f "$_TMP/libjawt.so" ]; then
-               JAWT_PATH=$_TMP
-       fi
+# This should work with OpenJDK or Oracle Java 7 and later.
+_TMP=`$JAVA -XshowSettings:properties -version 2>&1 | grep sun.boot.library.path | sed s/.*=\ //g`
+if [ -f "$_TMP/libjawt.so" ]; then
+       JAWT_PATH=$_TMP
 fi

 if [ "$JAWT_PATH" = "" ]; then
        # This may be required for IBM Java
-       _TMP=`java -XshowSettings:properties -version 2>&1 | grep java.home | sed s/.*=\ //g`
+       _TMP=`$JAVA -XshowSettings:properties -version 2>&1 | grep java.home | sed s/.*=\ //g`
        if [ -f "$_TMP/lib/$ARCH/libjawt.so" ]; then
                JAWT_PATH=$_TMP/lib/$ARCH
        fi
kisol-pw commented 5 years ago

Yes, scrapping the "if" is my current workaround too, since I mostly use 8 or 11 anyway (the java exec is PATH'ed by JAVA_HOME/bin/java on the dev-box in particular). The $JAVA selection also looks exactly how I have imagined it, that should work well.

I had been using the extended JAVA_HOME-if too, but been too lazy to type it all in after the last upgrade overwrote vncviewer again, oops.

dcommander commented 5 years ago

OK, I'll push the patches, and you can verify the pre-release builds to make sure everything still works.

dcommander commented 5 years ago

For future reference, can you tell me which Linux platform you are using? That will allow me to document the scope of the issue. (If I were a betting man, I'd bet you're going to say Arch.)

kisol-pw commented 5 years ago

make sure everything still works

will do (might take until Monday).

Close guess...: Debian sid - the lesser Arch, not less heavily modified for 10 years and counting though.

dcommander commented 5 years ago

Actually, upon closer inspection, this seems to be universally true of Java 9 and later. Here's my CentOS 7 system:

/usr/lib/jvm/java-1.7.0-openjdk-1.7.0.201-2.6.16.1.el7_6.x86_64/jre/lib/amd64/libjawt.so /usr/lib/jvm/java-1.6.0-openjdk-1.6.0.41.x86_64/jre/lib/amd64/libjawt.so /usr/lib/jvm/java-11-openjdk-11.0.1.13-3.el7_6.x86_64/lib/libjawt.so /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.191.b12-1.el7_6.x86_64/lib/amd64/libjawt.so /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.191.b12-1.el7_6.x86_64/jre/lib/amd64/libjawt.so

dcommander commented 5 years ago

Should now be fixed in both 2.2.x and 3.0 evolving builds

kisol-pw commented 5 years ago

Got around to test it. Both 2.2.2 and 3.0 work as expected. Thank you very much.

But holy apt-cow, 3.0 change 5 threw me a curve ball. As always, I casually typed vncviewer vm-w10-tst1809 and nothing happend at all. I first thought ultravnc server had broken now, but when starting the vncviewer with gui I saw the small print about ssh being the default now. It's certainly not right to discuss this here, but I've got to throw out a thought: "is this final?" I've the sneaking suspicion that this will break a lot of casual scripts that didn't go the extra mile to add :0 and kinda breaks conventions to other vncviewers (well at least tight.. tiger is doing whatever anyway) Also the drop down history still displays the previous hosts without the ports of course.

dcommander commented 5 years ago

Nothing is final until 3.0 beta is released, but it's incorrect to say that SSH is now "the default." The TurboVNC Session Manager, which uses SSH, is now the default in TurboVNC 3.0 if no port or display number is specified in the viewer. The TurboVNC Session Manager does more than just tunnel the connection through SSH. It also allows multiple TurboVNC Server sessions to be managed remotely.

You have to remember that, in addition to being an open source project, TurboVNC is also a standalone enterprise product used by many corporations and academic institutions. I don't charge money for the product; rather, I make money through support and funded development. However, in order to maximize that support and funded development revenue, I have to make TurboVNC as attractive as possible for enterprise users. Since our server, which only supports Un*x platforms, never listens on Display :0/Port 5900, the default behavior in the 3.0 TurboVNC Viewer is meant to provide the most intuitive user experience for those connecting to the 3.0 TurboVNC Server. TurboVNC has to compete with proprietary Linux remote desktop solutions, most of which allow you to connect to a server using only a hostname/IP address. Our inability to support any kind of session manager workflow has caused me to lose potential enterprise customers, so TurboVNC 3.0 is an attempt to reverse that. It's also an attempt to add value relative to other VNC solutions, to emphasize that TurboVNC-- while compatible with other VNC flavors-- is also its own thing. The ugly truth is that most of my revenue, and thus most of my ability to develop VirtualGL and TurboVNC and libjpeg-turbo full-time, comes from TurboVNC, so whither goest this project, so goest the others I maintain. Connecting to Display :0/Port 5900 requires a non-TurboVNC server, so it doesn't make sense to make that workflow more intuitive at the expense of making the workflow for our own server less intuitive.

I don't think it's too much to ask for users to add :0 to the hostname/IP address when connecting to non-TurboVNC servers that are attached to a physical display-- particularly since the viewer instructs you to do so right below the hostname/IP address field. I'm open to suggestions, though. Some possibilities I can think of:

  1. Perhaps it would make sense to revert to the TurboVNC 2.2.x default behavior (connecting to :0/5900 if the display/port # isn't specified) if the CompatibleGUI option is enabled, since that option is designed for use with non-TurboVNC servers. Before 3.0 beta is released, I'm going to make some deep modifications to the viewer's Options dialog, which will include adding a check box to enable/disable CompatibleGUI and also storing settings on a connection-by-connection basis. Thus, with the final 3.0 viewer, it will be easy to enable CompatibleGUI for a particular server and have that setting be persistent whenever you reconnect to the same server.
  2. The session manager could be disabled when a particular Java system property (turbovnc.sessmgr or whatnot) is set to false. That would allow you to disable it globally by setting the JAVA_TOOL_OPTIONS environment variable.
kisol-pw commented 5 years ago

Yeah sure, that's totally fine. It just caught me offguard since I didn't bother to read the preliminary changelog and the 'ingrained expected behavior' suddenly changed. Playing around with the Session Manager feature is actually something I'm looking forward to do.

Fun story to consider regarding catering to enterprise use when changes go too far: My corp is dealing in high security control systems and had licensed the big-R vnc for the encryption and really nice client scaling. Then they went full blown cloud with their lesser offerings, switched to an odd abo model for the hiend stuff and changing the viewer a tad too much. Well the auditors pulled the plug and even banned it internally. There goes a few thousand licenses per year... well, guess they just moved their market.

For my part I switched to Turbo (hence me posting issues) since its client can do scaling, is fast and works well with multimonitor setup (and supports the old pixmap-mask cursor stuff our old software tends to use which tiger fails to draw correctly). For the server part, that clustered away in whatever is available.. tight, tiger, ultra, turbo and some remaining real5.

Sadly as any good corp would do, they are too cheap to invest in a tool if they can somehow juryrig something with free stuff as long as it passes auditing. Therefore I'll be heading over to the donation pot now and add some personal funds.

Keep up the great work, greatly appreciated! I'll be calling back when I find another hole popping up.

P.S. having the option to tell software to shutit and do how I want is always appreciated, too. I can actually imagine use cases for both 'em options. 1 is nice when I use the gui to select between the many VMs on our servers or at home switching from laptop to htpc. 2 is useful when deployed in environments where the common user has to be restricted when he accidentally gains console access (yes someone decided to put ISO27001 into a law giving auditors the right to dictate what must be limited when using vnc or rdp.. no file transfer, no chat, any tunneling is a no no and we even had one that wanted f8 disabled).

dcommander commented 5 years ago

Excellent feedback! I'll add a new issue for providing both ways of disabling the TSM.

dcommander commented 5 years ago

And thank you very much for the donation!

dcommander commented 5 years ago

Following up on your point about enterprise users, what specifically did the big R change that made their viewer suddenly be rejected? I also don't know what ABO stands for in that context. Also, if there is any value-add service you can think of that I am capable of providing-- or any particular support model-- that enterprise customers might be willing to pay an open source developer for, please let me know. Just trying to improve my understanding of that market in general. If you prefer to e-mail me, feel free: https://turbovnc.org/About/Contact