10 Replies Latest reply on Jun 21, 2007 5:19 PM by kabirkhan

    IllegalAccessException using reflection on private field

    jxhill

      I have an app that relies heavily on reflection. I want to use AOP with it, but I get an error when accessing a private variable through java.lang.reflect.Field. It appears that aspects disables the call to Field.setAccessible(); The following sample code works fine until I apply aspects.

      // TestObject.java
      package test;
      
      public class TestObject
      {
       @SuppressWarnings("unused")
       private String testField = "Initial Value";
      }
      

      // TestAspect.java
      package test;
      
      import org.jboss.aop.joinpoint.FieldReadInvocation;
      import org.jboss.aop.joinpoint.FieldWriteInvocation;
      
      public class TestAspect
      {
       public Object accessField(FieldReadInvocation invocation) throws Throwable
       {
       return invocation.invokeNext();
       }
      
       public Object accessField(FieldWriteInvocation invocation) throws Throwable
       {
       return invocation.invokeNext();
       }
      }
      

      // Main.java
      package test;
      
      import java.lang.reflect.Field;
      
      public class Main
      {
       public static void main(String[] args)
       {
       TestObject test = new TestObject();
      
       fieldReadTest(test);
       out("");
       fieldWriteTest(test, "Some new value");
       out("");
       fieldReadTest(test);
       }
      
       public static void fieldWriteTest(TestObject test, Object newValue)
       {
       infoOut("Begin Field Write Test");
      
       try
       {
       Field f = TestObject.class.getDeclaredField("testField");
       f.setAccessible(true);
       f.set(test, newValue);
       infoOut("End Field Write Test - Success")
      ; }
       catch(Exception e)
       {
       out(e);
       infoOut("End Field Write Test - FAILED");
       }
       }
      
       public static void fieldReadTest(TestObject test)
       {
       infoOut("Begin Field Read Test");
      
       try
       {
       Field f = TestObject.class.getDeclaredField("testField");
       f.setAccessible(true);
       System.out.println(String.valueOf(f.get(test)));
       infoOut("End Field Read Test - Success")
      ; }
       catch(Exception e)
       {
       out(e);
       infoOut("End Field Read Test - FAILED");
       }
       }
      
       private static void infoOut(String s)
       {
       System.out.println("[" + s + "]");
       }
      
       private static void out(Object o)
       {
       System.out.println(o.toString());
       }
      }
      


      jboss-aop.xml
      <?xml version="1.0" encoding="UTF-8" standalone="yes"?>
      <aop>
       <aspect name="ReflectionAspect" class="org.jboss.aop.reflection.ReflectionAspect" scope="PER_VM"/>
      
       <bind pointcut="call(* java.lang.reflect.Field->get*(..))">
       <advice name="interceptFieldGet" aspect="ReflectionAspect"/>
       </bind>
      
       <bind pointcut="call(* java.lang.reflect.Field->set*(..))">
       <advice name="interceptFieldSet" aspect="ReflectionAspect"/>
       </bind>
      
       <aspect class="test.TestAspect" scope="PER_VM"/>
      
       <bind pointcut="field(* test.TestObject->*)">
       <advice name="accessField" aspect="test.TestAspect"/>
       </bind>
      </aop>
      


      Am I doing something wrong, or is this a bug?

        • 1. Re: IllegalAccessException using reflection on private field
          flavia.rainone

          Hello,

          I've taken a look at your problem and at the bug report you added to Jira (http://jira.jboss.com/jira/browse/JBAOP-405).

          The cause of your problem is not the fact that setAccessible doesn't take place. On the contrary, it does take place and has the desired effect (it allows calls to Field.get() and Field.set() methods). But, in your code, setting the field as accessible is unnecessary, since JBoss AOP will call a wrapper method it has generated instead of Field.set() and Field.get() methods. JBoss AOP will do this in order to trigger your TestAspect.

          The problem here is that this wrapper is generated as a private method inside TestObject class, and therefore it is not accessible from ReflectionAspect, which is a bug.

          While we take a look at the problem, you can intercept reflection calls using the other approach ReflectionAspect provides: to extend ReflectionAspect and override interceptFieldWrite and interceptFieldRead methods.

          // TestAspect.java
          package test;
          
          public class TestAspect extends ReflectionAspect
          {
           public Object interceptFieldRead(Invocation invocation, Field field, Object instance) throws Throwable
           {
           System.out.println(instance.getClass().getName() + "." + field.getName() + " being read on instance " + instance);
           return invocation.invokeNext();
           }
          
           public Object interceptFieldWrite(Invocation invocation, Field field, Object instance, Object arg) throws Throwable
           {
           System.out.println(instance.getClass().getName() + "." + field.getName() + " being set on instance " + instance + " with the following value \"" + arg + "\"");
           return invocation.invokeNext();
           }
           }
          


          With the approach above, just replace your jboss-aop.xml file by the following:
          <?xml version="1.0" encoding="UTF-8" standalone="yes"?>
          <aop>
           <aspect name="MyReflectionAspect" class="MyReflectionAspect" scope="PER_VM"/>
          
           <bind pointcut="call(* java.lang.reflect.Field->get*(..))">
           <advice name="interceptFieldGet" aspect="MyReflectionAspect"/>
           </bind>
          
           <bind pointcut="call(* java.lang.reflect.Field->set*(..))">
           <advice name="interceptFieldSet" aspect="MyReflectionAspect"/>
           </bind>
          </aop>
          


          This way, JBoss AOP won't perform a call to the wrapper method; it will proceed to Field.set() and Field.get() executions. Notice that, because of this, you need to keep your setAccessible calls.

          • 2. Re: IllegalAccessException using reflection on private field
            jxhill

            Flavia,

            Thanks for the quick response, I will use your workaround until the bug is fixed.

            I have one comment. If you correct the problem by making the wrapper method public, then won't the field be accessible through reflection without calling Field.setAccessible() first? That would be wrong because then you could bypass the security manager when accessing a private field. Any bug fix should require a call to setAccessible() before allowing access to a private field. Just a thought.

            Thanks

            • 3. Re: IllegalAccessException using reflection on private field
              flavia.rainone

              Making the wrapper public is a very delicate question. On one hand, there is the fact that making it private might be incompatible with the security configuration of the user's application. One doesn't have to be aware that a private method will be generated inside his/her class, nor that ReflectionAspect needs security permission to make that method accessible. On the other hand, we have the problem you said, a client can call reflectively the wrapper if it is public. But, again, the client shouldn't know that this method exists, and its name is purposely awkward for that matter.

              We can also write a public wrapper that calls Field.set and Field.get.. This one could be public, because Field.set and Field.get would only be accessible if the field was accessible.

              All this needs to be discussed... thanks for your input on the subject :).

              Let's wait for Kabir to return from his training, so we can know what he makes of this.

              • 4. Re: IllegalAccessException using reflection on private field
                jxhill

                Whichever way you choose to go, let me know if I can help. I am impressed with the AOP project and would like to contribute to it's improvement.

                • 5. Re: IllegalAccessException using reflection on private field
                  kabirkhan

                  Hi there,

                  your fix looks fine, thank you! If you want to contribute would you mind adding some tests to test this case to the ReflectionTester? Once that is done, I can add your fix to the aop source tree. Otherwise it will take a bit longer since I then need to write the tests :-) I think what you have done will also affect private constructors being called by reflection, since they also get a wrapper method, so if you really want to help it would be great if you could look at that too. Funnily enough the ReflectionAspect was the first thing I worked on when I started contributing to JBoss AOP :-)

                  To check out aop source
                  svn co http://anonsvn.jboss.org/repos/jbossas/projects/aop/trunk/ jboss-aop

                  The reflection test lives under jboss-aop/aop/src/main/org/jboss/aop/reflection/

                  And it is configured under
                  jboss-aop/aop/src/resources/test/reflection/jboss-aop.xml

                  Let me know if you need more info about how to build/run the tests?

                  BTW things like

                  m.setAccessible(true);

                  will not work if run under a security manager, and the aop tests are being run with a security manager enabled.

                  Take a look at the ReflectionAspect.SecurityAction class, which currently wraps calls to Class.getDeclaredFields() etc. when called with and without a security manager.

                  • 6. Re: IllegalAccessException using reflection on private field
                    jxhill

                    I attached a test case patch to the bug report (My first JUnit - so go easy on me) The test case only deals with situations where a security manager is not present. I will have to add a security manager test case later.

                    I'm not opposed to making the function public, in fact, after writing the test, I think that might be the way to go. I just think that if it is made public, code should be added to verify that the caller should be able to access a non-visible field.

                    I don't like the way that the code fix I added causes a superfluous call to the security manager: m.setAccessable(). I want to correct that. I will also look at the private constructor issue.

                    • 7. Re: IllegalAccessException using reflection on private field
                      kabirkhan

                      Thanks. We will try to integrate it shortly

                      • 8. Re: IllegalAccessException using reflection on private field
                        kabirkhan

                        Hi Jason,

                        I tried integrating this into AOP 2.0.0.alpha5 (releasing shortly), but am getting some problems in the ExtendedSetAccessible tests. I've attached the diffs from my machine after merging your code into the main codebase and making a few cosmetic changes.

                        • 9. Re: IllegalAccessException using reflection on private field
                          jxhill

                          Hey Kabir,

                          Sorry, I've been out of it for awhile. I have a major project at work that is due at the end of the month - our first JBoss project to move to production (-:

                          This was my first JUnit test, so I'm sure I made mistakes.

                          I should have more time by the middle of July to look at this stuff again and to help with other bug fixes.

                          Jason

                          • 10. Re: IllegalAccessException using reflection on private field
                            kabirkhan

                            As far as I can remember, the test was ok, but some more work needs doing on the aspect itself. Feel free to dip in whenever you have the time, we're always happy for extra help! If you hang around, and are interested we're pretty easy when it comes to giving away commit rights to svn, a few patches and you are there.

                            I've scheduled the JIRA issue for CR1 (mid-August)