hw_X11 and hw_xft don't work as expected in some cases

[jafcobend]

The primary situation occurs when a TWIN session is launched without $DISPLAY being set and then you try to twattach from an X server on that same machine. So here's a common (for me) case:

1. SSH to a machine and start session (hw_tty).
2. Get physically back to that machine and from X twattach....
3. BANG "no `$DISPLAY set". Had me scratching my head. :-D

A second similar scenario where $DISPLAY causes unexpected things:

1. From an X session on a machine start a TWIN session (hw=X11 | hw_xft).
2. From another machine remote in to a VNC session on the first.
3. twattach --hw=X.... Huh?!?! nothing happened.
4. Get back to that machine and the local X session has this mysterious TWIN window open on it. :-)

And that scenario can be reversed. You have to twdisplay to attach it to a different X session (Xorg|VNC) on the same machine. This seems a waste and was certainly unexpected.

The issue seems to stem from how $DISPLAY is set (or not) when the initial TWIN session is started. This creates surprising situations for unsuspecting users. At least if they access their machine(s) in as many different ways as I do. ;-)

It would seem like this could work much better if twattach passed the current $DISPLAY from the environment in which it was launched. I see two potential issues:

1. Can libX attach to more than one $DISPLAY. In my case this is not likely to happen but wouldn't be a bad thing if it could work.
2. $DISPLAY was passed from a remote session and TWIN can't access the display or it attaches an unexpected display.

No. 2 seems the norm for anyone taking advantage of remote X (ie ssh -X). Making sure $DISPLAY is accurate for your remote scenario is the norm. And taking care of xhost to allow it if needed is also expected. I can see no.1 limiting TWIN to one X session at a time. IMO that's acceptable.

The current behavior is quite unexpected for the initiate to TWIN land. So I consider this a bug.

[cosmos72]

I agree that twattach behavior is unexpected in the cases you describe.
As you point out, in most (all?) cases, the solution is to use twdisplay instead, which is much more predictable.

It may help considering that twattach does "almost" nothing: it connects to an existing twin server, without passing its environment variables ($DISPLAY etc.), and merely asks the twin server to start a new display driver (the --hw=... argument specified in twattach command line). There is a little magic to determine twattach current's controlling tty if you specify --hw=tty, but that's all.

This means the environment where you execute twattach (inside a different X11 session, on a different machine, with different environment variables, etc.) has no effect at all: only the environment where you started the twin server is used.

Trying to fix twattach behavior by propagating its environment to twin server would cause more issues than it solves:
the server's idea of $DISPLAY may change - without updating the corresponding file ~/.Xauthority - which would prevent any direct connection from twin server to surrounding X11 or VNC servers. And similarly for the rest of the environment variables.

Again, the solution is to use twdisplay, which is designed exactly for complex situations, where the enviroment at the moment twin's server was started and the current environment where twdisplay is executed are different.

If you prefer an analogy, you can consider twattach as a close-to-metal tool to send raw commands to twin's server, and twdisplay as the equivalent of a VNC viewer.

[jafcobend]

the server's idea of $DISPLAY may change - without updating the corresponding file ~/.Xauthority - which would prevent any direct connection from twin server to surrounding X11 or VNC servers.

That behavior is expected.

And similarly for the rest of the environment variables.

I just mentioned passing $DISPLAY, nothing else. That would be unexpected.

OK. Challenge accepted! I'll give it a whirl. I think it will work well.

[cosmos72]

One very simple and very effective thing you (or I) can do is to modify twattach source as follows:

If it receives a command line argument --hw=X... or --hw=X11... or --hw=xft... which does not contain @SOME_X11_DISPLAY, it can make a copy of such argument and modify it to contain @MY_DISPLAY before sending it to twin server, where MY_DISPLAY is the value of environment variable $DISPLAY.

Doing so will instruct twin server to open an X11 window on the $DISPLAY indicated by the environment variables of twattach .

As you pointed out, it's a more intuitive behavior than using twin server's environment variable $DISPLAY

[jafcobend]

Already done. Seems to work well. ;-) Needs more testing.

[cosmos72]

