Commons Logging
  1. Commons Logging
  2. LOGGING-25

[logging] call to getClassLoader() in LogFactoryImpl not checked for null

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.4
    • Fix Version/s: 1.1.0
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      In line 374 of LogFactoryImpl.java getClassLoader() is called:

      logInterface = this.getClass().getClassLoader().loadClass(LOG_INTERFACE);

      However, the docs for getClassLoader() state that some implementations may use
      null to return the system classloader. This occurs under CrEme a JVM for the
      PocketPC platform which some of our products run under, causing a null pointer
      exception. Perhaps it would be better to change line 374 to read:

      logClass = loadClass(LOG_INTERFACE);

      which seems to solve the problems I have been having. At any rate calls to
      getClassLoader() should be checked to ensure that they haven't returned null.

      In addition the error that I got:
      org.apache.commons.logging.LogConfigurationException:
      org.apache.commons.logging.LogConfigurationException:
      java.lang.NullPointerException (Caused by java.lang.NullPointerException)
      (Caused by org.apache.commons.logging.LogConfigurationException:
      java.lang.NullPointerException (Caused by java.lang.NullPointerException))

      Certianly wasnt very helpfull for figuring out what is going on.

      • Luke

        Activity

        Hide
        Simon Kitching added a comment -

        Hi Luke,

        Some work has gone into the trunk version of commons-logging to support null
        classloaders. There is some effort going in right now to try to get a new
        release out. Hopefully a release candidate (probably called
        commons-logging-1.1-RC1) will be released soon; it would be great if you could
        test that.

        Show
        Simon Kitching added a comment - Hi Luke, Some work has gone into the trunk version of commons-logging to support null classloaders. There is some effort going in right now to try to get a new release out. Hopefully a release candidate (probably called commons-logging-1.1-RC1) will be released soon; it would be great if you could test that.
        Hide
        Luke Sleeman added a comment -

        will do!

        Show
        Luke Sleeman added a comment - will do!
        Hide
        Simon Kitching added a comment -

        Hi Luke,

        Release candidate 1 of commons-logging 1.1 has now been created:
        http://people.apache.org/~rdonkin/commons-logging/

        Could you please check whether this meets your requirements?

        Show
        Simon Kitching added a comment - Hi Luke, Release candidate 1 of commons-logging 1.1 has now been created: http://people.apache.org/~rdonkin/commons-logging/ Could you please check whether this meets your requirements?
        Hide
        Luke Sleeman added a comment -

        Hey,
        I've downloaded and tried to use the RC1. Unfortunately the method:

        private static Enumeration getResources(final ClassLoader loader, final String name)

        inside LogFactory calls the getResource and getSystemResource methods on
        ClassLoader. These methods were only added in java 1.2 and since I am trying to
        run commons logging on a 1.1 JVM I get NoSuchMethodErrors thrown.

        Depending on what version of Java commons logging is supposed to run on this
        may, or may not be a bug.

        In case you are curious we are trying to develop java applications on the
        PocketPC, using the CrEme virtual machine:
        http://www.nsicom.com/Default.aspx?tabid=138

        Show
        Luke Sleeman added a comment - Hey, I've downloaded and tried to use the RC1. Unfortunately the method: private static Enumeration getResources(final ClassLoader loader, final String name) inside LogFactory calls the getResource and getSystemResource methods on ClassLoader. These methods were only added in java 1.2 and since I am trying to run commons logging on a 1.1 JVM I get NoSuchMethodErrors thrown. Depending on what version of Java commons logging is supposed to run on this may, or may not be a bug. In case you are curious we are trying to develop java applications on the PocketPC, using the CrEme virtual machine: http://www.nsicom.com/Default.aspx?tabid=138
        Hide
        rdonkin@apache.org added a comment -

        JCL uses the thread context classloader and the hierarchical class loader model
        introduced in Java 1.2.

        My expectation is that the JCL implementation code would fail under Java 1.1 but
        that a API compatible static bound alternative would work. (Basically, taken
        LogFactory and gut the complex classloading logic then recompile.) Perhaps it's
        time that we considered hosting such an implementation in the commons.

        Perhaps your JVM has a number of extra backported 1.2 methods (but not the few
        that JCL uses).

        Show
        rdonkin@apache.org added a comment - JCL uses the thread context classloader and the hierarchical class loader model introduced in Java 1.2. My expectation is that the JCL implementation code would fail under Java 1.1 but that a API compatible static bound alternative would work. (Basically, taken LogFactory and gut the complex classloading logic then recompile.) Perhaps it's time that we considered hosting such an implementation in the commons. Perhaps your JVM has a number of extra backported 1.2 methods (but not the few that JCL uses).
        Hide
        Simon Kitching added a comment -

        Thanks for the info Luke.

        I would like to see JCL 1.1 be compatible with java 1.1, though it isn't a
        pre-requisite for the release.

        As Robert says, a statically-bound version should work. As soon as the 1.1
        release is made, I'm keen to get started on a JCL 2.0 implementation that would
        use some kind of "static binding", ie one jar per implementation. In this case,
        there would be a simple non-TCCL-aware jar that you could use, and a separate
        TCCL-aware jar for people using servlet container style frameworks.

        However it isn't too difficult to just catch the NoSuchMethodError in this case.
        I've committed a change to do that (r375866), so if you could test by building
        from source, using a nightly build or the next release candidate that would be
        appreciated.

        Show
        Simon Kitching added a comment - Thanks for the info Luke. I would like to see JCL 1.1 be compatible with java 1.1, though it isn't a pre-requisite for the release. As Robert says, a statically-bound version should work. As soon as the 1.1 release is made, I'm keen to get started on a JCL 2.0 implementation that would use some kind of "static binding", ie one jar per implementation. In this case, there would be a simple non-TCCL-aware jar that you could use, and a separate TCCL-aware jar for people using servlet container style frameworks. However it isn't too difficult to just catch the NoSuchMethodError in this case. I've committed a change to do that (r375866), so if you could test by building from source, using a nightly build or the next release candidate that would be appreciated.
        Hide
        rdonkin@apache.org added a comment -

        Hi Luke,

        Release candidate 4 of commons-logging 1.1 has now been created:
        http://people.apache.org/~rdonkin/commons-logging/

        It contains Simon's fix.

        Could you please check whether this is any better?

        Robert

        Show
        rdonkin@apache.org added a comment - Hi Luke, Release candidate 4 of commons-logging 1.1 has now been created: http://people.apache.org/~rdonkin/commons-logging/ It contains Simon's fix. Could you please check whether this is any better? Robert
        Hide
        Luke Sleeman added a comment -

        Thanks for your help, I'll check it out when I get a free moment at work.

        The staticaly bound version for 2.0 sounds like a good idea. Doing a quick
        search around google returns a large number of people complaing about commons
        loggings complex classloading stuff.

        Show
        Luke Sleeman added a comment - Thanks for your help, I'll check it out when I get a free moment at work. The staticaly bound version for 2.0 sounds like a good idea. Doing a quick search around google returns a large number of people complaing about commons loggings complex classloading stuff.
        Hide
        rdonkin@apache.org added a comment -

        Sadly, there are no silver bullets.

        It's very easy to complain but much more difficult to understand the problem in
        detail. So, I should probably thank you know for being one of the tiny minority
        who step up and help

        JCL is incredibly widely used. This makes things very difficult. The issue for
        JCL is (and has always been) that the J2EE classloading specifications are broken.

        Static binding has it's own limitations. It also has it's own issues. The vast
        majority of problems experienced by users are caused by having JCL jars in
        different classloaders they are not aware of. Static binding will not solve this
        problem.

        Dynamic binding addresses application isolation in well behaved containers in a
        fashion that is impossible for compile time static binding. Application
        isolation should be possible by using byte code engineering to perform static
        binding but that has it's own set of issues.

        So, no silver bullets but maybe we can make things a little less painful...

        Show
        rdonkin@apache.org added a comment - Sadly, there are no silver bullets. It's very easy to complain but much more difficult to understand the problem in detail. So, I should probably thank you know for being one of the tiny minority who step up and help JCL is incredibly widely used. This makes things very difficult. The issue for JCL is (and has always been) that the J2EE classloading specifications are broken. Static binding has it's own limitations. It also has it's own issues. The vast majority of problems experienced by users are caused by having JCL jars in different classloaders they are not aware of. Static binding will not solve this problem. Dynamic binding addresses application isolation in well behaved containers in a fashion that is impossible for compile time static binding. Application isolation should be possible by using byte code engineering to perform static binding but that has it's own set of issues. So, no silver bullets but maybe we can make things a little less painful...
        Hide
        rdonkin@apache.org added a comment -

        Waiting for retest with latest release candidate.

        Show
        rdonkin@apache.org added a comment - Waiting for retest with latest release candidate.
        Hide
        Simon Kitching added a comment -

        As there has been no response from original poster, this is being closed presumed fixed.

        Show
        Simon Kitching added a comment - As there has been no response from original poster, this is being closed presumed fixed.
        Hide
        David Smiley added a comment -

        This hasn't been fixed. This problem can be seen using Apple's VM on Mac OS X if JCL is on the bootclasspath (such as via -Xbootclasspath). I looked in source control for JCL and I see that the result is not checked for null. The reference is:
        LogFactory.getClassLoader(classname).
        What needs to happen is that the result in there needs to be checked for null, and if it is then return ClassLoader.getSystemClassLoader(). At least there is only one place to make this fix now that all of JCL uses this method to get a ClassLoader, unlike how it used to work.

        Show
        David Smiley added a comment - This hasn't been fixed. This problem can be seen using Apple's VM on Mac OS X if JCL is on the bootclasspath (such as via -Xbootclasspath). I looked in source control for JCL and I see that the result is not checked for null. The reference is: LogFactory.getClassLoader(classname). What needs to happen is that the result in there needs to be checked for null, and if it is then return ClassLoader.getSystemClassLoader(). At least there is only one place to make this fix now that all of JCL uses this method to get a ClassLoader, unlike how it used to work.
        Hide
        David Smiley added a comment -

        FYI, here's the new code that should go into the try/catch block:
        ClassLoader result = clazz.getClassLoader();
        if (result == null)
        result = ClassLoader.getSystemClassLoader();
        if (result == null)
        throw new IllegalStateException("ClassLoader.getSystemClassLoader() returned null!");
        return result;

        It works for me.

        Show
        David Smiley added a comment - FYI, here's the new code that should go into the try/catch block: ClassLoader result = clazz.getClassLoader(); if (result == null) result = ClassLoader.getSystemClassLoader(); if (result == null) throw new IllegalStateException("ClassLoader.getSystemClassLoader() returned null!"); return result; It works for me.
        Hide
        Simon Kitching added a comment -

        Reopen due to report by David Smiley

        Show
        Simon Kitching added a comment - Reopen due to report by David Smiley
        Hide
        Simon Kitching added a comment -

        Applying this patch would cause some undesirable side-effects. For example, method logClassLoaderEnvironment uses this method to print diagnostic info. With this patch, we would report that a certain class was loaded from the system classloader when it is really loaded from the bootclassloader.

        I think it's better to fix the place(s) where we try to dereference this loader without checking for null. Presumably you get an exception with a stack trace when this occurs. Can you please run the standard JCL 1.1.1 release with the following code and post the resulting exception? (please also include the output of "java -version")

        import org.apache.commons.logging.Log;
        import org.apache.commons.logging.LogFactory;

        public class Tester {
        public static Log log = LogFactory.getLog("foo");
        public static void main(String[] args)

        { log.error("testing"); }

        }

        java -Xbootclasspath/a:commons-logging-1.1.1.jar Tester

        BTW, running the 1.1.1 release on Linux/java1.5 using -Xbootclasspath/a:commons-logging-1.1.1.jar works fine. I'm pretty sure the Apple JVM is just the Sun source code licensed by Apple and tweaked, so don't know what could be causing a difference on Apple.

        Thanks,

        Simon

        Show
        Simon Kitching added a comment - Applying this patch would cause some undesirable side-effects. For example, method logClassLoaderEnvironment uses this method to print diagnostic info. With this patch, we would report that a certain class was loaded from the system classloader when it is really loaded from the bootclassloader. I think it's better to fix the place(s) where we try to dereference this loader without checking for null. Presumably you get an exception with a stack trace when this occurs. Can you please run the standard JCL 1.1.1 release with the following code and post the resulting exception? (please also include the output of "java -version") import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; public class Tester { public static Log log = LogFactory.getLog("foo"); public static void main(String[] args) { log.error("testing"); } } java -Xbootclasspath/a:commons-logging-1.1.1.jar Tester BTW, running the 1.1.1 release on Linux/java1.5 using -Xbootclasspath/a:commons-logging-1.1.1.jar works fine. I'm pretty sure the Apple JVM is just the Sun source code licensed by Apple and tweaked, so don't know what could be causing a difference on Apple. Thanks, Simon
        Hide
        David Smiley added a comment -

        Sorry... it appears that the problem was with v1.0.4 and not 1.1 or later. I thought I tried 1.1 in my environment and it didn't work but I just did now and all is well. So sorry for re-opening this--problem solved.

        Show
        David Smiley added a comment - Sorry... it appears that the problem was with v1.0.4 and not 1.1 or later. I thought I tried 1.1 in my environment and it didn't work but I just did now and all is well. So sorry for re-opening this--problem solved.
        Hide
        Simon Kitching added a comment -

        That's ok David; I've done that before . Marking as closed/fixed.

        Show
        Simon Kitching added a comment - That's ok David; I've done that before . Marking as closed/fixed.

          People

          • Assignee:
            Unassigned
            Reporter:
            Luke Sleeman
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development