[JBINV-9] Guard against ClassMetadataSource.getDeclaredMethods impls …

[bstansberry]

…returning methods from Object.class

https://issues.redhat.com/browse/JBINV-9

[ropalka]

I need to analyse this problem deeper before proceeding but at first glance it LGTM.

[bstansberry]

@ropalka Jaikiran Pai left a good explanation at https://issues.redhat.com/browse/WFCORE-4514. Basically wildfly/wildfly-core@4382f0c resulted in that ClassMetadataSource impl returning not only default interface methods for the class passed in as a param, but also all superclass methods.

A WFCORE-4514 fix would probably involve restricting what's returned to methods whose getDeclaringClass returns the class itself or one of the interfaces the class implements. There might be some weird subtlety where that's wrong though, like a superclass implements the interface with the default method, not the target class. Which maybe doesn't matter; I haven't thought hard.

[ropalka]

Yes, the problem is very well described in WFCORE-4514 by @jaikiran .
I gave it some time to review the code base and I came with an enhanced fix that is
following the same principles as how equals(), hashcode(), toString() & finalize()
are implemented ATM in this library. My proposal commit is here:

ropalka@b995ad8

The idea of the fix is the following. ATM we're successfully filtering - via various criteria
(e.g. final method, non public method, excluded methods list, ...) - all java.lang.Object methods except
its clone() method. I was curious why is it the case and I came to a conclusion
this was an unintentional oversight (is my investigation outcome correct @stuartwdouglas ?)
So I got inspired how finalize() method (which is also protected like clone() method is)
override mechanism is implemented in JBoss invocation library and I came with aforementioned proposal.

WDYT about it @bstansberry ?

[bstansberry]

@ropalka AIUI this would mean that with the standard ProxyFactory any clone method would no longer be proxied. The new overrideClone would only be meaningful to custom AbstractSubclassFactory impls that choose to call it.

I think you're probably right that proxying clone makes no sense. But you'd probably need to move to a new minor version at least for that. For example that kind of change seems risky to put in an EAP CP. Might be worth discussing with other projects that use JB Inv, if there are any.

[ropalka]

Correct @bstansberry Oblect.clone() method and its overrides would no longer be proxied same as Object.finalize() method and its overrides are not. Your proposal PR disabled just Object.clone() to avoid Illegal reflective access warning being issued on JDK11 and above.

We definitely will need to increase the minor to 1.6.0.Final with my fix.
Would you want to put your proposal PR into 1.5 branch so it might be picked up by EAP in next cycle?