Good :)
When you have tested it, feel free to create a pull request containing your changes - this solution does not nees to modify twin server's environment variables, so I have no objections in merging it

[jafcobend]

Another things to consider while rejecting this idea. ;-)

• User George logs in to X. Starts a TWIN session detaches and logs out of X. Jane logs in to X on the same machine and also starts a TWIN session. $DISPLAY in the first session is no longer valid.

The point is that $DISPLAY is highly volatile. There really is no point in hanging on to it. Might even be a good idea to expunge it from TWIN's environment... But somebody may have a use for it... starting X apps from TWIN? It will work in limited scenarios where there is one and only one user and the X server is always running and logged in as that one user. At least logged in when the user wants to start something X from TWIN.

[cosmos72]

Exactly.

Keeping the original value of $DISPLAY in twin server seems the least surprising behavior to me.

After the patch we are currently discussing is merged, twin server's environment variable $DISPLAY will be used in very few cases - for example if you open a builtin terminal in twin and then you try to launch some X11 program from it. Or if you launch an X11 program from twin's builtin menu File|Execute.

And anyway, if the user logged in changes, X11 server's .Xauthority changes too, so it will reject connections from clients launched by the previous user, as their .Xauthority is no longer valid.

And if someone uses xhost + or xhost +local: or something similar that disables .Xauthority check, they will get what they asked for: an insecure configuration that grants X11 access to other users.

[jafcobend]

Exactly.... and since we have a solution we can both agree on: enough said! :-D I'll be building packages shortly and then distribute it to my GUI systems. I'll re-run my twattach series and try to spot any "blunders" I've made. If all goes well you should have my patch tomorrow.

In general its against my religion to do anything M$. So I'll just submit the patch here. Its pretty small...

[jafcobend]

Well its working for me. Here's what I did:

diff --git a/clients/attach.c b/clients/attach.c
index 22fce5f..27d18e9 100644
--- a/clients/attach.c
+++ b/clients/attach.c
@@ -101,6 +101,20 @@ static void InitSignals(void) {
 #endif
 }
 
+static char *Xfix(char *arg) {
+  char *ps, *pd, *r, *DISPLAY;
+  int l;
+
+  DISPLAY = getenv("DISPLAY");
+  if(!DISPLAY || !*DISPLAY) return strdup(arg); // nothing to do w/o $DISPLAY
+  pd = r = malloc(strlen(arg)+strlen(DISPLAY)+2);
+  for(ps=arg; *ps && *ps!=','; *pd++ = *ps++);
+  *pd++ = '@';
+  strcpy(pd, DISPLAY);
+  if(*ps==',') strcat(pd, ps);
+  return r;
+}
+
 TW_DECL_MAGIC(attach_magic);
 
 int main(int argc, char *argv[]) {
@@ -186,6 +200,11 @@ int main(int argc, char *argv[]) {
           sprintf(arg, "-hw=tty%s", s);
         } else
           arg = strdup(*argv);
+      } else if(!strncmp(*argv + 4, "X", 1) && (!(*argv)[5] || (*argv)[5]==',')) {
+        arg = Xfix(*argv);
+      } else if((!strncmp(*argv + 4, "xft", 3) || !strncmp(*argv + 4, "X11", 3))
+               && (!(*argv)[7] || (*argv)[7]==',')) {
+        arg = Xfix(*argv);
       } else if ((*argv)[4])
         arg = strdup(*argv);
       else {

[cosmos72]

You approach is correct, but once again I ended up modifying your patch - see commit 8f4b7d1

As a side note, using twattach --hw={X,X11,xft} causes my twin server to crash if it already has an open X11 driver - I'll investigate it

[jafcobend]

You approach is correct, but once again I ended up modifying your patch

A shame. My solution was KISS++ and DRY++. It took care of all of the situations you mentioned in your email and the future potential or hacker shenanigans of hw driver names starting with the same prefix.

FYI: I did test all three driver names, with and without @ and with and without arguments. Some 12 test cases. Which used two different X servers on the same machine and were repeated with TWIN sessions started in and out of an X environment. Very tedious.

But at least others get the fix too.