[JENKINS-49054] TreeUnmarshaller.convert could leave behind a corrupted type stack

[jglick]

jenkinsci#2 just applied to the upstream trunk with trivial stylistic cleanup.

jenkinsci/jenkins#3248 is an integration test for this in Jenkins but if you need a unit test here let me know and I will try to come up with one.

Also see #70, which proposes backporting of various other fixes (not this one).

@reviewbybees

[ghost]

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.

[coveralls]

Coverage decreased (-0.002%) to 77.908% when pulling 7ef77cf on jglick:TreeUnmarshaller into d29c62c on x-stream:master.

[joehni]

See #91

[jglick]

From that:

The raised exception has to quit the complete unmarshalling process

This is completely incompatible with how Jenkins needs to use XStream; it cannot block loading of an entire config.xml (etc.) just because there was a plugin error in a single element of some list. So if this will not be reconsidered, we will need to continue using a fork with jenkinsci#2, as would any modular application.

[joehni]

And how do you deal with the underlaying XML parser? XStream cannot make any assumptions about its state and will never be able to guarantee anything to a consumer.

[jglick]

I am not sure what the XML parser has to do with this.

To be clear, Jenkins already handles all sorts of nonfatal errors in XStream input (missing classes, exceptions from readResolve, field types changed from one plugin release to the next, etc. etc.), and has done so for years. With a few security-related exceptions, a load error in any object field or collection element is considered recoverable. It even has a UI for displaying XStream errors encountered while loading various configuration files and letting the user choose to discard the erroneous portions by remarshalling from memory.

Recently we found this corner case where one custom unmarshaller happened to care about the type in the context stack and we noticed that TreeUnmarshaller had an obvious bug in that it has mutable state which is supposed to be reset upon exit from a method but the finally block was forgotten, and so under those conditions the handling gets broken. The case must be pretty rare, because I had never encountered it until a few days ago, could not find any mention of the stack trace in JIRA, and @mbakhoff only filed their proposed fix a few months ago.

[joehni]

Sorry to say, but you make wrong assumptions. If it works for you, it is pure coincidence.

XStream is based on a stream model. When it starts unmarshalling, it requests the first opening tag from the underlaying XML parser (not knowing how many bytes this parser really reads physically from the byte stream). Depending on the tag XStream guesses the type and selects a converter that is supposed to handle the content part of the open tag. This content might be either a value or more children, but the converter is in charge expecting the correct content. If it expects children it will again read the opening tag, guess the type and starts a recursion by letting XStream selecting the proper converter again. However, the converter is always supposed to handle also the closing tag. The calls to the moveDown and moveUp methods of the HierarchicalStreamReader have to be balanced.

Now, if somewhere deep down the stack an exception is raised and you catch it somewhere along the stack in an own converter and continue with XML processing, you silently assume that all the converters on the stack processed the remaining part of "their" content in the data stream (including the closing tag) while unwinding the stack. This assumption is simply wrong. None of the converters will do this. The stream model and XML parser is after an exception actually in an undefined state. The only safe thing to assume is, that reading the XML stream has failed.

[jglick]

The only safe thing to assume is, that reading the XML stream has failed.

That is not a realistic option for a modular application (and for compatibility reasons moving away from XStream is out of the question at this point), so I have filed JENKINS-49169 to track any follow-up fixes we may need to make in our fork as well as other classes. I would prefer to minimize the differences to the vendor branch but that is now looking impractical (CC @ndeloof).

[joehni]

The interesting thing about moveUp is that it consumes anything in the stream unless it finds a closing tag. However, it is not very realistic that all converters will take care for balanced calls to moveDown and moveUp. But all that is missing in the exception handling is the number of calls that have to be made to moveUp. Therefore I've added for version 1.5 to HierarchicalStreamReader the method getLevel() that delivers the current nesting level of the reader.

Since this change is not compatible to 1.4.x, you may currently use a workaround to track the nesting level. d0ad410 contains an acceptance test that demonstrates how to implement the exception handling. In the test it is done while processing an ObjectInputStream, but you can do the same in a custom converter.

Nevertheless, you cannot recover from every exception. If the XML parser chokes because of XML syntax errors or because of I/O problems, you should still abort the processing. XStream will typically throw an c.t.x.io.StreamException in such a case.