diff --git a/core/src/main/java/hudson/XmlFile.java b/core/src/main/java/hudson/XmlFile.java index 72d4d6c128ac605d4abd74d0d0587fb2d39af508..3c0a68bee36d61251cf369897cab4eed692c8692 100644 --- a/core/src/main/java/hudson/XmlFile.java +++ b/core/src/main/java/hudson/XmlFile.java @@ -50,8 +50,13 @@ import java.io.IOException; 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 @@ import org.apache.commons.io.IOUtils; public final class XmlFile { private final XStream xs; private final File file; + private static final Map beingWritten = Collections.synchronizedMap(new IdentityHashMap<>()); public XmlFile(File file) { this(DEFAULT_XSTREAM,file); @@ -168,7 +174,12 @@ public final class XmlFile { AtomicFileWriter w = new AtomicFileWriter(file); try { w.write("\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 final class XmlFile { } } + /** + * 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 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(); } diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index 8cd5a1625e3640bfbc21674f2ad16df1eafd1634..424e934e31aedf5860bb18b74156ebd55629a76c 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -519,6 +519,24 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet 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); } diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java index 84a31086a9502b02940931a2c32b9819c36dfdc9..8cf50d2fb47cb8e0caf0e2a5d233729922cd863e 100644 --- a/core/src/main/java/hudson/model/Run.java +++ b/core/src/main/java/hudson/model/Run.java @@ -51,7 +51,6 @@ import hudson.cli.declarative.CLIMethod; 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 abstract class Run ,RunT extends Run"; + } return project.getFullName() + " #" + number; } @@ -1938,6 +1940,19 @@ public abstract class Run ,RunT extends Run 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 abstract class Run ,RunT extends Run job = j.getItemByFullName(jobName, Job.class); if (job == null) { return null; diff --git a/core/src/main/java/hudson/model/User.java b/core/src/main/java/hudson/model/User.java index 845d76986282a236f2dc5b35149c3351a8d4a030..902ae36e60d19e6a10bf785a14a94ce530cfd8da 100644 --- a/core/src/main/java/hudson/model/User.java +++ b/core/src/main/java/hudson/model/User.java @@ -735,6 +735,19 @@ public class User extends AbstractModelObject implements AccessControlled, Descr 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. * diff --git a/core/src/main/java/jenkins/model/RunAction2.java b/core/src/main/java/jenkins/model/RunAction2.java index b5443bcccb19b7df05a20dd814c1c413c05f6e1a..1f180e5b9919db964e518640fb5ee18e0e938893 100644 --- a/core/src/main/java/jenkins/model/RunAction2.java +++ b/core/src/main/java/jenkins/model/RunAction2.java @@ -29,6 +29,7 @@ import hudson.model.Run; /** * 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 { diff --git a/test/src/test/java/hudson/model/AbstractItem2Test.java b/test/src/test/java/hudson/model/AbstractItem2Test.java new file mode 100644 index 0000000000000000000000000000000000000000..a35044ee8782236f99ef3a53f2d6efc10bbf6c4b --- /dev/null +++ b/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("this is p1"))); + assertThat(text, containsString("p1")); + } + }); + 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 { + final FreeStyleProject other; + BadProperty(FreeStyleProject other) { + this.other = other; + } + } + +} diff --git a/test/src/test/java/hudson/model/RunActionTest.java b/test/src/test/java/hudson/model/RunActionTest.java new file mode 100644 index 0000000000000000000000000000000000000000..16aac5e404a15e0f092cd176af89161b7fb7d028 --- /dev/null +++ b/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(""))); + assertThat(text, containsString("p#1")); + } + }); + 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; + } + } + +} diff --git a/test/src/test/java/hudson/model/UserRestartTest.java b/test/src/test/java/hudson/model/UserRestartTest.java index 43fbbc80ec4e4b3f125b79aa44091ae7cceda1e7..87ddf1f5d293232d1c7c221da2d41d82b285fc71 100644 --- a/test/src/test/java/hudson/model/UserRestartTest.java +++ b/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 class UserRestartTest { }); } + @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("Pat Q. Hacker"))); + assertThat(text, containsString("pqhacker")); + } + }); + 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 { + final User user; + BadProperty(User user) { + this.user = user; + } + } + }