From 3e97b19d1beeb87281232e94a8a9b00254b76034 Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Sun, 25 Oct 2020 11:38:46 -0400 Subject: [PATCH 1/2] Follow-up to PR#104. Small updates to OgnlRuntime, attempting to ensure more consistent behaviour for JDK7-11. - Update OgnlRuntime isMethodCallable(). Removed (now) unnecessary isJDK15 check, added clearer logic and explanatory JavaDoc comments. - Added OgnlRuntime isMethodCallable_BridgeOrNonSynthetic() for specialized usage such as choosing valid Read and Write methods. - Made checks in getReadMethod() and getWiteMethod symmetric again. - Added new unit tests to confirm expected behaviour with respect to bridge methods and synthetic methods. --- src/main/java/ognl/OgnlRuntime.java | 58 +++++++++++-- src/test/java/ognl/TestOgnlRuntime.java | 106 +++++++++++++++++++++++- 2 files changed, 154 insertions(+), 10 deletions(-) diff --git a/src/main/java/ognl/OgnlRuntime.java b/src/main/java/ognl/OgnlRuntime.java index 116c5383..4affab09 100644 --- a/src/main/java/ognl/OgnlRuntime.java +++ b/src/main/java/ognl/OgnlRuntime.java @@ -2738,18 +2738,46 @@ private static void addIfAccessor(List result, Method method, String baseName, b } /** - * Convenience used to check if a method is volatile or synthetic so as to avoid - * calling un-callable methods. + * Convenience used to check if a method is a synthetic method so as to avoid + * calling un-callable methods. These methods are not considered callable by + * OGNL in almost all circumstances. + * + * This method considers any synthetic method (even bridge methods) as being un-callable. + * Even though synthetic and bridge methods can technically be called, by default + * OGNL excludes them from consideration. + * + * Synthetic methods should be excluded in general, since calling such methods + * could introduce unanticipated risks. * * @param m The method to check. - * @return True if the method should be callable, false otherwise. + * @return True if the method should be callable (non-synthetic), false otherwise. */ static boolean isMethodCallable(Method m) { - if ((isJdk15() && m.isSynthetic()) || Modifier.isVolatile(m.getModifiers())) - return false; + return !(m.isSynthetic() || m.isBridge()); + } - return true; + /** + * Convenience used to check if a method is either a non-synthetic method or + * a bridge method. + * + * Warning: This method should NOT be used as a direct replacement for + * {@link #isMethodCallable(Method)}. Almost all OGNL processing assumes the + * exlcusion of synthetic methods in order to process correctly. Only + * use this method to determine method callability for any OGNL processing + * after careful consideration. + * + * This method considers synthetic methods that are not also bridge methods + * as being un-callable. + * + * Synthetic methods should be excluded in general, since calling such methods + * could introduce unanticipated risks. + * + * @param m The method to check. + * @return True if the method should be callable (non-synethetic or bridge), false otherwise. + */ + static boolean isMethodCallable_BridgeOrNonSynthetic(Method m) { + return !m.isSynthetic() || m.isBridge(); // Reference: See PR#104. } /** @@ -3597,6 +3625,11 @@ public static Method getReadMethod(Class target, String name, Class[] argClasses for (int i = 0; i < methods.length; i++) { + // Consider bridge methods as callable (also) for Read methods. + if (!isMethodCallable_BridgeOrNonSynthetic(methods[i])) { + continue; + } + if ((methods[i].getName().equalsIgnoreCase(name) || methods[i].getName().toLowerCase().equals("get" + name) || methods[i].getName().toLowerCase().equals("has" + name) @@ -3614,6 +3647,11 @@ public static Method getReadMethod(Class target, String name, Class[] argClasses for (int i = 0; i < methods.length; i++) { + // Consider bridge methods as callable (also) for Read methods. + if (!isMethodCallable_BridgeOrNonSynthetic(methods[i])) { + continue; + } + if (methods[i].getName().equalsIgnoreCase(name) && !methods[i].getName().startsWith("set") && !methods[i].getName().startsWith("get") @@ -3676,8 +3714,10 @@ public static Method getWriteMethod(Class target, String name, Class[] argClasse ArrayList candidates = new ArrayList(); for (int i = 0; i < methods.length; i++) { - if (!isMethodCallable(methods[i].getMethod())) + // Consider bridge methods as callable (also) for Write methods. + if (!isMethodCallable_BridgeOrNonSynthetic(methods[i].getMethod())) { continue; + } if ((methods[i].getName().equalsIgnoreCase(name) || methods[i].getName().toLowerCase().equals(name.toLowerCase()) @@ -3697,8 +3737,10 @@ public static Method getWriteMethod(Class target, String name, Class[] argClasse Method[] cmethods = target.getClass().getMethods(); for (int i = 0; i < cmethods.length; i++) { - if (!isMethodCallable(cmethods[i])) + // Consider bridge methods as callable (also) for Write methods. + if (!isMethodCallable_BridgeOrNonSynthetic(cmethods[i])) { continue; + } if ((cmethods[i].getName().equalsIgnoreCase(name) || cmethods[i].getName().toLowerCase().equals(name.toLowerCase()) diff --git a/src/test/java/ognl/TestOgnlRuntime.java b/src/test/java/ognl/TestOgnlRuntime.java index 4898c3ee..a46be5e9 100644 --- a/src/test/java/ognl/TestOgnlRuntime.java +++ b/src/test/java/ognl/TestOgnlRuntime.java @@ -699,6 +699,9 @@ public String testMethod (String element, int i) { } } + /** + * Protected class for synthetic/bridge method tests. + */ protected static class ProtectedParent { public void setName(String name) { } @@ -707,15 +710,114 @@ public String getName() { } } + /** + * Public descendant class for synthetic/bridge method tests. + */ public static class PublicChild extends ProtectedParent { } - public void testSyntheticReadMethod() throws Exception { + /** + * Test that synthetic bridge read methods can be found successfully. + * + * Note: Only bridge methods should qualify, non-bridge synthetic methods should not. + * + * @throws Exception + */ + public void testSyntheticBridgeReadMethod() throws Exception { assertNotNull(OgnlRuntime.getReadMethod(PublicChild.class, "name")); } - public void testSyntheticWriteMethod() throws Exception { + /** + * Test that synthetic bridge write methods can be found successfully. + * + * Note: Only bridge methods should qualify, non-bridge synthetic methods should not. + * + * @throws Exception + */ + public void testSyntheticBridgeWriteMethod() throws Exception { assertNotNull(OgnlRuntime.getWriteMethod(PublicChild.class, "name")); } + /** + * Public class for "is callable" method tests. + */ + public static class SimplePublicClass { + String name = "name contents"; + + public String getName() { + return name; + } + } + + /** + * Public class with non-public nested class for "is callable" method tests. + */ + public static class SimpleNestingClass { + class NestedClass { + private String name = "nested name contents"; + } + + public String getNestedName() { + return new NestedClass().name; // Should force creation of a synthetic method for NestedClass (to access its name field). + } + } + + /** + * Test that normal non-synthetic methods are considered callable by both isMethodCallable() and isMethodCallable_BridgeOrNonSynthetic(). + */ + public void testConfirmStandardMethodCallability() { + Method method = null; + try { + method = SimplePublicClass.class.getDeclaredMethod("getName", (Class[]) null); + } catch (NoSuchMethodException nsme) { + fail("SimplePublicClass.getName() method retrieval by reflection failed (NoSuchMethodException) ?"); + } + assertNotNull("getName() method retrieval failed ?", method); + assertTrue("SimplePublicClass.getName() is a synthetic or bridge method ?", !(method.isBridge() || method.isSynthetic())); + assertTrue("SimplePublicClass.getName() is not considered callable by isMethodCallable() ?", OgnlRuntime.isMethodCallable(method)); + assertTrue("SimplePublicClass.getName() is not considered callable by isMethodCallable_BridgeOrNonSynthetic() ?", OgnlRuntime.isMethodCallable_BridgeOrNonSynthetic(method)); + } + + /** + * Test that bridge methods ARE considered callable by isMethodCallable_BridgeOrNonSynthetic() ONLY, and NOT by isMethodCallable(). + */ + public void testConfirmBridgeMethodCallability() { + Method method = null; + try { + method = PublicChild.class.getDeclaredMethod("getName", (Class[]) null); + } catch (NoSuchMethodException nsme) { + fail("PublicChild.getName() method retrieval by reflection failed (NoSuchMethodException) ?"); + } + assertNotNull("getName() method retrieval failed ?", method); + assertTrue("PublicChild.getName() is not a bridge method ?", method.isBridge()); + assertFalse("PublicChild.getName() is considered callable by isMethodCallable() ?", OgnlRuntime.isMethodCallable(method)); + assertTrue("PublicChild.getName() is not considered callable by isMethodCallable_BridgeOrNonSynthetic() ?", OgnlRuntime.isMethodCallable_BridgeOrNonSynthetic(method)); + + try { + Class[] argumentTypes = {String.class}; + method = PublicChild.class.getDeclaredMethod("setName", argumentTypes); + } catch (NoSuchMethodException nsme) { + fail("PublicChild.setName() method retrieval by reflection failed (NoSuchMethodException) ?"); + } + assertNotNull("setName() method retrieval failed ?", method); + assertTrue("PublicChild.setName() is not a bridge method ?", method.isBridge()); + assertFalse("PublicChild.setName() is considered callable by isMethodCallable() ?", OgnlRuntime.isMethodCallable(method)); + assertTrue("PublicChild.setName() is not considered callable by isMethodCallable_BridgeOrNonSynthetic() ?", OgnlRuntime.isMethodCallable_BridgeOrNonSynthetic(method)); + } + + /** + * Test that non-bridge synthetic methods are NOT considered callable by either isMethodCallable() or isMethodCallable_BridgeOrNonSynthetic(). + */ + public void testConfirmSyntheticMethodNonCallablility() { + Method method; + Method[] methods = SimpleNestingClass.NestedClass.class.getDeclaredMethods(); + assertNotNull("Nested class has no methods ?", methods); + assertTrue("Nested class has no methods ?", methods.length > 0); + method = methods[0]; + assertNotNull("Nested class method at index 0 is null ?", method); + assertTrue("SimpleAbstractClass.getName() is a synthetic method ?", method.isSynthetic()); + assertFalse("SimpleAbstractClass.getName() is a bridge method ?", method.isBridge()); + assertFalse("SimpleAbstractClass.getName() is considered callable by isMethodCallable() ?", OgnlRuntime.isMethodCallable(method)); + assertFalse("SimpleAbstractClass.getName() is considered callable by isMethodCallable_BridgeOrNonSynthetic() ?", OgnlRuntime.isMethodCallable_BridgeOrNonSynthetic(method)); + } } From e8821cc0a0c374d609764bbaf523276a60cef0dc Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Sun, 25 Oct 2020 12:11:10 -0400 Subject: [PATCH 2/2] - Add @since tag to identify when isMethodCallable_BridgeOrNonSynthetic() was introduced. --- src/main/java/ognl/OgnlRuntime.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/ognl/OgnlRuntime.java b/src/main/java/ognl/OgnlRuntime.java index 4affab09..f5d76723 100644 --- a/src/main/java/ognl/OgnlRuntime.java +++ b/src/main/java/ognl/OgnlRuntime.java @@ -2772,6 +2772,8 @@ static boolean isMethodCallable(Method m) * * Synthetic methods should be excluded in general, since calling such methods * could introduce unanticipated risks. + * + * @since 3.2.16 * * @param m The method to check. * @return True if the method should be callable (non-synethetic or bridge), false otherwise.