[JENKINS-45892] Merged #2957: prevent model objects from being serial…

…ized except at top level.

core/src/main/java/hudson/XmlFile.java

@@ -50,8 +50,13 @@
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.Reader;
+import java.io.Serializable;
 import java.io.Writer;
 import java.io.StringWriter;
+import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.function.Supplier;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import org.apache.commons.io.IOUtils;
@@ -114,6 +119,7 @@
 public final class XmlFile {
     private final XStream xs;
     private final File file;
+    private static final Map<Object, Void> beingWritten = Collections.synchronizedMap(new IdentityHashMap<>());
 
     public XmlFile(File file) {
         this(DEFAULT_XSTREAM,file);
@@ -168,7 +174,12 @@ public void write( Object o ) throws IOException {
         AtomicFileWriter w = new AtomicFileWriter(file);
         try {
             w.write("<?xml version='1.0' encoding='UTF-8'?>\n");
-            xs.toXML(o,w);
+            beingWritten.put(o, null);
+            try {
+                xs.toXML(o, w);
+            } finally {
+                beingWritten.remove(o);
+            }
             w.commit();
         } catch(StreamException e) {
             throw new IOException(e);
@@ -177,6 +188,27 @@ public void write( Object o ) throws IOException {
         }
     }
 
+    /**
+     * Provides an XStream replacement for an object unless a call to {@link #write} is currently in progress.
+     * As per JENKINS-45892 this may be used by any class which expects to be written at top level to an XML file
+     * but which cannot safely be serialized as a nested object (for example, because it expects some {@code onLoad} hook):
+     * implement a {@code writeReplace} method delegating to this method.
+     * The replacement need not be {@link Serializable} since it is only necessary for use from XStream.
+     * @param o an object ({@code this} from {@code writeReplace})
+     * @param replacement a supplier of a safely serializable replacement object with a {@code readResolve} method
+     * @return {@code o}, if {@link #write} is being called on it, else the replacement
+     * @since 2.74
+     */
+    public static Object replaceIfNotAtTopLevel(Object o, Supplier<Object> replacement) {
+        if (beingWritten.containsKey(o)) {
+            return o;
+        } else {
+            // Unfortunately we cannot easily tell which XML file is actually being saved here, at least without implementing a custom Converter.
+            LOGGER.log(Level.WARNING, "JENKINS-45892: reference to {0} being saved but not at top level", o);
+            return replacement.get();
+        }
+    }
+
     public boolean exists() {
         return file.exists();
     }

core/src/main/java/hudson/model/AbstractItem.java

@@ -519,6 +519,24 @@ public final XmlFile getConfigFile() {
         return Items.getConfigFile(this);
     }
 
+    private Object writeReplace() {
+        return XmlFile.replaceIfNotAtTopLevel(this, () -> new Replacer(this));
+    }
+    private static class Replacer {
+        private final String fullName;
+        Replacer(AbstractItem i) {
+            fullName = i.getFullName();
+        }
+        private Object readResolve() {
+            Jenkins j = Jenkins.getInstanceOrNull();
+            if (j == null) {
+                return null;
+            }
+            // Will generally only work if called after job loading:
+            return j.getItemByFullName(fullName);
+        }
+    }
+
     public Descriptor getDescriptorByName(String className) {
         return Jenkins.getInstance().getDescriptorByName(className);
     }

core/src/main/java/hudson/model/Run.java

@@ -51,7 +51,6 @@
 import hudson.model.Descriptor.FormException;
 import hudson.model.listeners.RunListener;
 import hudson.model.listeners.SaveableListener;
-import hudson.model.queue.Executables;
 import hudson.model.queue.SubTask;
 import hudson.search.SearchIndexBuilder;
 import hudson.security.ACL;
@@ -774,6 +773,9 @@ public boolean hasntStartedYet() {
 
     @Override
     public String toString() {
+        if (project == null) {
+            return "<broken data JENKINS-45892>";
+        }
         return project.getFullName() + " #" + number;
     }
 
@@ -1938,6 +1940,19 @@ public synchronized void save() throws IOException {
         return new XmlFile(XSTREAM,new File(getRootDir(),"build.xml"));
     }
 
+    private Object writeReplace() {
+        return XmlFile.replaceIfNotAtTopLevel(this, () -> new Replacer(this));
+    }
+    private static class Replacer {
+        private final String id;
+        Replacer(Run<?, ?> r) {
+            id = r.getExternalizableId();
+        }
+        private Object readResolve() {
+            return fromExternalizableId(id);
+        }
+    }
+
     /**
      * Gets the log of the build as a string.
      * @return Returns the log or an empty string if it has not been found
@@ -2329,7 +2344,10 @@ public EnvVars getEnvironment() throws IOException, InterruptedException {
         } catch (NumberFormatException x) {
             throw new IllegalArgumentException(x);
         }
-        Jenkins j = Jenkins.getInstance();
+        Jenkins j = Jenkins.getInstanceOrNull();
+        if (j == null) {
+            return null;
+        }
         Job<?,?> job = j.getItemByFullName(jobName, Job.class);
         if (job == null) {
             return null;

core/src/main/java/hudson/model/User.java

@@ -735,6 +735,19 @@ public synchronized void save() throws IOException, FormValidation {
         SaveableListener.fireOnChange(this, getConfigFile());
     }
 
+    private Object writeReplace() {
+        return XmlFile.replaceIfNotAtTopLevel(this, () -> new Replacer(this));
+    }
+    private static class Replacer {
+        private final String id;
+        Replacer(User u) {
+            id = u.getId();
+        }
+        private Object readResolve() {
+            return getById(id, false);
+        }
+    }
+
     /**
      * Deletes the data directory and removes this user from Hudson.
      *

core/src/main/java/jenkins/model/RunAction2.java

@@ -29,6 +29,7 @@
 
 /**
  * Optional interface for {@link Action}s that add themselves to a {@link Run}.
+ * You may keep a {@code transient} reference to an owning build, restored in {@link #onLoad}.
  * @since 1.519, 1.509.3
  */
 public interface RunAction2 extends Action {

test/src/test/java/hudson/model/AbstractItem2Test.java

@@ -0,0 +1,71 @@
+/*
+ * The MIT License
+ *
+ * Copyright 2017 CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package hudson.model;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
+import org.junit.Test;
+import static org.junit.Assert.*;
+import org.junit.Rule;
+import org.junit.runners.model.Statement;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.RestartableJenkinsRule;
+
+public class AbstractItem2Test {
+
+    @Rule
+    public RestartableJenkinsRule rr = new RestartableJenkinsRule();
+
+    @Issue("JENKINS-45892")
+    @Test
+    public void badSerialization() {
+        rr.addStep(new Statement() {
+            @Override
+            public void evaluate() throws Throwable {
+                FreeStyleProject p1 = rr.j.createFreeStyleProject("p1");
+                p1.setDescription("this is p1");
+                FreeStyleProject p2 = rr.j.createFreeStyleProject("p2");
+                p2.addProperty(new BadProperty(p1));
+                String text = p2.getConfigFile().asString();
+                assertThat(text, not(containsString("<description>this is p1</description>")));
+                assertThat(text, containsString("<fullName>p1</fullName>"));
+            }
+        });
+        rr.addStep(new Statement() {
+            @Override
+            public void evaluate() throws Throwable {
+                FreeStyleProject p1 = rr.j.jenkins.getItemByFullName("p1", FreeStyleProject.class);
+                FreeStyleProject p2 = rr.j.jenkins.getItemByFullName("p2", FreeStyleProject.class);
+                assertEquals(/* does not work yet: p1 */ null, p2.getProperty(BadProperty.class).other);
+            }
+        });
+    }
+    static class BadProperty extends JobProperty<FreeStyleProject> {
+        final FreeStyleProject other;
+        BadProperty(FreeStyleProject other) {
+            this.other = other;
+        }
+    }
+
+}

