jenkins icon indicating copy to clipboard operation
jenkins copied to clipboard

Published 20 hours ago • verified publisher icon jenkinsci

[JENKINS-73687] Make deserialization of `Map` fields in XML files more robust

Open dwnusbaum opened this issue 6 months ago • 4 comments

See JENKINS-73687.

I was recently made aware of a change in hashicorp-vault plugin that accidentally broke serial compatibility of a folder property in a way that unexpectedly caused the folder to become very corrupted after deserialization. See https://github.com/jenkinsci/hashicorp-vault-plugin/issues/326#issuecomment-2307757372 for details, but the summary is that config.xml for the folder had an element for a map which looked like this associated with a field of type Map<String, Calendar>, which is not correct.

<tokenExpiry>
  <time>1724357123299</time>
  <timezone>America/New_York</timezone>
</tokenExpiry>

The correct format would have been something like this:

<tokenExpiry>
  <entry>
    <string>key</string>
    <gregorian-calendar>
      <time>1724357123299</time>
      <timezone>America/New_York</timezone>
    </gregorian-calendar>
  </entry>
</tokenExpiry>

This led to a bare RuntimeException being thrown here as a result of this call, which expects that it has just read the <entry> element and needs to move down to read the key, but in this case has in fact just read the element named <time>, and so there are no children to read:

https://github.com/jenkinsci/jenkins/blob/fb09eafb02ead56b93620b9f47cbb22a43b8eb70/core/src/main/java/hudson/util/RobustMapConverter.java#L67

Since this is a RuntimeException, it actually escapes quite far up through the converter hierarchy until it is caught here and then the wrapped exception is caught in RobustReflectionConverter.doUnmarshal, but at that point things go pretty badly wrong and the content that XStream is reading is associated with the wrong field in the class, leading to further problems and eventually a corrupted object. In the original issue, I saw this warning in the logs: Cannot convert type hudson.util.DescribableList to type hudson.model.ViewGroup, indicating pretty bad type confusion.

Here is the full stack trace of the OldDataMontior warning: 
java.lang.RuntimeException
     at com.thoughtworks.xstream.io.xml.AbstractPullReader.moveDown(AbstractPullReader.java:105)
     at com.thoughtworks.xstream.io.ReaderWrapper.moveDown(ReaderWrapper.java:36)
     at com.thoughtworks.xstream.io.path.PathTrackingReader.moveDown(PathTrackingReader.java:37)
     at hudson.util.RobustMapConverter.read(RobustMapConverter.java:67)
     at hudson.util.RobustMapConverter.putCurrentEntryIntoMap(RobustMapConverter.java:49)
     at com.thoughtworks.xstream.converters.collections.MapConverter.populateMap(MapConverter.java:99)
     at com.thoughtworks.xstream.converters.collections.MapConverter.populateMap(MapConverter.java:93)
     at com.thoughtworks.xstream.converters.collections.MapConverter.unmarshal(MapConverter.java:88)
     at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:74)
 Caused: com.thoughtworks.xstream.converters.ConversionException: 
 ---- Debugging information ----
 cause-exception     : java.lang.RuntimeException
 cause-message       : null
 class               : java.util.HashMap
 required-type       : java.util.HashMap
 converter-type      : hudson.util.RobustMapConverter
 path                : /com.cloudbees.hudson.plugins.folder.Folder/properties/com.cloudbees.hudson.plugins.folder.FolderTest$Property/fieldB/time
 line number         : 6
 -------------------------------
     at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:81)
     at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:72)
     at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:68)
     at hudson.util.RobustReflectionConverter.unmarshalField(RobustReflectionConverter.java:454)
     at hudson.util.RobustReflectionConverter.doUnmarshal(RobustReflectionConverter.java:350)
     at hudson.util.RobustReflectionConverter.unmarshal(RobustReflectionConverter.java:289)
     at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:74)
     at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:72)
     at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:68)
     at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:52)
     at com.thoughtworks.xstream.converters.collections.AbstractCollectionConverter.readBareItem(AbstractCollectionConverter.java:132)
     at com.thoughtworks.xstream.converters.collections.AbstractCollectionConverter.readItem(AbstractCollectionConverter.java:117)
     at hudson.util.CopyOnWriteList$ConverterImpl.unmarshal(CopyOnWriteList.java:203)
     at hudson.util.DescribableList$ConverterImpl.unmarshal(DescribableList.java:285)
     at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:74)
     at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:72)
     at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:68)
     at hudson.util.RobustReflectionConverter.unmarshalField(RobustReflectionConverter.java:454)
     at hudson.util.RobustReflectionConverter.doUnmarshal(RobustReflectionConverter.java:350)
     at hudson.util.RobustReflectionConverter.unmarshal(RobustReflectionConverter.java:289)
     at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:74)
     at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:72)
     at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:68)
     at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:52)
     at com.thoughtworks.xstream.core.TreeUnmarshaller.start(TreeUnmarshaller.java:136)
     at com.thoughtworks.xstream.core.AbstractTreeMarshallingStrategy.unmarshal(AbstractTreeMarshallingStrategy.java:32)
     at com.thoughtworks.xstream.XStream.unmarshal(XStream.java:1464)
     at hudson.util.XStream2.unmarshal(XStream2.java:230)
     at hudson.util.XStream2.unmarshal(XStream2.java:201)
     at com.thoughtworks.xstream.XStream.unmarshal(XStream.java:1441)
     at com.thoughtworks.xstream.XStream.fromXML(XStream.java:1330)
     at hudson.XmlFile.read(XmlFile.java:165)
     at hudson.model.Items.load(Items.java:375)
     at hudson.model.ItemGroupMixIn$2.call(ItemGroupMixIn.java:284)
     at hudson.model.ItemGroupMixIn$2.call(ItemGroupMixIn.java:282)
     at hudson.model.Items.whileUpdatingByXml(Items.java:132)
     at hudson.model.ItemGroupMixIn.createProjectFromXML(ItemGroupMixIn.java:282)
     at jenkins.model.Jenkins.createProjectFromXML(Jenkins.java:4291)
     at com.cloudbees.hudson.plugins.folder.FolderTest.badProperty(FolderTest.java:610)
     at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
     at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
     at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
     at java.base/java.lang.reflect.Method.invoke(Method.java:568)
     at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
     at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
     at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
     at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
     at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:655)
     at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299)
     at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293)
     at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
     at java.base/java.lang.Thread.run(Thread.java:833)

