WARNING: An illegal reflective access operation has occurred

[Ian Carroll]

While using Java 9, I am getting this error:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.yaml.snakeyaml.introspector.PropertyUtils (file:/Users/me/.m2/repository/org/yaml/snakeyaml/1.19/snakeyaml-1.19.jar) to method java.beans.FeatureDescriptor.isTransient()
WARNING: Please consider reporting this to the maintainers of org.yaml.snakeyaml.introspector.PropertyUtils
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

I'm not seeing anyone else reporting it, so I'll go ahead and be the first. (Maybe user error, if so, thanks for helping out, otherwise, hope this helps :) )

[Uwe Schindler]

This prevents usage of Snakeyaml in Apache Solr's Metrics reporter, as Apache Solr enforces strictness with --illegal-access=deny: https://issues.apache.org/jira/browse/SOLR-11795?focusedCommentId=16374338&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16374338

[Andrey Somov]

Dear Uwe,

1. Do you have a proposal ?
2. Do you suggest to migrate SnakeYAML to Java 9 ?
3. Will you contribute a patch ?

[Uwe Schindler]

Hi I think the issue can be solved by making the error handling in the reflective code correct. Currenbtly it breaks completely with Java 9 if you configure the JVM to prevent access to the class library (setAccessible).

In addition the code needs to use AccessController.doPrivileged() when doing relection like this, otherwise the library

There is no need to move to Java 9, the problem is the code and how it does reflection:

The Exception handling at this place (and possible other places) is broken in multiple ways: https://bitbucket.org/asomov/snakeyaml/src/9ffe9d8b2ba4e5eabde045d407799a71997cda20/src/main/java/org/yaml/snakeyaml/introspector/PropertyUtils.java?at=default&fileviewer=file-view-default#PropertyUtils.java-124:151

• Uses e.printStackTrace() -> this is not acceptable behaviour of a library. Log those calls correctly
• It catches only those exceptions, that eclipse autogenerated (and it does not even use multi-catch). It should better catch RuntimeException and ReflectiveOperationException and bail out if anything goes wrong. SecurityException is then automatically handled.
• it does not use AccessController.doPrivileged(), so it can never be made safe with security manager! The code doing deep reflection needs to use AccessController, so code using snakeyaml is able to restrict deep reflection per JAR file.

I can provide a patch, but as I don't use HG, I'd like to fork the github repository instead, but it's not uptodate.

[Uwe Schindler]

And on top: The default behaviour of snakeyaml should be to never ever try to do setAccessible() on any JDK classes, so it should not try to do stuff like this. Really! People can do this in their own code, but a library you import should never-ever do this by default!

I have no idea why it needs to do this here at all? Why does it need access to this strange isTransient()? It should be optional. For code that really needs to use this, there should be an opt-in, correctly guarded with AccessController. But it should not try to do this by default without control of the user.

If people opt-in, they should be aware that this no longer works with Java 9 or later (it prints warning by default, but won't work if strict mode is enabled or later Java versions completely denying those hacks).

[Andrey Somov]

After having issues with HG<->GIT we have decided to use only one version control system.

To send a patch you do not need GIT. Just implement your idea and send us the 'hg diff -g > my-idea.patch'

Looking forward to make SnakeYAML better !

[Uwe Schindler]

OK, I will do a patch, but no offical pull request. My HG knowledge equals zero.

I just have the question: What is this isTransient() about? I can just work around the bad reflection done here. From looking at the code quality around this "bad reflective code" it looks like it was submitted by someone else, because it looks quite different than snakeyaml's code (your's). I have no idea how to do "git blame" on this code base, to find the offender: Code here like e.printStackTrace() in a catch clause looks like done by someone who had no idea about Exception handling and he just applied all suggested fixes that Eclipse provided to him, just to make the code running.

[Andrey Somov]

I do not think that it is important to blame anybody. If we just want to have the issue fixed. You can assume that the whole code is written by the whole team (I am not the only contributor !!!)

We have a comprehensive test suite. Feel free to make your change and see which tests complain.

[Alexander Maslov]

Hi @thetaph1 if it makes your day you can blame me. It was me who committed that code.

answering your question: isTransient is the new method added in java 1.7 because of the addition of @Transient annotation. Unfortunately it has been added with package visibility. That is a workaround to be able to support it and still be on 1.6 source level.

It is possible to change it to be as simple as