test/src/test/java/hudson/model/RunActionTest.java

@@ -0,0 +1,73 @@
+/*
+ * The MIT License
+ *
+ * Copyright 2017 CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package hudson.model;
+
+import hudson.XmlFile;
+import java.io.File;
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.*;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runners.model.Statement;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.RestartableJenkinsRule;
+
+public class RunActionTest {
+
+    @Rule
+    public RestartableJenkinsRule rr = new RestartableJenkinsRule();
+
+    @Issue("JENKINS-45892")
+    @Test
+    public void badSerialization() {
+        rr.addStep(new Statement() {
+            @Override
+            public void evaluate() throws Throwable {
+                FreeStyleProject p = rr.j.createFreeStyleProject("p");
+                FreeStyleBuild b1 = rr.j.buildAndAssertSuccess(p);
+                FreeStyleBuild b2 = rr.j.buildAndAssertSuccess(p);
+                b2.addAction(new BadAction(b1));
+                b2.save();
+                String text = new XmlFile(new File(b2.getRootDir(), "build.xml")).asString();
+                assertThat(text, not(containsString("<owner class=\"build\">")));
+                assertThat(text, containsString("<id>p#1</id>"));
+            }
+        });
+        rr.addStep(new Statement() {
+            @Override
+            public void evaluate() throws Throwable {
+                FreeStyleProject p = rr.j.jenkins.getItemByFullName("p", FreeStyleProject.class);
+                assertEquals(p.getBuildByNumber(1), p.getBuildByNumber(2).getAction(BadAction.class).owner);
+            }
+        });
+    }
+    static class BadAction extends InvisibleAction {
+        final Run<?, ?> owner; // oops, should have been transient and used RunAction2
+        BadAction(Run<?, ?> owner) {
+            this.owner = owner;
+        }
+    }
+
+}

test/src/test/java/hudson/model/UserRestartTest.java

@@ -25,10 +25,13 @@
 package hudson.model;
 
 import hudson.tasks.Mailer;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
 import org.junit.Test;
 import static org.junit.Assert.*;
 import org.junit.Rule;
 import org.junit.runners.model.Statement;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.RestartableJenkinsRule;
 
 public class UserRestartTest {
@@ -58,4 +61,37 @@ public void evaluate() throws Throwable {
         });
     }
 
+    @Issue("JENKINS-45892")
+    @Test
+    public void badSerialization() {
+        rr.addStep(new Statement() {
+            @Override
+            public void evaluate() throws Throwable {
+                rr.j.jenkins.setSecurityRealm(rr.j.createDummySecurityRealm());
+                FreeStyleProject p = rr.j.createFreeStyleProject("p");
+                User u = User.get("pqhacker");
+                u.setFullName("Pat Q. Hacker");
+                u.save();
+                p.addProperty(new BadProperty(u));
+                String text = p.getConfigFile().asString();
+                assertThat(text, not(containsString("<fullName>Pat Q. Hacker</fullName>")));
+                assertThat(text, containsString("<id>pqhacker</id>"));
+            }
+        });
+        rr.addStep(new Statement() {
+            @Override
+            public void evaluate() throws Throwable {
+                FreeStyleProject p = rr.j.jenkins.getItemByFullName("p", FreeStyleProject.class);
+                User u = p.getProperty(BadProperty.class).user; // do not inline: call User.get second
+                assertEquals(User.get("pqhacker"), u);
+            }
+        });
+    }
+    static class BadProperty extends JobProperty<FreeStyleProject> {
+        final User user;
+        BadProperty(User user) {
+            this.user = user;
+        }
+    }
+
 }