not fail on getDeclaredXXX when user has used a SecurityManager #79

yasserzamani · 2019-08-13T11:17:06Z

Falls back to getXXX on SecurityException to be more adaptable for environments that have a SecurityManager applied.

JCgH4164838Gh792C124B5 · 2019-08-14T19:38:42Z

Hello @yasserzamani .
This change could cause different behaviour for environments using a SecurityManager vs. ones that are not, couldn't it (due to returning a different list of methods) ?
If an application/user's security manager blocks access to declared methods it might be better to let it fail and force the application to adjust its security manager to allow the needed access ?

yasserzamani · 2019-08-15T06:26:14Z

On 8/15/2019 12:08 AM, JCgH4164838Gh792C124B5 wrote: Hello @yasserzamani <https://github.com/yasserzamani> . This change could cause different behaviour for environments using a SecurityManager vs. ones that are not, couldn't it (due to returning a different list of methods) ?

Howdy, If the SecurityManager doesn't allow "suppressaccesscheck", yes it will. But I think it's a nice adaption rather than a back-ward compatibility issue.

If an application/user's security manager blocks access to declared methods it might be better to let it fail and force the application to adjust its security manager to allow the needed access ?

She already can allow the needed access then OGNL will operate exactly as before. This change takes the control and adapts only when there are a SecurityManager and that doesn't allow "suppressaccesscheck" elsewhere (i.e. when there are no SecurityManager OR there are but does allow "suppressaccesscheck") OGNL will operate exactly as before. Regards.

lukaszlenart · 2019-09-02T08:11:42Z

Looks like we must switch to JDK7

[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] Source option 5 is no longer supported. Use 6 or later.
[ERROR] Target option 1.5 is no longer supported. Use 1.6 or later.
[INFO] 2 errors

yasserzamani · 2019-09-07T09:26:19Z

It seems Travis environment doesn't support 5 and 1.5 for source and target option anymore. Is it OK to change them to 6 and 1.6 in pom.xml? I think it should be fine because java 5 is obviously absolute.

lukaszlenart · 2019-09-07T09:39:59Z

I was sure I was even using 1.7 but cannot figure out where it go :\

yasserzamani · 2019-09-07T09:50:48Z

I skimmed pom.xml in different branches and realized it's 7 only in master :)

yasserzamani · 2019-09-07T10:34:33Z

fixed via switching to 1.6 :)

JCgH4164838Gh792C124B5

The PR does appear to have the potential to improve behaviour under the circumstances it identified, but at the cost of extra overhead the rest of the time (e.g. when no SecurityManager is installed).

Someone might have a different suggestion, but the one provided could reduce the overhead a bit.

JCgH4164838Gh792C124B5 · 2019-09-09T01:50:28Z

src/java/ognl/OgnlRuntime.java