private static final String TRANSIENT = "transient";

private boolean isTransient(FeatureDescriptor fd) {
    return Boolean.TRUE.equals(fd.getValue(TRANSIENT));
}

[Alexander Maslov]

changeset b118e0b removes reflective access to FeatureDescriptor.isTransient

[Uwe Schindler]

Changeset looks fine, although I have no idea how the extra test should pass on Java 8. InaccessibleObjectException is a new Exception type only available in Java 9, so how can that work with Java 7/8? See https://docs.oracle.com/javase/9/docs/api/java/lang/reflect/InaccessibleObjectException.html

In Groovy, I know they just catch RuntimeException and then just do a getClass().getName() on the exception to compare its name, so yoiu can detect the inaccessible object case. The same in Lucene, although we don't care about the exact Exception type. Here is Groovy's example:

https://github.com/apache/groovy/blob/5011bf02e8769fc35ff30533267deb6b5cb2e542/src/main/java/org/codehaus/groovy/reflection/CachedConstructor.java#L52

To make the test also run with Java 8, I'd do it in a similar way.

Uwe

[Alexander Maslov]

it is not going to pass in 7 anyway because Optional is since 1.8. But you are correct - it is not going to even compile with 1.8 because of the reason you mentioned (forgot about it while making those changes)

I didn't want to add any new folders for java9 tests (but maybe I will)

[Uwe Schindler]

@maslovalex: I'd do the check much simpler: Catch RuntimeException and compare the class name. Very easy and enough. If you will start with separate Java 9 features, I'd add a new folder (maybe also build a Java 9 MultiRelease-JAR, so the Java 9 stuff is only used when applicable).

[Uwe Schindler]

Hi, I can confirm: The Solr Prometheus Exporter passes its test suite with a JAR file "1.20-SNAPSHOT" built from source (built with Java 6):

-test:
   [junit4] <JUnit4> says olß! Master seed: A26CB9B3FB4D1BCD
   [junit4] Your default console's encoding may not display certain unicode glyphs: windows-1252
   [junit4] Executing 5 suites with 3 JVMs.
   [junit4]
   [junit4] Started J0 PID(1736@VEGA).
   [junit4] Started J2 PID(11720@VEGA).
   [junit4] Started J1 PID(15296@VEGA).
   [junit4] Suite: org.apache.solr.prometheus.collector.config.SolrCollectorConfigTest
   [junit4] Completed [1/5] on J0 in 12.31s, 7 tests
   [junit4]
   [junit4] Suite: org.apache.solr.prometheus.scraper.config.SolrScraperConfigTest
   [junit4] Completed [2/5] on J0 in 0.32s, 5 tests
   [junit4]
   [junit4] Suite: org.apache.solr.prometheus.scraper.config.SolrQueryConfigTest
   [junit4] Completed [3/5] on J0 in 0.12s, 8 tests
   [junit4]
   [junit4] Suite: org.apache.solr.prometheus.collector.SolrCollectorTest
   [junit4] Completed [4/5] on J2 in 47.24s, 2 tests
   [junit4]
   [junit4] Suite: org.apache.solr.prometheus.exporter.SolrExporterTest
   [junit4] Completed [5/5] on J1 in 49.03s, 1 test
   [junit4]
   [junit4] JVM J0:     2.03 ..    16.85 =    14.82s
   [junit4] JVM J1:     2.03 ..    52.53 =    50.50s
   [junit4] JVM J2:     2.03 ..    51.00 =    48.97s
   [junit4] Execution time total: 53 seconds
   [junit4] Tests summary: 5 suites, 23 tests

So if you could make a relaese soon, we may be able to also get the Prometheus exporter into Apache Solr 7.3!

As a final comment: I grepped through the code and have seen still some setAccessible(true) at some places. Do we really need them? For deserializing objects, it should really only use public members! At least it might be better to do this as an opt-in feature. By default setting everything to accessible might be a security issue for some projects!

Thanks for help, Uwe

[Alexander Maslov]

Yes, there are still places with setAccessible(true) and I believe we will need to take give more "love" to those in future releases. And I agree we need to make it as a configurable option, but to cover most of the real life scenarios it is going to be on by default.

I am closing this one (we are actually planning to release quite soon)

[Alexander Maslov]

will be delivered as part of 1.20 release

[Andrey Somov]

This is delivered in release 1.20