Fb jpms - fixes for illegal access to private information in java.* packages.

[struberg]

please review and give feedback. Can push it later myself or you just apply it - txs!

[garydgregory]

Hi @struberg
I have a few comments scattered throughout.

[garydgregory]

Hi @struberg

See my comments and @ppkarwasz's.

[garydgregory]

Hi @struberg

We cannot test that an object implements a stock interface like TemporalAccessor or CharSequence because this won't work with custom implementations (TemporalAmount is also not accounted for, and others I am sure).

See the new test I added in git master: EqualsBuilderReflectJreImplementationTest

I think we can only test for an actual class or the the package that class lives in.

TY

[garydgregory]

Hello @struberg

Checking against an interface in isJavaInternalClass() is not going to work.

I updated EqualsBuilderReflectJreImplementationTest in git master.

[struberg]

Hello @struberg

Checking against an interface in isJavaInternalClass() is not going to work.

I updated EqualsBuilderReflectJreImplementationTest in git master.

Gary, this has already long been resolved in f20639a and ec4150b

I've even merged your changes (and removed that new --add-opens again...) from main to this very branch.

This branch clearly improves the situation with standard jpms setup for a lot of common cases. Of course, this does not yet fix ALL the problems (albeit it's a good start). But if people come across another InaccessibleObjectException then they can still --add-opens as needed. So the current version is clearly an improvement. If you agree, then we should merge it into main instead of yet more parallel work in different branches...

[garydgregory]

Hello @struberg
Checking against an interface in isJavaInternalClass() is not going to work.
I updated EqualsBuilderReflectJreImplementationTest in git master.

Gary, this has already long been resolved in f20639a and ec4150b

I've even merged your changes (and removed that new --add-opens again...) from main to this very branch.

This branch clearly improves the situation with standard jpms setup for a lot of common cases. Of course, this does not yet fix ALL the problems (albeit it's a good start). But if people come across another InaccessibleObjectException then they can still --add-opens as needed. So the current version is clearly an improvement. If you agree, then we should merge it into main instead of yet more parallel work in different branches...

Hello @struberg

The issue is not that this PR is a wants to start improving something, the issue is that this PR breaks existing behavior.

You cannot check for interfaces to short-circuit logic in Reflection.isJavaInternalClass(). This will break existing applications.

The branch is not up to date with master. This fails:

git clone https://github.com/struberg/commons-lang.git 
cd commons-lang
git checkout fb_jpms
git remote add upstream https://github.com/apache/commons-lang.git
git pull upstream master
mvn clean test -Dtest=EqualsBuilderReflectJreImplementationTest

[struberg]

You cannot check for interfaces to short-circuit logic in Reflection.isJavaInternalClass(). This will break existing applications.

Which of those is an interface?
https://github.com/struberg/commons-lang/blob/ec4150bc13cfd2705e5d672f47706930af578c39/src/main/java/org/apache/commons/lang3/builder/Reflection.java#L62

The thing you are talking about IS ALREADY RESOLVED since yesterday.

And one other thing: commons-lang3 ReflectionToString ReflectionEquals etc was BROKEN on anything Java9++ - period!
The build is ONLY green because you just disabled all the modularity - which is something not everyone can do in real projects!

My spare time to work on this stuff is very limited - as is yours I bet. So please use our both time productively. As I said: not everything will fully work from the beginning. You just added java.time.Period, fine, good catch. But why on earth don't we just take that PR and commit it and then improve it TOGETHER? I'm not willing to do more empty rounds getting sticks thrown at me at will. I really appreciate the review feedback as it indeed did highly improve the code. But now is the point to decide whether the direction is right or not - and then improve things from here TOGETHER.

[struberg]

Btw, I'm looking for another method name for isJavaInternalClass(). It should rather express that the given clazz cannot be introspected via reflection. Maybe something like isNonIntrospectibleClass()?

Further down the road we need some way to tell whether a class is open or not. And all that in a way which also works on Java8 still...

[garydgregory]

You cannot check for interfaces to short-circuit logic in Reflection.isJavaInternalClass(). This will break existing applications.

Which of those is an interface? https://github.com/struberg/commons-lang/blob/ec4150bc13cfd2705e5d672f47706930af578c39/src/main/java/org/apache/commons/lang3/builder/Reflection.java#L62

The thing you are talking about IS ALREADY RESOLVED since yesterday.

I guess you could be bothered to look it up? https://docs.oracle.com/javase/8/docs/api/java/time/temporal/Temporal.html

[struberg]

I guess you could be bothered to look it up

Indeed, had in the back of my mind that Temporal is an abstract class.
Latest version uses reflection to figure that a class is not accessible. This works, but maybe we should try a mixture of the old approach + the new one?

Thought about having a static Map<Class, Boolean> KNOWN_INACCESSIBLE_CLASSES which contains the concrete classes like LocalDate, Boolean, Character, Integer, etc.
And then in a second step we do the reflection check. But as this might also have unexpected results (people might forget that they closed the jars) we should probably log it out?

point was valid in the past, but has already been resolved before the review

[garydgregory]

This is not specific to this PR: I don't think we should introduce a new JPMS module dependency, here on java.sql. Lang does not use any classes from java.sql so this would drag in a new JPMS module. For 4.0, we've mentioned dropping our dependency on java.desktop and only depending on java.base.

[struberg]

yup true

[garydgregory]

This looks problematic since the map will grow uncontrollably, possibly until every class on your classpath gets referenced, there's no way to know really.

[struberg]

this is a trade-off vs performance. it will only cache those classes which got hit in a reflection equals, toString, etc. Thus in practice it is a very limited set of classes I'd say. Can you think of a possible place where we can hold such a cache at least during a single invocation? Maybe inside the builder and handing the Map as another parameter to this method?

[garydgregory]

"internas" spelling

[garydgregory]

Hi All,

So far, I'm still against this change.

These features are designed to use reflection, and this PR tries to turn some of that off.

It seems to me like it will cause unintended consequences beyond all of the ones I pointed out through the new unit tests I wrote which showed the initial implementation would break existing apps.

Now, we have a different implementation, which brings in a new set of problems:

• It depends on a new JPMS module java.sql
• it grows a map without any bounds (a CVE waiting to happen IMO)
• you can't turn it off
• The behavior is completely hidden from the user

[ppkarwasz]

So far, I'm still against this change.

These features are designed to use reflection, and this PR tries to turn some of that off.

I agree. All the reflectionX methods have a warning:

Fields can be private, thus AccessibleObject.setAccessible is used to bypass normal access control checks. This will fail under a security manager unless the appropriate permissions are set.

The warning might require an update, but it seems clear to me that it also applies to the JPMS access checks. So it is up to caller to ensure that the class is open to org.apache.commons.lang3. If the caller uses testRecursive, he must also ensure that the appropriate permissions are set for all the field types.

[struberg]

I agree. All the reflectionX methods have a warning:

Fields can be private, thus AccessibleObject.setAccessible is used to bypass normal access control checks.

why is this disussion again NOT happening on any ASF list but here where it does not get reflected to any ASF list afaict?

[garydgregory]

This discussion is mirrored on the apache list here:
https://lists.apache.org/thread/gwljc3ylz3cjvbg9m7sv9dtj139obprz

[garydgregory]

Note that this PR is worse than I thought because we already have a feature to bypass reflection! Use EqualsBuilder.setBypassReflectionClasses(List<Class<?>>).

[struberg]

further comments over at the commons dev list.

[struberg]

Note that this PR is worse than I thought because we already have a feature to bypass reflection! Use EqualsBuilder.setBypassReflectionClasses(List<Class<?>>).

Are you sure that you really understand that code?

If you would setBypassReflectionClasses(ArrayList.class) or HashSet.class or HashMap.class etc, then you would totally break the EqualsBuilder. Because this would also stop looking down into the equality of the elements contained in that List/Set/Map.

Funnily you yourself did add hand-tinkered bypasses for those classes in selected places - just not for every use case. So you yourself added code to NOT use reflection for some of the cases.

So this rather sounds like a strawman argument to me.

[struberg]

I agree. All the reflectionX methods have a warning:

Well this is way more subtly weird right now.

Imagine the following code:

        Map<String, String> a = new HashMap<>(20);
        a.put("A", "A");

        Map<String, String> b = new HashMap<>();
        b.put("A", "A");

Here we have the case that a.equals(b) returns true. Because that's the contract of Maps!
But when you use new EqualsBuilder().setTestRecursive(true).append(a, b).isEquals() then you'll get false.
That's because by giving a different initial array size, we'll get a different internal (!) structure (bigger internal arrays, etc).

But that means that right now the EqualsBuilder (even in Java8) does not even adheres to the contract of the Map interface. But those internal details should not matter! Even if I would not have set an initial size - what if I add 30 elements to one of the 2 maps, but then remove everything but the "A" entry again? It will be equals according to the Map contract, but not for the EqualsBuilder. Looks rather broken to me.

One might argue that that's exactly how all the reflection stuff in commons-lang was designed from the beginning.
Otoh there are already a few places where Maps, Collections, etc get a special treatment. So why not also here?

[struberg]

Note that this PR is worse than I thought because we already have a feature to bypass reflection! Use EqualsBuilder.setBypassReflectionClasses(List<Class<?>>).

This is just plain wrong. You cannot use setBypassReflectionClasses for e.g. Map without loosing the reflexive comparison of it's entries.

Because the reason given is just plain wrong. See my last 2 comments. Either we adhere the Map, Collection, etc equals contracts (as indicated in the JavaDoc of the ReflectionEquals and others commons lang classes) then we need this handling. Or not, then we need to to remove all the references to Effective Java etc. But right now we have rather broken behaviour