Prevent 'illegal reflective access' problems when instantiating XStream

[famod]

Relates to #101 and quarkusio/quarkus#10303 (comment)

I realized that just by instantiating XStream a whole bunch of reflection access is performed by various converters (triggered via Xstream.setupConverters()).
This is regardless of whether you will actually need the converter or not, just the setup triggers this.

This PRs defers all the static reflection access by moving the respective fields and lookups to static inner classes which per JLS are loaded on demand in a thread safe way.
I also addded a test that analyzes the classes (via https://github.com/classgraph/). First I wanted to use ArchUnit but that requires at least JUnit 4 but Core still uses 3.8.1.

The most debatable change is the one to AbstractAttributedCharacterIteratorAttributeConverter.
I had to drop readResolve method and the attributeMap member. The map was transient anyway and readResolve puzzled me a bit, because the converter does not seem to implement Serializable at all.

With this PR I am not getting any illegal reflective access warnings anymore when instantiating XStream on AdoptOpenJDK 11.0.6.

[melloware]

Fantastic

[coveralls]

Coverage remained the same at 81.236% when pulling eb2dcfd on famod:defer-reflective-access into 470f1cb on x-stream:master.

[joehni]

Good approach. While it does not solve the real issue, it is the base for a dynamic solution.

readResolve: XStream calls these methods also for non-serializable types, because it serializes those types nevertheless. That change might have to be adjusted.

[famod]

While it does not solve the real issue, it is the base for a dynamic solution.

Yeah, it is "only" a mitigation for cases where you don't need those converters.

That change might have to be adjusted.

Do you have a suggestion? How can we check whether my change broke anything? I mean, tests are passing...
I could try to restore readResolve in a certain form if it is not required that the method is called from the constructor.

[joehni]

There are two aspects. 7 years ago there was no alternative than to use reflection to restore the constant after deserialization. Now, with Java 8 as baseline this might have changed and an implementation is possible without the magic stuff. The other aspect is the compatibility of the XML, which must stay compatible.

The readResolve here is for a different purpose. XStream's converters can be marshalled themselves and should work after unmarshalling. The attributeMap is transient, because it should not be part of the persistent data of the converter, but must be restores at unmarshalling time.

[famod]

7 years ago there was no alternative than to use reflection to restore the constant after deserialization. Now, with Java 8 as baseline this might have changed and an implementation is possible without the magic stuff.

I did not check every converter I touched, but the two I had a look at do still need that magic, unfortunately.

The other aspect is the compatibility of the XML, which must stay compatible.

AFAICS, I have not changed anything in that regard.

XStream's converters can be marshalled themselves and should work after unmarshalling.

This should still work, even without the readResolve method I removed from AbstractAttributedCharacterIteratorAttributeConverter .

The attributeMap is transient, because it should not be part of the persistent data of the converter, but must be restores at unmarshalling time.

Why must it be restored?
I removed it entirely (as a member) and now each attributeMap is looked up from static instanceMaps when the converter is actually used (not just instantiated).
When the respective attributeMap is not yet available, it is built once with the same logic as before and is added to instanceMaps (which I changed to ConcurrentHashMap since multiple threads could access the map at once, even before my change).

So, if I am not missing something, all should be well?

[famod]

I've just pushed a minor cleanup in the test class.

[joehni]

I'll explain my changes tomorrow ... just tired now.

[famod]

Alright, take your time! I'll have a look tomorrow.

Is there any chance to get this into another small 1.4 release?
Many users are affected by this. I could try to support by creating a PR against the 1.4 branch.

[joehni]

First, you were right with your comment about the AbstractAttributedCharacterIteratorAttributeConverter.

I've omitted the unit test for two reasons. First of all it is a static analysis of the current classes. All it does is to prevent the usage of such static stuff in upcoming implementations. Second reason is, that I'd like to implement the access to the critical stuff in a dynamic way, i.e. XStream should only use it, if the module containing the type is open for reflective access. In the long run I'd like to use the acceptance tests to be run on a module path and once in the unnamed module. The former case will prevent the current implementation from working, the latter case will work,but might create the warnings. These tests will catch any problem of the omitted unit test also.

Regarding 1.4: I did not had the intension to create another 1.4.x release although it would be possible with this interesting change. However, keep in mind that 1.4.x still targets Java 1.4 (except the enum stuff, which targets Java 1.5). So, no annotations, no diamond operators, no ConcurrentHashMap or multi-catch...

[melloware]

i think a 1.5 release would be great as I can't imagine that many people are still using Java 1.4 in 2020 but who knows!

[joehni]

@melloware : Possibly not JDK 1.4, but I don't want to change that for a final release. However, some people will hesitate to switch to 1.5, because then they will have to deal with the white list. There are still a lot of users running 1.3.x despite the security issues.

[famod]

@joehni Thanks a lot for this detailed explanation, I really appreciate it!

Dropping the test makes perfect sense under the described circumstances.
I learned something so it wasn't for nothing.

Regarding 1.4 vs. 1.5, for me it depends on which one is most likely to be released earlier than the other one.
How likely is another 1.4 release within the next 3 weeks in case I come with a PR for the 1.4 branch? Quarkus will have its next release (1.8) at the of August or beginning of September.

[joehni]

I'll be offline for the last two weeks in August. However, first week of September is manageable.

[famod]

Ok, I'll try to come up with a 1.4 PR this month then.