[JENKINS-45892] Prevent model objects from being serialized except at top level #2957

jglick · 2017-08-01T00:43:45Z

handle Run
handle AbstractItem
handle User

Proposed changelog entries

Jenkins will now prevent core or plugin code from mistakenly attempting to serialize jobs, builds, or users except in their intended top-level XML file positions, preventing a class of serious errors.

@reviewbybees

jglick · 2017-08-01T20:55:13Z

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

+                return this;
+            } else {
+                // Unfortunately we cannot easily tell which XML file is actually being saved here, at least without implementing a custom Converter.
+                LOGGER.log(WARNING, "JENKINS-45892: reference to {0} being saved but not at top level", this);


Note that in the common case that you just have a [Run]Action storing a (nontransient) Run owner, writeReplace will get called only once—at top level—and we proceed normally. XStream then writes a reference="../../.." or similar, and deserialization works. We would rather print a warning in this case, but I could find no way to detect this case: AbstractReferenceMarshaller.convert runs and does not offer us any chance to intercept the process. This patch therefore only changes behavior in other cases:

A build stored via some reference chain in another build, as in the test.

A build stored in some unrelated object.

A build stored in another copy of itself.

The third is the problematic case for lazy-loading. I am not sure of the exact sequence of events (which is why I have no test for it), but somehow a build gets loaded, a badly written action is attached to it, the build is GC’d, a new Run object is reloaded from disk, but we have the original Action somewhere, and Run.save is called on one copy while another copy is referred to internally. Then instead of reference, XStream writes out a new nested <build> element. When deserialized, this explodes because onLoad is not called on the nested Run and so project is left null…as seen in in the toString patch.

ghost · 2017-08-01T21:04:09Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

oleg-nenashev

Minor 🐛 for the replacer logic, but the approach generally looks good to me.

I would suggest moving the logic to the Restricted API classes, because there are use-case in other Jenkins model objects.

I also think a changelog entry is mandatory since the change implies a significant regression risk

oleg-nenashev · 2017-08-02T09:01:48Z

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

+        synchronized (saving) {
+            if (saving.contains(this)) {
+                return this;
+            } else {


Why do you need to keep the else branch synchronized?

Could limit the monitor scope to the contains call, but should not matter—neither the logging nor the Replacer.<init> should block.

oleg-nenashev · 2017-08-02T09:04:20Z

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

+        }
+        private Object readResolve() {
+            // May or may not work:
+            return Jenkins.getInstance().getItemByFullName(fullName);


From what I see this call may happen when Jenkins instance is null and when the jobs are not fully loaded. It would be better to handle this cases explicitly 🐛

It may indeed happen when jobs are not fully loaded, in which case the replacement is null, as seen in the test. It should not happen when Jenkins.instance == null because that would be before startup even, well, starts.

oleg-nenashev · 2017-08-02T09:05:16Z

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

+            saving.add(this);
+        }
+        try {
+            getConfigFile().write(this);


code dup. would be better to move it to private saveInternal()

private? I guess you mean, public but @Restricted(NoExternalUse.class)?

fine as well

Should be final then though

oleg-nenashev · 2017-08-02T09:08:25Z

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

+            saving.add(this);
+        }
+        try {
+            getConfigFile().write(this);


Would it be possible to move the save/replacer logic into a shared Impl class + Interface with default methods? It seems to be a common use-case for Jenkins which could be reused in other model objects. Having it in external class may help, especially when/if somebody starts working on pluggable storage stories in the core

Default methods are not an option since the saving list cannot be kept in an interface. Could be in a utility class somewhere.

Or JustAnotherMixIn contributed via interface

…le.replaceIfNotAtTopLevel.

oleg-nenashev

🐝 resolver logic should work in such way. If not, we will get nice NPEs later at least

jglick · 2017-08-04T14:40:46Z

@reviewbybees done

…ized except at top level.

[JENKINS-45892] Prevent a Run from being serialized except at top level.

c8b5086

jglick added the work-in-progress label Aug 1, 2017

jglick added 2 commits August 1, 2017 16:40

Extending fix to AbstractItem.

29a9ad7

Extending fix to User.

4bbdf81

jglick commented Aug 1, 2017

View reviewed changes

jglick removed the work-in-progress label Aug 1, 2017

oleg-nenashev requested changes Aug 2, 2017

View reviewed changes

jglick added 4 commits August 2, 2017 15:42

Merge branch 'master' into backref-JENKINS-45892

d315519

Strengthening tests a bit.

32be27a

Make readResolve methods tolerate Jenkins.instance == null.

b6758e3

Simplifying writeReplace methods by factoring common logic into XmlFi…

2d21012

…le.replaceIfNotAtTopLevel.

jglick added the needs-more-reviews label Aug 2, 2017

jglick requested a review from oleg-nenashev August 3, 2017 16:12

oleg-nenashev approved these changes Aug 3, 2017

View reviewed changes

jglick merged commit 2d21012 into jenkinsci:master Aug 8, 2017

jglick added a commit that referenced this pull request Aug 8, 2017

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

14028ec

…ized except at top level.

jglick deleted the backref-JENKINS-45892 branch August 8, 2017 12:51

jglick mentioned this pull request Aug 24, 2017

Provide the data about parent builds as environment variable to the dependent build jenkinsci/gerrit-trigger-plugin#319

Merged

jglick mentioned this pull request Aug 31, 2017

[JENKINS-45892] Enhanced diagnostics #2997

Merged

jglick mentioned this pull request Feb 2, 2018

[JENKINS-49328] Prevent inappropriate serialization of MultiJobBuild.SubBuild.build jenkinsci/jenkins-multijob-plugin#129

Merged

jglick mentioned this pull request Jun 21, 2018

Revert "Merge pull request #82" - restores cleanup code that requires core 2.76 jenkinsci/workflow-job-plugin#83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-45892] Prevent model objects from being serialized except at top level #2957

[JENKINS-45892] Prevent model objects from being serialized except at top level #2957

jglick commented Aug 1, 2017 •

edited

Loading

jglick Aug 1, 2017

ghost commented Aug 1, 2017

oleg-nenashev left a comment

oleg-nenashev Aug 2, 2017

jglick Aug 2, 2017

oleg-nenashev Aug 2, 2017

jglick Aug 2, 2017

oleg-nenashev Aug 2, 2017

jglick Aug 2, 2017

oleg-nenashev Aug 2, 2017

oleg-nenashev Aug 2, 2017

oleg-nenashev Aug 2, 2017

jglick Aug 2, 2017

oleg-nenashev Aug 2, 2017

oleg-nenashev left a comment

jglick commented Aug 4, 2017

[JENKINS-45892] Prevent model objects from being serialized except at top level #2957

[JENKINS-45892] Prevent model objects from being serialized except at top level #2957

Conversation

jglick commented Aug 1, 2017 • edited Loading

Proposed changelog entries

Choose a reason for hiding this comment

ghost commented Aug 1, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

jglick commented Aug 4, 2017

jglick commented Aug 1, 2017 •

edited

Loading