Here are some things that I still want to check:

  • [x] Are there other cases where XStream throws RuntimeException where we'd like for one of our Robust*Converter classes to instead just ignore the offending element?
    • See https://github.com/jenkinsci/jenkins/pull/9653#issuecomment-2310550942. I did not see any other obvious things that we can improve here.
  • [x] Are there cases where the proposed fix leads to worse problems than without the fix?
    • Not that I can think of.
  • [x] Can we improve the error message in this case to list the path to the problematic element?

Testing done

Proposed changelog entries

  • Make deserialization of Map fields in XML files more robust

Proposed upgrade guidelines

N/A

### Submitter checklist
- [x] The Jira issue, if it exists, is well-described.
- [x] The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see [examples](https://github.com/jenkins-infra/jenkins.io/blob/master/content/_data/changelogs/weekly.yml)). Fill in the **Proposed upgrade guidelines** section only if there are breaking changes or changes that may require extra steps from users during upgrade.
- [x] There is automated testing or an explanation as to why this change has no tests.
- [ ] New public classes, fields, and methods are annotated with `@Restricted` or have `@since TODO` Javadocs, as appropriate.
- [ ] New deprecations are annotated with `@Deprecated(since = "TODO")` or `@Deprecated(forRemoval = true, since = "TODO")`, if applicable.
- [ ] New or substantially changed JavaScript is not defined inline and does not call `eval` to ease future introduction of Content Security Policy (CSP) directives (see [documentation](https://www.jenkins.io/doc/developer/security/csp/)).
- [ ] For dependency updates, there are links to external changelogs and, if possible, full differentials.
- [ ] For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

Before the changes are marked as ready-for-merge:

### Maintainer checklist
- [ ] There are at least two (2) approvals for the pull request and no outstanding requests for change.
- [ ] Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
- [ ] Changelog entries in the pull request title and/or **Proposed changelog entries** are accurate, human-readable, and in the imperative mood.
- [ ] Proper changelog labels are set so that the changelog can be generated automatically.
- [ ] If the change needs additional upgrade steps from users, the `upgrade-guide-needed` label is set and there is a **Proposed upgrade guidelines** section in the pull request title (see [example](https://github.com/jenkinsci/jenkins/pull/4387)).
- [ ] If it would make sense to backport the change to LTS, a Jira issue must exist, be a _Bug_ or _Improvement_, and be labeled as `lts-candidate` to be considered (see [query](https://issues.jenkins.io/issues/?filter=12146)).

dwnusbaum avatar Aug 23 '24 23:08 dwnusbaum