Alternate placing of the xauthority file

A workaround for JENKINS-19139. If the workspace path contains spaces it will try to put the file in the slave fs root, and if that path also contains spaces it will try to put it in java.io.tmpdir.
jenkinsci · Aug 26, 2015 · a599340 · a599340
1 parent b3fdaf4
commit a599340
Show file tree

Hide file tree

Showing 2 changed files with 188 additions and 15 deletions.
diff --git a/src/main/java/hudson/plugins/xvnc/Xvnc.java b/src/main/java/hudson/plugins/xvnc/Xvnc.java
@@ -12,10 +12,12 @@
 import hudson.model.Node;
 import hudson.model.Run;
 import hudson.model.TaskListener;
+import hudson.remoting.VirtualChannel;
 import hudson.tasks.BuildWrapper;
 import hudson.tasks.BuildWrapperDescriptor;
 import hudson.util.FormValidation;
 
+import java.io.File;
 import java.io.IOException;
 import java.io.PrintStream;
 import java.util.Collections;
@@ -25,7 +27,9 @@
 import java.util.WeakHashMap;
 import javax.annotation.CheckForNull;
 
+import jenkins.MasterToSlaveFileCallable;
 import jenkins.model.Jenkins;
+import jenkins.security.MasterToSlaveCallable;
 import jenkins.tasks.SimpleBuildWrapper;
 import jenkins.util.BuildListenerAdapter;
 import net.sf.json.JSONObject;
@@ -97,18 +101,20 @@ private void doSetUp(Context context, Run<?,?> build, FilePath workspace, Node n
         final DisplayAllocator allocator = getAllocator(node);
 
         final int displayNumber = allocator.allocate(minDisplayNumber, maxDisplayNumber);
-        final String actualCmd = Util.replaceMacro(cmd, Collections.singletonMap("DISPLAY_NUMBER",String.valueOf(displayNumber)));
+        final String actualCmd = Util.replaceMacro(cmd, Collections.singletonMap("DISPLAY_NUMBER", String.valueOf(displayNumber)));
 
         logger.println(Messages.Xvnc_STARTING());
 
         String[] cmds = Util.tokenize(actualCmd);
 
-        final FilePath xauthority = workspace.createTempFile(".Xauthority-", "");
+        final FilePath xauthority;
         final Map<String,String> xauthorityEnv = new HashMap<String, String>();
         if (useXauthority) {
+            xauthority = createXauthorityFile(workspace, logger);
             xauthorityEnv.put("XAUTHORITY", xauthority.getRemote());
             context.env("XAUTHORITY", xauthority.getRemote());
         } else {
+            xauthority = null;
             // Need something to identify it by for Launcher.kill in DisposerImpl.
             xauthorityEnv.put("XVNC_COOKIE", UUID.randomUUID().toString());
         }
@@ -139,20 +145,56 @@ private void doSetUp(Context context, Run<?,?> build, FilePath workspace, Node n
         }
 
         context.env("DISPLAY", ":" + displayNumber);
-        context.setDisposer(new DisposerImpl(displayNumber, xauthorityEnv, vncserverCommand, takeScreenshot, xauthority.getRemote()));
+        context.setDisposer(new DisposerImpl(displayNumber, xauthorityEnv, vncserverCommand, takeScreenshot, xauthority != null ? xauthority.getRemote() : null));
     }