@@ -1861,7 +1861,12 @@ public static Map getMethods(Class targetClass, boolean staticMethods)
    }

    private static void collectMethods(Class c, Map result, boolean staticMethods) {
-        Method[] ma = c.getDeclaredMethods();
+        Method[] ma;


Since there is loop processing involved below, there will probably be extra overhead for processing the try..catch multiple times within the loop. Since we're attempting the getDeclaredMethods vs. getMethods checks here it should be possible to set a boolean flag that determines which methodology the current SecurityManager allows.

boolean smSupportsGetDeclaredMethods = true; try { ma = c.getDeclaredMethods(); } catch (SecurityException ignored) { smSupportsGetDeclaredMethods = false; ma = c.getMethods(); }

JCgH4164838Gh792C124B5 · 2019-09-09T01:55:18Z

src/java/ognl/OgnlRuntime.java

@@ -1966,7 +1971,11 @@ public static Map getFields(Class targetClass)
                    Field fa[];

                    result = new HashMap(23);
-                    fa = targetClass.getDeclaredFields();
+                    try {


And then in the loop here the try..catch code could be replaced by something like:

smSupportsGetDeclaredMethods ? fa = targetClass.getDeclaredFields() : fa = targetClass.getFields();

which limits the overhead of looping over a try..catch when the determination of which mechanism to use is known (someone would have to look at the bytecode to be certain, but a boolean check should be more efficient, especially over multiple iterations).

Thanks! I got your idea but they aren't in same scope (method). Their call order isn't guaranteed. Furthermore, I'm not going to use a global flag because user might remove security manager at runtime after some calls. At bottom, according to my googling and experience, java security checks aren't expensive.

Hi @yasserzamani . I know this PR has already been merged, but I wanted to respond to make sure I understood your reply.

Ooops ... I missed that the two code blocks were in separate methods, and interpreted them as being in the same block scope with a loop involved. 😥 Since there's not actually any looping over a try..catch involved the new logic shouldn't introduce any noticeable overhead (especially since post-computation results are cached).

Thanks for clarifying and pointing out my mis-interpretation. 😄

Cool! Frankly I had overlooked the performance. Thanks for your acknowledgement.

JCgH4164838Gh792C124B5 · 2019-09-09T01:58:56Z

src/java/ognl/OgnlRuntime.java

@@ -2264,7 +2273,12 @@ public static List getDeclaredMethods(Class targetClass, String propertyName, bo

    private static void collectAccessors(Class c, String baseName, List result, boolean findSets)
    {
-        final Method[] methods = c.getDeclaredMethods();
+        Method[] methods;
+        try{


Could change try{ to try { (whitespace to match the others in the PR)

@yasserzamani this tiny thing and I'm good to merge this PR :)

yasserzamani · 2019-09-12T13:04:09Z

fixed :+1 Now this will be a good candidate to squash :) Thanks!

lukaszlenart · 2019-09-13T10:20:15Z

Ok, now I can release a new OGNL 3.1.x version, thanks!

yasserzamani · 2019-09-13T13:26:58Z

Yes please. Then I will adapt Struts 2.5.x to be less brittle with Security Managers via a PR :)

…an-oss#79) not fail on getDeclaredXXX when user has used a SecurityManager (cherry picked from commit 32d076d)

cherry pick #79 and #82 and related commits

not fail on getDeclaredXXX when user has used a SecurityManager

44d4038

Falls back to getXXX on SecurityException to be more adaptable for environments that have a SecurityManager applied.

fix Travis build

88a2209

JCgH4164838Gh792C124B5 suggested changes Sep 9, 2019

View reviewed changes

JCgH4164838Gh792C124B5 mentioned this pull request Sep 9, 2019

OGNL improve getter/setter detection algorithm: #75

Merged

format code

8853a98

lukaszlenart merged commit 32d076d into orphan-oss:ognl-3-1-x Sep 13, 2019

yasserzamani deleted the ognl_3_1_SecurityManager_getDeclaredXXX branch September 14, 2019 11:29

yasserzamani added a commit to yasserzamani/ognl that referenced this pull request Dec 16, 2019

not fail on getDeclaredXXX when user has used a SecurityManager (orph…

a973c19

…an-oss#79) not fail on getDeclaredXXX when user has used a SecurityManager (cherry picked from commit 32d076d)

lukaszlenart added a commit that referenced this pull request Dec 16, 2019

Merge pull request #94 from yasserzamani/PR79PR82

0c614d5

cherry pick #79 and #82 and related commits

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

not fail on getDeclaredXXX when user has used a SecurityManager #79

not fail on getDeclaredXXX when user has used a SecurityManager #79

yasserzamani commented Aug 13, 2019

JCgH4164838Gh792C124B5 commented Aug 14, 2019

yasserzamani commented Aug 15, 2019 via email

lukaszlenart commented Sep 2, 2019

yasserzamani commented Sep 7, 2019

lukaszlenart commented Sep 7, 2019

yasserzamani commented Sep 7, 2019

yasserzamani commented Sep 7, 2019

JCgH4164838Gh792C124B5 left a comment

JCgH4164838Gh792C124B5 Sep 9, 2019

JCgH4164838Gh792C124B5 Sep 9, 2019

yasserzamani Sep 11, 2019

JCgH4164838Gh792C124B5 Sep 14, 2019

yasserzamani Sep 15, 2019

JCgH4164838Gh792C124B5 Sep 9, 2019

lukaszlenart Sep 12, 2019

yasserzamani commented Sep 12, 2019 via email

lukaszlenart commented Sep 13, 2019

yasserzamani commented Sep 13, 2019 via email

not fail on getDeclaredXXX when user has used a SecurityManager #79

not fail on getDeclaredXXX when user has used a SecurityManager #79

Conversation

yasserzamani commented Aug 13, 2019

JCgH4164838Gh792C124B5 commented Aug 14, 2019

yasserzamani commented Aug 15, 2019 via email

lukaszlenart commented Sep 2, 2019

yasserzamani commented Sep 7, 2019

lukaszlenart commented Sep 7, 2019

yasserzamani commented Sep 7, 2019

yasserzamani commented Sep 7, 2019

JCgH4164838Gh792C124B5 left a comment

Choose a reason for hiding this comment

JCgH4164838Gh792C124B5 Sep 9, 2019

Choose a reason for hiding this comment

JCgH4164838Gh792C124B5 Sep 9, 2019

Choose a reason for hiding this comment

yasserzamani Sep 11, 2019

Choose a reason for hiding this comment

JCgH4164838Gh792C124B5 Sep 14, 2019

Choose a reason for hiding this comment

yasserzamani Sep 15, 2019

Choose a reason for hiding this comment

JCgH4164838Gh792C124B5 Sep 9, 2019

Choose a reason for hiding this comment

lukaszlenart Sep 12, 2019

Choose a reason for hiding this comment

yasserzamani commented Sep 12, 2019 via email

lukaszlenart commented Sep 13, 2019

yasserzamani commented Sep 13, 2019 via email