-
+
+    /**
+     * Attempts to find a suitable place for the Xauthority file where the path to it doesn't contain any spaces.
+     * The order of tries is the workspace, the slave's fs root and last the system temp dir.
+     * If the system temp dir also contains a space a warning will be printed to the log but the temp dir path will
+     * be created and returned anyways.
+     * @param workspace the build's workspace.
+     * @param logger the build's log to print the warning to.
+     * @return the path on the slave to the created temp file.
+     *
+     * @throws IOException if so
+     * @throws InterruptedException if so
+     */
+    private FilePath createXauthorityFile(FilePath workspace, final PrintStream logger) throws IOException, InterruptedException {
+        if (workspace.getRemote().indexOf(' ') < 0) {
+            //If the workspace doesn't have any spaces it is probably safe
+            return workspace.createTempFile(".Xauthority-", "");
+        } else {
+            //Try the fs root
+            Computer computer = workspace.toComputer();
+            Node node = computer.getNode();
+            FilePath rootPath = node.getRootPath();
+            if (rootPath.getRemote().indexOf(' ') < 0) {
+                return rootPath.createTempFile(".Xauthority-", "");
+            } else {
+                FilePath file = workspace.createTextTempFile(".Xauthority-", "", "", false);
+                if (file.getRemote().indexOf(' ') >= 0) {
+                    logger.println("WARNING! Could not find somewhere to place the Xauthirity file not containing a space in the path.");
+                }
+                return file;
+            }
+        }
+
+    }
+
     private static class DisposerImpl extends Disposer {
 
         private static final long serialVersionUID = 1;
 
         private final int displayNumber;
         private final Map<String,String> xauthorityEnv;
-        private final @CheckForNull String vncserverCommand;
+        @CheckForNull
+        private final String vncserverCommand;
         private final boolean takeScreenshot;
+        @CheckForNull
         private final String xauthorityPath;
 
-        DisposerImpl(int displayNumber, Map<String,String> xauthorityEnv, @CheckForNull String vncserverCommand, boolean takeScreenshot, String xauthorityPath) {
+        DisposerImpl(int displayNumber, Map<String,String> xauthorityEnv, @CheckForNull String vncserverCommand, boolean takeScreenshot, @CheckForNull String xauthorityPath) {
             this.displayNumber = displayNumber;
             this.xauthorityEnv = xauthorityEnv;
             this.vncserverCommand = vncserverCommand;
@@ -188,7 +230,9 @@ private static class DisposerImpl extends Disposer {
                 throw new AbortException("No node recognized for " + workspace);
             }
             getAllocator(node).free(displayNumber);
-            workspace.child(xauthorityPath).delete();
+            if (xauthorityPath != null) {
+                new FilePath(workspace.toComputer().getChannel(), xauthorityPath).delete();
+            }
         }
     }
 

diff --git a/src/test/java/hudson/plugins/xvnc/XvncTest.java b/src/test/java/hudson/plugins/xvnc/XvncTest.java
@@ -23,27 +23,40 @@
  */
 package hudson.plugins.xvnc;
 
+import hudson.FilePath;
 import hudson.Launcher;
+import hudson.model.AbstractBuild;
 import hudson.model.BuildListener;
+import hudson.model.Computer;
+import hudson.model.Descriptor;
 import hudson.model.FreeStyleBuild;
-import hudson.model.Result;
-import hudson.model.AbstractBuild;
 import hudson.model.FreeStyleProject;
 import hudson.model.Hudson;
 import hudson.model.Node;
+import hudson.model.Result;
+import hudson.model.TaskListener;
 import hudson.plugins.xvnc.Xvnc.DescriptorImpl;
+import hudson.slaves.ComputerListener;
 import hudson.slaves.DumbSlave;
+import hudson.slaves.RetentionStrategy;
 import hudson.tasks.Builder;
 import hudson.util.OneShotEvent;
-
-import java.io.IOException;
-import java.util.concurrent.Future;
-import static org.junit.Assert.assertTrue;
-
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.Bug;
 import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.TemporaryDirectoryAllocator;
+import org.jvnet.hudson.test.TestBuilder;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.URISyntaxException;
+import java.util.Collections;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Future;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 public class XvncTest {
 
@@ -134,6 +147,118 @@ public void avoidNpeAfterDeserialiation() throws Exception {
         j.buildAndAssertSuccess(p);
     }
 
+    public void testXauthorityInWorkspace() throws Exception {
+        DumbSlave slave = j.createOnlineSlave();
+
+        FreeStyleProject job = j.jenkins.createProject(FreeStyleProject.class, "jobA");
+        job.setAssignedNode(slave);
+
+        runXvnc(job, true);
+
+        job.getBuildersList().add(new TestBuilder() {
+            @Override
+            public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
+                FilePath[] list = build.getWorkspace().list(".Xauthority-*");
+                assertEquals("There should be one Xauthority file in the workspace", 1, list.length);
+                assertEquals(build.getWorkspace().getRemote(), list[0].getParent().getRemote());
+                return true;
+            }
+        });
+        j.buildAndAssertSuccess(job);
+
+        FilePath[] list = job.getLastBuild().getWorkspace().list(".Xauthority-*");
+        assertEquals("The Xauthority file should be removed", 0, list.length);
+    }
+
+    @Test
+    public void testXauthorityInSlaveFsRoot() throws Exception {
+        final DumbSlave slave = j.createOnlineSlave();
+
+        FreeStyleProject job = j.jenkins.createProject(FreeStyleProject.class, "job A");
+        job.setAssignedNode(slave);
+
+        runXvnc(job, true);
+
+        job.getBuildersList().add(new TestBuilder() {
+            @Override
+            public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
+                FilePath[] list = slave.getRootPath().list(".Xauthority-*");
+                assertEquals("There should be one Xauthority file in the slave fs root", 1, list.length);
+                assertEquals(slave.getRootPath().getRemote(), list[0].getParent().getRemote());
+
+                list = build.getWorkspace().list(".Xauthority-*");
+                assertEquals("There should be no Xauthority file in the workspace", 0, list.length);
+
+                return true;
+            }
+        });
+        j.buildAndAssertSuccess(job);
+
+        FilePath[] list = slave.getRootPath().list(".Xauthority-*");
+        assertEquals("The Xauthority file should be removed", 0, list.length);
+    }
+
+    @Test
+    public void testXauthorityInTemp() throws Exception {
+        final File base = new File(System.getProperty("java.io.tmpdir"), "some bad path");
+        base.mkdirs();
+        final TemporaryDirectoryAllocator directoryAllocator = new TemporaryDirectoryAllocator(base);
+        try {
+            final DumbSlave slave = createTweakedSlave(directoryAllocator);
+            final FilePath tmpPath = new FilePath(slave.getChannel(), System.getProperty("java.io.tmpdir"));
+            FreeStyleProject job = j.jenkins.createProject(FreeStyleProject.class, "job A");
+            job.setAssignedNode(slave);
+
+            runXvnc(job, true);
+
+            job.getBuildersList().add(new TestBuilder() {
+                @Override
+                public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
+
+                    FilePath[] list = tmpPath.list(".Xauthority-*");
+                    assertEquals("There should be one Xauthority file in tmp", 1, list.length);
+                    assertEquals(tmpPath.getRemote(), list[0].getParent().getRemote());
+
+                    list = build.getWorkspace().list(".Xauthority-*");
+                    assertEquals("There should be no Xauthority file in the workspace", 0, list.length);
+
+                    list = slave.getRootPath().list(".Xauthority-*");
+                    assertEquals("There should be no Xauthority file in the slave fs root", 0, list.length);
+
+                    return true;
+                }
+            });
+            j.buildAndAssertSuccess(job);
+
+            FilePath[] list = tmpPath.list(".Xauthority-*");
+            assertEquals("The Xauthority file should be removed", 0, list.length);
+        } finally {
+            directoryAllocator.dispose();
+        }
+    }
+
+    private DumbSlave createTweakedSlave(TemporaryDirectoryAllocator directoryAllocator) throws IOException, Descriptor.FormException, URISyntaxException, InterruptedException {
+        final CountDownLatch latch = new CountDownLatch(1);
+        ComputerListener waiter = new ComputerListener() {
+            @Override
+            public void onOnline(Computer C, TaskListener t) {
+                latch.countDown();
+                unregister();
+            }
+        };
+        waiter.register();
+        //Create a slave manually
+        final DumbSlave slave;
+        synchronized (j.jenkins) {
+            slave = new DumbSlave("SlaveX", "dummy",
+                    directoryAllocator.allocate().getPath(), "1", Node.Mode.NORMAL, "", j.createComputerLauncher(null), RetentionStrategy.NOOP, Collections.EMPTY_LIST);
+            j.jenkins.addNode(slave);
+
+        }
+        latch.await();
+        return slave;
+    }
+
     private Xvnc fakeXvncRun(FreeStyleProject p) throws Exception {
         final Xvnc xvnc = new Xvnc(false, false);
         p.getBuildWrappersList().add(xvnc);
@@ -145,7 +270,11 @@ private Xvnc fakeXvncRun(FreeStyleProject p) throws Exception {
     }
 
     private Xvnc.DescriptorImpl runXvnc(FreeStyleProject p) throws Exception {
-        final Xvnc xvnc = new Xvnc(false, false);
+        return runXvnc(p, false);
+    }
+
+    private Xvnc.DescriptorImpl runXvnc(FreeStyleProject p, boolean useXauthority) throws Exception {
+        final Xvnc xvnc = new Xvnc(false, useXauthority);
         p.getBuildWrappersList().add(xvnc);
         DescriptorImpl descriptor = Hudson.getInstance().getDescriptorByType(DescriptorImpl.class);
         descriptor.maxDisplayNumber = descriptor.minDisplayNumber = 42;